Fix chrome://network CSS for cr-network-icon

Fix chrome://network CSS for cr-network-icom

BUG=455499
Committed: https://crrev.com/dde8985ac86c397072fbba013194fb44e089c4dc
Cr-Commit-Position: refs/heads/master@{#318978}

[stevenjb, 2015-03-03 17:41:38]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2015-03-03 21:09:09]

Sorry for the zillion-line comment but this stuff is confusing. Each time I look
at vertical-align I hate it more and more. We should ban it, if it wasn't so
damn useful when it works.

It might be easier for me to walk you through this in person tomorrow.

+dbeam in case there's some simple way to fix "overflow: hidden; display:
inline-block" so it doesn't set the bottom margin to the baseline. (but if this
isn't something you've dealt with before, no worries.)

https://codereview.chromium.org/972223002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/network_ui/network_ui.css (right):

https://codereview.chromium.org/972223002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/network_ui/network_ui.css:73: vertical-align:
middle;
w00t I figured this out! (kind of)

TL;DR: It's a trap. We should update cr-network-icon to be "display: block", add
a warning there that "display: inline-block" will screw with the positioning
with a link to the spec, and remove line 73. OR do some trickery**.

===================================

I still don't know why display:flex creates moar borders. But the reason this
vertical alignment nonsense is necessary at all is: *the cr-network-icon's
bottom margin is on the baseline*, which is too high.

Setting "display: block" or adding line 73 ("vertical-align: middle") fixes
this.

My explanation follows but it gets technical:

1. Without "display: flex", the table cell vertically centers its content. This
is the default for table cells.

2. So "vertical-align: middle" here looks weird -- it should be vertically
centered anyway.

3. In fact, the icon IS vertically centered within the <td> -- it's just a few
pixels too high. (Insert a bunch of <br>s in a sibling <td> or change the height
of the cell to see this.)

4. If I change cr-network-icon from "display: inline-block" to "display: block",
it becomes fully vertically centered. ("display: inline" also centers it but
this kills the icon.)

WTF, right?

===================================

The explanation is the very last line of
http://www.w3.org/TR/CSS2/visudet.html#leading:

    The baseline of an 'inline-block' is the baseline of its
    last line box in the normal flow, unless it has either no
    in-flow line boxes or if its 'overflow' property has a
    computed value other than 'visible', in which case the
    baseline is the bottom margin edge.

Examples here: http://jsfiddle.net/5g2ejzww/1/

In the 2nd row, the baseline of the 'inline-block' is the baseline of its
containing line box -- see how the baseline matches up (ABgjy -> ABgjy).

In the 3rd row, because the 'overflow' property is 'hidden' instead of
'visible', "the baseline is the bottom margin edge" -- as in, the bottom margin
is PLACED on the baseline. See how the bottom margin of the <div> is aligned
with the baseline of the container (ABgjy).

So because cr-network-icon uses both "overflow: hidden" and "display:
inline-block", the element is shifted up onto the baseline of the table cell.

More examples here: http://jsfiddle.net/Lxofxgpt/ and you can change the array
of vertical-align values to see how different options interact. Some
combinations are "safe", others are not (they're affected by an element being
"overflow: hidden"). Further confusing things, elements in a <td> are vertically
aligned if the <td> has "vertical-align: middle" (the default), but this is a
Separate Thing from using "vertical-align: middle" on inline-block elements. So
the way cr-network-icon will align is dependent on both its own vertical-align
and sibling elements's vertical-align, *and* whether it's within a <td> or not.

===================================

I would update cr-network-icon to be "display: block", and add a warning there
that "display: inline-block" will screw with the positioning with a link to the
spec.

** Some trickery: A 3rd option might be possible. See if you can wrap the
offending "overflow: hidden" element with your cr-network-icon element. In other
words, instead of:

<cr-network-icon> // overflow: hidden, display: inline-block, height/width: 25px
  <implementation...>
</cr-network-icon>

see if we can negate the positioning weirdness by making cr-network-icon
"overflow: visible":

<cr-network-icon> // display: inline-block
  <div> // overflow: hidden, height/width: 25px
    <implementation...>
  </div>
</cr-network-icon>

Not sure this will work. I don't think it's this simple. Time to memorize the
CSS spec....

[stevenjb, 2015-03-03 21:43:01]

PTAL

