We're currently use "GetBSizeOfARow()" which is the maximum block-size of the <option>s and <optgroup> labels *using the style of those elements*. This wrong, first because we shouldn't include <optgroup> labels at all since they are never displayed in the combobox itself, second because the displayed option value is using the style of the <select> (or more precisely, the internal ::-moz-display-comboboxcontrol-frame pseudo), not the style of the <option> element.

Also, remove the !important on 'line-height' on <select> since other UAs allows authors to change it. (I don't really see a reason to have it on <option>/<optgroup> either, but let's deal with that in a follow-up bug if needed.)

Comment on attachment 9017913 [details] [diff] [review] Make one line-height the intrinsic block-size for <select> comboboxes Review of attachment 9017913 [details] [diff] [review]: ----------------------------------------------------------------- So the reason <select>'s line-height wasn't modifiable was because according to https://bugzilla.mozilla.org/show_bug.cgi?id=349259#c117 no other browser supported it at the time... Oh well. Looks great, thanks! ::: layout/forms/nsComboboxControlFrame.cpp @@ +1346,5 @@ > MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!"); > > ReflowInput state(aReflowInput); > if (state.ComputedBSize() == NS_INTRINSICSIZE) { > + float inflation = nsLayoutUtils::FontSizeInflationFor(this); Do you want to pass mCombobox here instead of this? I'm not that familiar with the font-inflation code so might need a sanity check. I think it mostly uses font-size so it might not really matter but it does poke at StylePosition() a bit as well... ::: layout/style/res/forms.css @@ +224,5 @@ > border-color: ThreeDLightShadow; > background-color: -moz-Combobox; > color: -moz-ComboboxText; > font: -moz-list; > + line-height: initial; Given all the other inputs use `normal`, I'd rather leave it as normal, or change it everywhere. ::: testing/web-platform/tests/html/rendering/replaced-elements/the-select-element/select-1-block-size.html @@ +14,5 @@ > +html,body { > + color:black; background-color:white; font:16px/1 monospace; padding:0; margin:0; > +} > + > +.none select { -webkit-appearance: none; } Given all the selects are under <div class="none">, what about just doing: select { -webkit-appearance: none } ?

Comment on attachment 9017913 [details] [diff] [review] Make one line-height the intrinsic block-size for <select> comboboxes Review of attachment 9017913 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsComboboxControlFrame.cpp @@ +1351,5 @@ > + // We intentionally use the combobox frame's style here, which has > + // the 'line-height' specified by the author, if any. > + // (This frame has 'line-height: -moz-block-height' in the UA > + // sheet which is suitable when there's a specified block-size.) > + auto lh = aReflowInput.CalcLineHeight(mComboBox->GetContent(), Oh, one last nit, given this version of the function is static, I think it'd be clear to replace aReflowInput.CalcLineHeight by ReflowInput::CalcLineHeight. wdyt?

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/50a3c9a567e3 Make one line-height the intrinsic block-size for <select> comboboxes. r=emilio

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2) > > + float inflation = nsLayoutUtils::FontSizeInflationFor(this); > > Do you want to pass mCombobox here instead of this? I don't think it makes a difference, but sure :-) > > + line-height: initial; > > Given all the other inputs use `normal`, I'd rather leave it as normal, or > change it everywhere. I'll file a follow-up bug to use 'initial' for all inherited properties instead of using their initial value literally. It makes the intention a little clearer. > Given all the selects are under <div class="none">, what about just doing: > > select { -webkit-appearance: none } OK

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3) > Oh, one last nit, given this version of the function is static, I think it'd > be clear to replace aReflowInput.CalcLineHeight by > ReflowInput::CalcLineHeight. wdyt? OK, did so.

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

Upstream PR merged