1560824 - select elements are 2px taller than buttons and textfields, due to internal padding that's documented as making them the same size

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Load attached testcase.

EXPECTED RESULTS:
The three form controls should render with the same height, and the alert popup should show the same offsetHeight for them.

ACTUAL RESULTS:
The first one (the select) is taller than the other two. Specifically, 2px taller, which seems to come from this bonus internal padding:
https://searchfox.org/mozilla-central/rev/0b7007a23bc16c857f894140e12f307bfeef2fdd/layout/style/res/forms.css#324,328-329

A web developer reported this on twitter after it caused a problem for him:
https://twitter.com/adamwathan/status/1142392166892888064

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (elements all taller via line-height:10 in font shorthand)

Comment 3

[Daniel Holbert [:dholbert]]

So in "old builds" (from ~2011 through 2016), the select and the button are the same height, and the text-input element is a bit smaller (on its own). That kinda made sense, because select elements render like a button.

But in December 2016, the button got a little smaller (becoming the same height as the text-input element) leaving <select> all on its own. The commit range for that change is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f65ad27efe839ce9df0283840a1a40b4bbc9ead0&tochange=557548714db55136b51e1129d649e2599797985f

And I think the relevant change there was:

https://hg.mozilla.org/mozilla-central/rev/f86ce2e2f6a51f351974e3781158bc0586ab71ed
Thomas Wisniewski — Bug 140562 - Part 2: Remove the 2px extra padding on buttons for a prospective -moz-focus-inner ring, and just size that ring the same as the content frame (inflated by its CSS padding). r=dbaron

I think that "2px extra padding" that we removed there is what we're trying to mimic with the select forms.css declarations that I linked in comment 0. We should probably just remove that from forms.css, now that the corresponding button padding is gone.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (default styling)

Here's a testcase with default-styled select, button, and input.

In this case, the input is a good bit taller on my linux system (and doesn't render "button-flavored" anyway), so its height isn't really something we should aim to match, for this bug's purposes. But again, the select and the button (both "button-flavored") are nearly the same size, but are just 2px apart, due to this 2px of unnecessary internal padding.

So: even with default-styled form controls, this fix will be a step towards consistency.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1560824: Remove 2px of extra padding inside of comboboxes (<select> elements). r?jfkthame

Comment 6

[Daniel Holbert [:dholbert]]

Let's see if this breaks any tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff45548bbbeacf6acd64bc02e26882bb18d38ede

Comment 7

[Daniel Holbert [:dholbert]]

The Try run looks good -- only one failure that looks related, and that was just due to hand-tuning of values in the reference case. I've updated the patch to fix that test (select/vertical-centering.html) and to address a hand-wavy question that was embedded within it.

Comment 8

[Daniel Holbert [:dholbert]]

Here's another Try run with the updated patch (fixing the test that failed in the previous try run, and adding a dedicated test for this bug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=353358e99e21b2700ca3580fd69e0002ac1cca3a

Note: the test I added isn't a WPT, because e.g. Chrome doesn't pass it. I don't think there are any/many cross-browser guarantees about how select elements & buttons are supposed to render in general. (Chrome does serve as a "reference" for the testcases that I attached to this bug report, but my patch's new reftest includes some scenarios where Chrome sizes buttons vs. selects differently, for Unknown Reasons.)

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: screenshot comparing default rendering of <select> on macOS: Safari / Chrome / Nightly / Nightly+patch

Comment 10

[Daniel Holbert [:dholbert]]

As noted in phabricator, this needs a bit more work before it's ready...

In particular, the patch as it stands would change native-themed default-sized <select> elements on Mac for the worse -- they'd end up being 1px shorter than an equivalent <button>, which is inconsistent & can make glyphs with descenders jut up against the widget border as shown at far right end of jfkthame's screenshot.

This is because right now, native-themed <select> elements are only 1px taller than <button>, in Nightly on my mac mini -- e.g. in my attached testcase 3. For unthemed elements (in testcase 1 and 2), it's a 2px difference as described in comment 0. But for native-themed buttons it's only 1px for some reason (on mac).

Comment 11

[Daniel Holbert [:dholbert]]

For comparison: Safari 12.1.1 on my mac mini shows equal heights for all three elements (select/button/input) in testcases 1 and 2.

And for testcase 3 (where Safari kind of gets to define what "native theming" should look like), Safari has select and button having equal heights, with input only being 1px taller. Here's the comparison:

                 Sel  Btn  Inp
Safari           18   18   19
Nightly          19   18   22
Patched Nightly  17   18   22

Bottom line, it looks like it's still legitimate to aim for buttons and select elements having the same height under native theming (not to mention the same height as textfield-inputs as well, when unthemed) -- and we should figure out what's responsible for the 1px sizing difference.

Comment 12

[Jonathan Kew [:jfkthame]]

FWIW, while <button> and <select> look like they have the same height in Safari on macOS (and in Chrome), they don't actually align: the <select> is 1 device pixel lower than the button when I look at a testcase:

data:text/html,<button>foo bar</button><select><option>foo bar

(while in Firefox, the <select> is visibly taller, as noted here).

If we're going to try and harmonize their heights, I think we should also ensure their baselines match.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Oh, I wrote a patch for this in bug 1571764, somewhat different approach though.

Comment 14

[Daniel Holbert [:dholbert]]

Looks like the original testcases here (from the Twitter thread) are now working as-expected -- everything is the same height, at least on my system in Nightly (whereas the select is slightly taller in Firefox 68 release version).
Reporter's original testcase: https://jsfiddle.net/s9fj2qrb/3/
My reduced version: https://jsfiddle.net/kpnjhv01/2/

Also, in testcase 2 here (attachment 9073474 [details]), all of the elements are now the same height in Nightly (vs. the select is taller in Firefox 68).

However, testcase 1 (like testcase 2 but with line-height unspecified) and testcase 3 (where elements have default/native styling) have not changed -- they still show the select being 2px taller than a similar-looking button, for no clear reason.

So this is somewhat better, but not entirely fixed.

Comment 15

[Daniel Holbert [:dholbert]]

--> Unassigning to reflect reality, since I'm not actively working on this & don't foresee having time to pick it up again soon.

(Per comment 14, there's still arguably something to be fixed here for testcase 1 and testcase 3, so this isn't fixed, but it's also not high priority. If anyone else picks this up, https://phabricator.services.mozilla.com/D35628 has my first attempt at a fix & some automated testcases that may be useful -- but as jfkthame pointed out in phabricator, that patch breaks some Mac widget stuff, which is why it didn't land.)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1560824 - Move combobox select padding to the select rule rather than the combobox frame. r=dholbert,#layout-reviewers

This doesn't change behavior by default but allows authors to remove the
padding if they wish to.

I thought this was going to be problematic because of the windows
arrowbutton, but that doesn't respect select padding so we're good.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

I think my patch shouldn't cause that issue.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2539c891f28
Move combobox select padding to the select rule rather than the combobox frame. r=dholbert

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/f2539c891f28