https://codereview.chromium.org/972223002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/network_ui/network_ui.css (right):

https://codereview.chromium.org/972223002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/network_ui/network_ui.css:73: vertical-align:
middle;
On 2015/03/03 21:09:09, michaelpg wrote:
> w00t I figured this out! (kind of)
> 
> TL;DR: It's a trap. We should update cr-network-icon to be "display: block",
add
> a warning there that "display: inline-block" will screw with the positioning
> with a link to the spec, and remove line 73. OR do some trickery**.
> 
> ===================================
> 
> I still don't know why display:flex creates moar borders. But the reason this
> vertical alignment nonsense is necessary at all is: *the cr-network-icon's
> bottom margin is on the baseline*, which is too high.
> 
> Setting "display: block" or adding line 73 ("vertical-align: middle") fixes
> this.
> 
> My explanation follows but it gets technical:
> 
> 1. Without "display: flex", the table cell vertically centers its content.
This
> is the default for table cells.
> 
> 2. So "vertical-align: middle" here looks weird -- it should be vertically
> centered anyway.
> 
> 3. In fact, the icon IS vertically centered within the <td> -- it's just a few
> pixels too high. (Insert a bunch of <br>s in a sibling <td> or change the
height
> of the cell to see this.)
> 
> 4. If I change cr-network-icon from "display: inline-block" to "display:
block",
> it becomes fully vertically centered. ("display: inline" also centers it but
> this kills the icon.)
> 
> WTF, right?
> 
> ===================================
> 
> The explanation is the very last line of
> http://www.w3.org/TR/CSS2/visudet.html#leading:
> 
>     The baseline of an 'inline-block' is the baseline of its
>     last line box in the normal flow, unless it has either no
>     in-flow line boxes or if its 'overflow' property has a
>     computed value other than 'visible', in which case the
>     baseline is the bottom margin edge.
> 
> Examples here: http://jsfiddle.net/5g2ejzww/1/
> 
> In the 2nd row, the baseline of the 'inline-block' is the baseline of its
> containing line box -- see how the baseline matches up (ABgjy -> ABgjy).
> 
> In the 3rd row, because the 'overflow' property is 'hidden' instead of
> 'visible', "the baseline is the bottom margin edge" -- as in, the bottom
margin
> is PLACED on the baseline. See how the bottom margin of the <div> is aligned
> with the baseline of the container (ABgjy).
> 
> So because cr-network-icon uses both "overflow: hidden" and "display:
> inline-block", the element is shifted up onto the baseline of the table cell.
> 
> More examples here: http://jsfiddle.net/Lxofxgpt/ and you can change the array
> of vertical-align values to see how different options interact. Some
> combinations are "safe", others are not (they're affected by an element being
> "overflow: hidden"). Further confusing things, elements in a <td> are
vertically
> aligned if the <td> has "vertical-align: middle" (the default), but this is a
> Separate Thing from using "vertical-align: middle" on inline-block elements.
So
> the way cr-network-icon will align is dependent on both its own vertical-align
> and sibling elements's vertical-align, *and* whether it's within a <td> or
not.
> 
> ===================================
> 
> I would update cr-network-icon to be "display: block", and add a warning there
> that "display: inline-block" will screw with the positioning with a link to
the
> spec.
> 
> ** Some trickery: A 3rd option might be possible. See if you can wrap the
> offending "overflow: hidden" element with your cr-network-icon element. In
other
> words, instead of:
> 
> <cr-network-icon> // overflow: hidden, display: inline-block, height/width:
25px
>   <implementation...>
> </cr-network-icon>
> 
> see if we can negate the positioning weirdness by making cr-network-icon
> "overflow: visible":
> 
> <cr-network-icon> // display: inline-block
>   <div> // overflow: hidden, height/width: 25px
>     <implementation...>
>   </div>
> </cr-network-icon>
> 
> Not sure this will work. I don't think it's this simple. Time to memorize the
> CSS spec....

TLDR; display:block in the icon does fix this and seems perfectly reasonable to
me. Included a short comment.

[michaelpg, 2015-03-03 22:23:03]

lgtm

This works, but we may need to figure out a solution in the future if we want to
display cr-network-icon inline. It's silly that this is so difficult so
hopefully there's an easy fix. I'll post a StackOverflow question...

[stevenjb, 2015-03-03 22:53:54]

