1501908 - <select> label is mis-aligned in 64.0beta when using line-height + padding-top

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[bgstandaert]

Attachment: select-comparison.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

Visit https://jsfiddle.net/q3suxfbh/


Actual results:

The <select> label is not aligned correctly (see the attached image).

This causes <select> inputs to display incorrectly on an internal website I use (although the website isn't publicly accessible, so I can't share a link).


Expected results:

The label should be aligned correctly.

Mozregression output:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2fa4b07f6f7e489047808bd0238301ea943b4b3&tochange=ea09443dac2888f8c9f72964f907ccdb66e9960d

Comment 1

[Alice0775 White]

ea09443dac28	Mats Palmgren — Bug 1500609 part 2 - Make the declaration of operator<< for nsReflowStatus unconditional since it's needed to build some COLUMN_SET_LOG expression in nsColumnSetFrame.cpp (idempotent patch). r=me
e1fde2f49f84	Mats Palmgren — Bug 1500609 part 1 - Add a nsFlexContainerFrame method to remove cached data that depend on flex items' intrinsic isize (idempotent patch). r=dholbert
a9b1d69e379a	Mats Palmgren — Bug 1499230 - Make empty <select>s have zero content-box inline-size, like in other UAs. r=emilio
50a3c9a567e3	Mats Palmgren — Bug 1499578 - Make one line-height the intrinsic block-size for <select> comboboxes. r=emilio


@Mats,

Your bunch of patch srrms to cause the regression. Could you please look into this?

Comment 2

[Mats Palmgren (inactive)]

select {
height: 30px;
line-height: 30px;
padding-top: 10px;
}

Well, that combination of values makes no sense.  It looks like
the author made the assumption that <select> has 'box-sizing:content-box'
by default, but it's not, it's 'border-box'.

But yeah, it looks like other UAs don't apply 'line-height' unless
'-webkit-appearance' is 'none', so we should probably try to limit
it to that.

Comment 3

[Mats Palmgren (inactive)]

In any case, my recommendation to the author is to simply remove that rule.
It doesn't really do anything that is desirable for the end user.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase

The 'line-height' isn't really the main problem for the reported
example either, it's the padding.  And that problem also occurs
in other UAs to some extent.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Screenshot of Firefox v63 on Linux.

Here's how the first two lines in that test renders in Firefox v63 on Linux.
So the regression from applying 'line-height' is that we now don't render
those 1 or 2 pixels of text in the first control...

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix+test

Not sure if this is really worth it... the reported site is already broken
due to the padding.  But I guess it's the safe option until we can convince
other vendors to apply the specified 'line-height'...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9020230 [details] [diff] [review]
fix+test

Review of attachment 9020230 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not terribly happy about this patch, specially about the fact that the non-static CalcLineHeight() stops doing the same than the static version of it, I think that's somewhat footgunny...

Can we instead keep the weirdness in the combobox stuff, by, let's say, doing something like the following:

  auto bsize = StyleDisplay()->HasAppearance() ? mComboBox->mListControlFrame->GetBSizeOfARow() : CalcLineHeight(..);
  state.SetComputedBSize(bsize);

instead?

Relatedly, do we really want the appearance check to be explicitly for menulist or HasAppearance instead? Chrome doesn't seem to grow the height if I say, -webkit-appearance: button, for example.

Comment 8

[Mats Palmgren (inactive)]

No, we never want to use GetBSizeOfARow() for sizing the button itself,
since the <option>/<optgroup> might be styled differently.  Try this in Chrome:
data:text/html,<select><option style="font-size:48pt">BIG
(we currently don't honor that font-size but that's just a bug in our
implementation of the dropdown menu - see bug 1409613 and bug 1154677 etc.
If you use -layoutdebug on the command line in a debug build you should
be able to see it though.)

> Relatedly, do we really want the appearance check to be explicitly for menulist
> or HasAppearance instead? Chrome doesn't seem to grow the height if I say,
> -webkit-appearance: button, for example.

I considered that but chose not to, because Safari and Edge apply 'line-height'
for -webkit-appearance:button.  Also, we have the same logic already for
suppressing the triangular dropdown button so I think it makes sense to use
the same conditions here to make it simpler to understand.

Also, if you look closely at Chrome you'll see that they do in fact apply
the line-height, they just don't make the height reflect that for some reason:
data:text/html,<select style="-webkit-appearance:button"><option>A</select><select style="line-height:400px; -webkit-appearance:button"><option>A
Note the baseline alignment...  so this seems more like an edge case bug
in Blink to me.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> I'm not terribly happy about this patch, specially about the fact that the
> non-static CalcLineHeight() stops doing the same than the static version of
> it, I think that's somewhat footgunny...

