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

VERIFIED FIXED in Firefox 64

Status

()

defect
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: bgstandaert, Assigned: mats)

Tracking

({parity-chrome, parity-safari, regression})

64 Branch
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
webcompat ?
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 verified, firefox65 verified)

Details

(Whiteboard: [webcompat])

Attachments

(6 attachments)

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
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?
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
Flags: needinfo?(mats)
Product: Firefox → Core
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.
Assignee: nobody → mats
Flags: needinfo?(mats)
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.
Posted file 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.
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...
Posted patch fix+testSplinter Review
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'...
Attachment #9020230 - Flags: review?(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.
Attachment #9020230 - Flags: review?(emilio)
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.
(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.
(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 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.)
Attachment #9020230 - Flags: review?(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?
Attachment #9020230 - Flags: review?(emilio) → review+
Or maybe just doing the check for HTMLSelectElement && select->IsCombobox() in the static CalcLineHeight function does the trick.
Which would be more consistent with how we handle <input>
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...
Attachment #9020570 - Flags: review?(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;
}
Attachment #9020570 - Flags: review?(emilio) → review+
(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.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89697dbd654a
Force line-height:normal for themed comboboxes for compat with other UAs.  r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13753 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
(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.
https://hg.mozilla.org/mozilla-central/rev/89697dbd654a
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Upstream PR merged
Duplicate of this bug: 1502233
Blocks: 1499578
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:
Attachment #9020570 - Flags: approval-mozilla-beta?
Flags: webcompat?
Whiteboard: [webcompat]
Duplicate of this bug: 1501670
Flags: in-testsuite+
Flags: qe-verify+
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.
Attachment #9020570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.