On 2015/03/03 22:23:03, michaelpg wrote:
> lgtm
> 
> This works, but we may need to figure out a solution in the future if we want
to
> display cr-network-icon inline. It's silly that this is so difficult so
> hopefully there's an easy fix. I'll post a StackOverflow question...

Doesn't the "display:block" in :host refer to the element container, i.e. how to
display the <cr-network-icon> contents, which is just the icon? It is still
possible to display an icon inline with a span (in fact we do this in the
'default-network' div in network_ui.html.

Now, if we wanted to include text as part of the element and display it
alongside the image, then we would need to wrap the image in a div and get that
to work properly, but I have no expectation of doing that.

[stevenjb, 2015-03-03 22:54:19]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-03-03 22:55:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/972223002/20001

[michaelpg, 2015-03-03 23:41:49]

I'm asking would this work:

<span>Regular image <img width=25 height=25> Network icon <cr-network-icon
style="display: inline-block"> </span>

Would the img and cr-network-icon be vertically aligned the same? I think
<cr-network-icon> will be offset. And if you just use <cr-network-icon>
it'll be a block-level element in the flow whether or not it's wrapped in
an inline element like <span>, right?

On Tue, Mar 3, 2015 at 2:55 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/972223002/20001
>
>
> https://codereview.chromium.org/972223002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2015-03-04 00:02:13]

I see. You're right, that doesn't work. I misunderstood how 'display'
works. I assumed it affected child elements, not the element itself, i.e. I
thought you could set inline-block for the containing span. (This is
something I can never seem to keep straight with CSS properties).

Our current examples wrap an icon and a span inside a div, which works
fine, but inlining within a span does not. I guess maybe we do need a div
within the cr-network-icon element to make this generally work correctly.


On Tue, Mar 3, 2015 at 3:41 PM, Michael Giuffrida <michaelpg@chromium.org>
wrote:

> I'm asking would this work:
>
> <span>Regular image <img width=25 height=25> Network icon <cr-network-icon
> style="display: inline-block"> </span>
>
> Would the img and cr-network-icon be vertically aligned the same? I think
> <cr-network-icon> will be offset. And if you just use <cr-network-icon>
> it'll be a block-level element in the flow whether or not it's wrapped in
> an inline element like <span>, right?
>
> On Tue, Mar 3, 2015 at 2:55 PM, <commit-bot@chromium.org> wrote:
>
>> CQ is trying da patch. Follow status at
>>  https://chromium-cq-status.appspot.com/patch-status/972223002/20001
>>
>>
>> https://codereview.chromium.org/972223002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dan Beam, 2015-03-04 00:15:12]

On 2015/03/04 00:02:13, stevenjb wrote:
> I see. You're right, that doesn't work. I misunderstood how 'display'
> works. I assumed it affected child elements, not the element itself, i.e. I
> thought you could set inline-block for the containing span. (This is
> something I can never seem to keep straight with CSS properties).

https://developer.mozilla.org/en-US/docs/Web/CSS/display

> 
> Our current examples wrap an icon and a span inside a div, which works
> fine, but inlining within a span does not. I guess maybe we do need a div
> within the cr-network-icon element to make this generally work correctly.
> 
> 
> On Tue, Mar 3, 2015 at 3:41 PM, Michael Giuffrida
<mailto:michaelpg@chromium.org>
> wrote:
> 
> > I'm asking would this work:
> >
> > <span>Regular image <img width=25 height=25> Network icon <cr-network-icon
> > style="display: inline-block"> </span>
> >
> > Would the img and cr-network-icon be vertically aligned the same? I think
> > <cr-network-icon> will be offset. And if you just use <cr-network-icon>
> > it'll be a block-level element in the flow whether or not it's wrapped in
> > an inline element like <span>, right?
> >
> > On Tue, Mar 3, 2015 at 2:55 PM, <mailto:commit-bot@chromium.org> wrote:
> >
> >> CQ is trying da patch. Follow status at
> >>  https://chromium-cq-status.appspot.com/patch-status/972223002/20001
> >>
> >>
> >> https://codereview.chromium.org/972223002/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-03-04 00:36:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-04 00:36:59]

Patchset 2 (id:??) landed as
https://crrev.com/dde8985ac86c397072fbba013194fb44e089c4dc
Cr-Commit-Position: refs/heads/master@{#318978}