Yeah, we should probably try to get rid of the static variant.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #8)
Hmm, alright, fair enough.

(In reply to Mats Palmgren (:mats) from comment #9)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> > I'm not terribly happy about this patch, specially about the fact that the
> > non-static CalcLineHeight() stops doing the same than the static version of
> > it, I think that's somewhat footgunny...
> 
> Yeah, we should probably try to get rid of the static variant.

Hmm, probably yeah... Maybe we can add a static CalcNormalLineHeight for now that just goes into GetNormalLineHeight?

That'd keep the two functions doing the same, and keep the off-band appearance check in nsComboboxControlFrame.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 9020230 [details] [diff] [review]
fix+test

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
> Maybe we can add a static CalcNormalLineHeight for now
> that just goes into GetNormalLineHeight?
> That'd keep the two functions doing the same, and keep the off-band
> appearance check in nsComboboxControlFrame.

I tried that but it broke the fix since it missed this call:
(rr) bt
#0  mozilla::ReflowInput::CalcLineHeight()
#1  mozilla::BlockReflowInput::BlockReflowInput
#2  nsBlockFrame::Reflow
#3  nsComboboxControlFrame::Reflow
...
(nsComboboxControlFrame is a subclass of nsBlockFrame)

====

It'd be nice if we could catch any calls on a nsComboboxControlFrame
to the static CalcLineHeight function too, but that is perhaps a
little riskier than warranted in this bug.  It looks like the static
version is only called with:
  CalcLineHeight(frame->GetContent(), frame->Style(), ...
so we could replace those two params with the frame instead:
https://searchfox.org/mozilla-central/search?q=CalcLineHeight
But that's probably better as a follow-up bug.  WDYT?
(at first glance, it looks like it never happens, except maybe for
the call in nsBlockFrame::GetCaretBaseline(), but hopefully that
doesn't matter much)

(BTW, there's a typo in the code comment in this patch: "!= 'menulist'"
should be "== 'menulist'".  I'll fix that before landing.)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: computed value test

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9020230 [details] [diff] [review]
fix+test

Review of attachment 9020230 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mats Palmgren (:mats) from comment #11)
> It'd be nice if we could catch any calls on a nsComboboxControlFrame
> to the static CalcLineHeight function too, but that is perhaps a
> little riskier than warranted in this bug.  It looks like the static
> version is only called with:
>   CalcLineHeight(frame->GetContent(), frame->Style(), ...
> so we could replace those two params with the frame instead:
> https://searchfox.org/mozilla-central/search?q=CalcLineHeight
> But that's probably better as a follow-up bug.  WDYT?

I don't think you can get rid of the static version of CalcLineHeight, since we use it for getComputedStyle()...

Looks like Blink and WebKit change this at computed value time, that is, they compute to normal, and thus the difference is visible in getComputedStyle().

Edge doesn't do this and shows the same resolved line height regardless of the theming of the combobox, even though they don't honor it.

Furthermore, looks like only we and WebKit resolve line-height: normal to pixels, Blink and Edge serialize to `normal`, which goes against the spec:

  https://drafts.csswg.org/cssom/#resolved-values

So will file issues on them.

I'm ok with this patch, since it does match what Edge does in this case at least.

Though maybe we should make this fixup in the style system instead, in StyleAdjuster? That way we avoid this and keep getComputedStyle properly returning the used value trivially, which will match WebKit (and Chrome modulo that CSSOM bug above).

I'd slightly prefer that, and can write that patch if you want, WDYT?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Or maybe just doing the check for HTMLSelectElement && select->IsCombobox() in the static CalcLineHeight function does the trick.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Which would be more consistent with how we handle <input>

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix+test (computed value version)

Normally I'd prefer tweaking the used value, but in this case it
might be better to tweak the computed value so that we can remove
the 'line-height' declarations for option/optgroup in the UA sheet
eventually...

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9020570 [details] [diff] [review]
fix+test (computed value version)

Review of attachment 9020570 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with those, thanks!

::: servo/components/style/style_adjuster.rs
@@ +695,5 @@
>              .mutate_position()
>              .set_computed_justify_items(parent_justify_items.computed);
>      }
>  
> +    ///

nit: remove the extra line here.

@@ +697,5 @@
>      }
>  
> +    ///
> +    /// If '-webkit-appearance' is 'menulist' on a <select> element then
> +    /// the computed value of 'line-height' is 'normal'.

Blink and WebKit do this regardless of whether you're a <select> element. Given the check is simpler I think I'd stick with that, but if you feel strongly about it I wouldn't mind much.

@@ +700,5 @@
> +    /// If '-webkit-appearance' is 'menulist' on a <select> element then
> +    /// the computed value of 'line-height' is 'normal'.
> +    ///
> +    /// https://github.com/w3c/csswg-drafts/issues/3257
> +    fn adjust_for_line_height<E>(&mut self, element: Option<E>)

These functions are named based on what they use to decide whether to change the style or not, not based on what style they change, so I'd rename it to `adjust_for_appearance`.

@@ +704,5 @@
> +    fn adjust_for_line_height<E>(&mut self, element: Option<E>)
> +    where
> +        E: TElement,
> +    {
> +        if self.style.get_box().clone__moz_appearance() == Appearance::Menulist &&

uber-nit: These functions usually use early-return, so if you don't mind much:

if self.style.get_box().clone__moz_appearance() != Appearance::Menulist {
    return;
}

if self.style.get... != LineHeight::normal() {
    return;
}

// ...

@@ +706,5 @@
> +        E: TElement,
> +    {
> +        if self.style.get_box().clone__moz_appearance() == Appearance::Menulist &&
> +            self.style.get_inherited_text().clone_line_height() != LineHeight::normal() &&
> +            element.map_or(false, |e| e.is_html_element() &&

If you keep the element check, you need to check for whether this is a pseudo-element or not. So this should become something like:

let is_html_select_element = element.map_or(false, |e| e.is_html_element() && e.local_name() == &*local_name!("select"));
if !is_html_select_element {
    return;
}

if self.style.pseudo.is_some() {
    return;
}

Comment 18

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)
> Blink and WebKit do this regardless of whether you're a <select> element.

That's unfortunate.  Maybe we can convince them to limit it
to <select>?  My impression is that people want to limit
-webkit-appearance in general to affect as few elements as
possible, so it'd suck to have random stuff like this lying
around.  The appearance/form controls spec is far from being
useable at this point though, so we'll see...

> uber-nit: These functions usually use early-return, so if you don't mind
> much:
> 
> if self.style.get_box().clone__moz_appearance() != Appearance::Menulist {
>     return;
> }

That's inconvenient if we want to add a tweak for some other
appearance value later though (which seems plausible).  Then we'd
have to rewrite this code, rather than just append the new code.
I think I prefer to keep it as is, but I'll change the element/pseudo
checks inside it to early returns.

> If you keep the element check, you need to check for whether this is a
> pseudo-element or not.

Thanks, I didn't know that.
(Hmm, it seems a bit wasteful to resolve the style for <select>
::before/::after given that we never actually create them.)

> let is_html_select_element = element.map_or(false, |e| e.is_html_element() && e.local_name() == &*local_name!("select"));

It'd be nice if one could write element.is_html("select") instead.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89697dbd654a
Force line-height:normal for themed comboboxes for compat with other UAs.  r=emilio

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13753 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #18)
> Thanks, I didn't know that.
> (Hmm, it seems a bit wasteful to resolve the style for <select>
> ::before/::after given that we never actually create them.)

Yeah, if we know the element generates a leaf box we could avoid computing the pseudo styles for it. But note that this path is also taken for inheriting anonymous boxes, and select does have one at least (::-moz-display-comboboxcontrol-frame). We probably are not messing with appearance in those rules, but it's a bit footgunny not to check that.

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/89697dbd654a

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 9020570 [details] [diff] [review]
fix+test (computed value version)

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1499578

User impact if declined: broken <select> layout on some web sites that set an unreasonable 'line-height' on it

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020570 [details] [diff] [review]
fix+test (computed value version)

[Triage Comment]
Fixes broken <select> layout on some web sites. Approved for 64.0b6.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fcb8fd2dd657

Comment 30

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 65.0a1 (2018-11-01) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.

Comment 31

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Firefox 64.0b6 on Windows 10 x 64, Windows 7 x32 and on Mac OS X 10.13.

On Ubuntu 16.04 x64 the <select> label is still not aligned correctly, somehow I missed this when I verified the bug on Nightly, but today when I tried to verify this bug on Firefox 64.0b6 I observed the issue. Please see https://imgur.com/a/RdLp8yR

I'll log bug 1504183 for this.