Noissue - In the devtools panel, preserve space for missing document domain

Noissue - In the devtools panel, preserve space for missing document domain

[Sebastian Noack, 2018-04-02 18:51:29]

Currently, if the document domain is empty the URL is vertically aligned to the
middle of the row. (However, the other way around if the URL is missing, the
space for the URL is preserved and the document domain remains at the bottom of
the row).

This is also misleading because in the table header it's indicated that the top
value is the URL and the bottom value is the document domain. If one appears in
the middle of the row its ambiguous.

[Thomas Greiner, 2018-04-03 12:39:43]

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html...
devtools-panel.html:91: <div class="domain">&nbsp;</div>
This issue is purely cosmetical so, instead of not allowing this element to be
empty, why not make sure that neither of the rows here collapses if it's empty?
That way we won't have to worry about what's inside those rows.

For instance, using CSS this could be achieved with `min-height: 1em`.

[Sebastian Noack, 2018-04-03 23:21:56]

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html...
devtools-panel.html:91: <div class="domain">&nbsp;</div>
On 2018/04/03 12:39:43, Thomas Greiner wrote:
> This issue is purely cosmetical so, instead of not allowing this element to be
> empty, why not make sure that neither of the rows here collapses if it's
empty?
> That way we won't have to worry about what's inside those rows.

Well, this is how we already avoid other elements with optional content (see
"resource-link", "filter" and "origin") from collapsing. I didn't intend to do a
major refactoring here, but merely made the behavior consistent.

> For instance, using CSS this could be achieved with `min-height: 1em`.

While I agree that inserting dummy data (i.e. &nbsp) to influence the visual
representation is a hack (and against semantic markup practices), I couldn't
come up with a better way back then when I implemented the devtools panel.
(Redundantly) hard-coding the (min-)height is hardly any less of a hack.

[Thomas Greiner, 2018-04-04 12:34:01]

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html...
devtools-panel.html:91: <div class="domain">&nbsp;</div>
On 2018/04/03 23:21:56, Sebastian Noack wrote:
> Well, this is how we already avoid other elements with optional content (see
> "resource-link", "filter" and "origin") from collapsing. I didn't intend to do
a
> major refactoring here, but merely made the behavior consistent.

Sure, we should avoid any major refactoring. So if you think the suggested
change is out of scope for fixing this issue and if you want to make it
consistent, feel free to make two separate commits.

> While I agree that inserting dummy data (i.e. &nbsp) to influence the visual
> representation is a hack (and against semantic markup practices), I couldn't
> come up with a better way back then when I implemented the devtools panel.
> (Redundantly) hard-coding the (min-)height is hardly any less of a hack.

It's great that we're revisiting code to iterate on it. In this case I don't see
how defining how elements should look like in CSS is a hack. For instance, I'd
also consider it reasonable to set the anchor tag above to `display: block` to
ensure that it occupies the entire width of its parent. So the former defines
the link's height and the latter defines its width.

[Sebastian Noack, 2018-04-05 00:36:10]

This change is no longer relevant. After #6543 and #6544, all requests that end
up being logged should have a document domain.

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html
File devtools-panel.html (right):

https://codereview.adblockplus.org/29740563/diff/29740564/devtools-panel.html...
devtools-panel.html:91: <div class="domain">&nbsp;</div>
On 2018/04/04 12:34:01, Thomas Greiner wrote:
> In this case I don't see how defining how elements should look like in CSS
> is a hack.

It's a hack to superfluously define a height on the element. This seems much
more error-prone than the approach with the &nbsp;. Currently, the element's
height is defined by the font-size/line-height of its content. If we now
explicitly define a height for the element itself and later change the size of
the content, we have to remember updating the height of the container as well,
otherwise the layout is off.