Closed Bug 1562057 Opened 5 years ago Closed 5 years ago

<select> elements with size containment & auto width should render the same as an empty <select> element

Categories

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

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 2 obsolete files)

testcase 1
803 bytes, text/html
Details
Bug 1562057: Change size-contained & empty select elements to have the same inline-size. r?TYLin
47 bytes, text/x-phabricator-request
Details | Review

As discussed in bug 1476127 comment 6, empty <select> elements actually end up getting a little bit of width from an automatically-inserted &nbsp; character in their ComboboxDisplay frame.

For correctness, we need to match that width for size-contained <select> elements, in order to pass this WPT test:
https://wpt.fyi/results/css/css-contain/contain-size-select-001.html

(Right now, my implementation in bug 1476127 simply makes size-contained elements behave as if their ComboboxDisplay frame were empty, which isn't quite right -- it produces a rendering that looks something like [v] instead of [ v].

I don't think this bug needs to block the shipping of css containment, BTW.

The &nbsp; gets inserted here, BTW:
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/layout/forms/nsComboboxControlFrame.cpp#981-987

void nsComboboxControlFrame::ActuallyDisplayText(bool aNotify) {
  RefPtr<nsTextNode> displayContent = mDisplayContent;
  if (mDisplayedOptionTextOrPreview.IsEmpty()) {
    // Have to use a non-breaking space for line-block-size calculations
    // to be right
    static const char16_t space = 0xA0;
    displayContent->SetText(&space, 1, aNotify);

Based on the comment there, it looks like we're only inserting it to influence the BSize (height) -- we're not explicitly intending for it to influence the ISize (width), though it is influencing it here. I wonder if we actually need to care about its ISize at all? If not, then maybe we can avoid this issue by using another character that's guaranteed to have no ISize (if such a character exists)...

If we're happy having the &nbsp's ISize influencing things, though, we could probably do some sort of hacky dance to produce the correct results in a size-contained select element.

I am compiling an implementation report for css-contain, and this is the last thing standing between us and REC for css-contain level 1 (there are other failures, but they're not blocking: they either test at risk features, or cross test features of specs with a lower maturity).

This is a fairly minor bug, but for the sake of pushing the spec over the finish line, it'd be great if you could fix this soon.

Is there any reason we can't use a zero-width joiner character?

Maybe Zero Width Space U+200B rather?

There's more to this than just what dholbert said in https://bugzilla.mozilla.org/show_bug.cgi?id=1562057#c1. If you swap 0xA0 for some larger character (e.g. 0x3000), the size of an empty <select> does get appropriately larger, but if you set it to something smaller (e.g. 0x200B), it doesn't shrink further. Something else seems to be enforcing a minimum size.

Aha. The culprit is in forms.css

*|*::-moz-display-comboboxcontrol-frame {
...
  padding-inline-start: 4px;
  padding-inline-end: 4px;
...
}

I wonder if we should add these back even when size containment is on, or if we should drop them from the empty <select>.

  • I doubt it's terribly important to continue showing a little bit of empty space next to the dropdown button in an empty <select>, especially given that other browsers don't.
  • This is probably not overly performance sensitive, but even then, skipping inline sizing calculations in the case of an empty <select> is a tiny win, while adding back padding calculations in the case of a size contained <select> is a tiny loss.
  • padding added in author stylesheets are a separate thing, and are not ignored

=> My suggestion would be to skip inline sizing when empty just the same way it is skipped when size containment is on

I haven't yet figured out how to make contributions to gecko using the proper process, so here's a raw patch for now. It makes the wpt contain-size-select-001 and contain-size-select-002 pass using the logic described in my previous comment.

I'll probably try to figure out how to submit a patch normally sometimes soon, but in case someone wants to have a look before I get there, here's what I have.

Thanks, Florian! At first glance, this seems OK to me.

This doc has the proper way to submit a patch, if you want to take a look. In the meantime, I've pushed your patch to our "Try" repository to see if it happens to break anything in our test suite:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e85c00e1992f5a273593dd3bb24f95a9752f0a0

(It's normal for a few known-intermittent unrelated test tasks to have a failure, so don't necessarily worry if there ends up being some orange. I'll take a look at the results later today and see if there are any failures that look related/concerning.)

One small nit: you'll need to remove the failure annotation for our "upstream" layout/reftests copy of the now-passing testcase, too.

Specifically, you'll want to remove the fails-if(!gtkWidget) annotation from this line:
https://searchfox.org/mozilla-central/rev/05db7b82d5a4adaa8b36ec6f4fd1715f6fd7885b/layout/reftests/w3c-css/submitted/contain/reftest.list#39

That'll address the TEST-UNEXPECTED-PASS orange on https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e85c00e1992f5a273593dd3bb24f95a9752f0a0&selectedJob=265397898

Flags: needinfo?(florian)

(er sorry, by "upstream" I should've said "similar". :) It's a different testcase.)

Anyway - that reftest.list tweak is needed, and you'll also want to delete this failure annotation file:
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-size-select-elem-002.html.ini

Assignee: nobody → florian
Flags: needinfo?(florian)
Status: NEW → ASSIGNED

Per the css-contain specification, size contained element must be sized as if
they were empty. The code added to handle size containment shortciruits the
(inline) size calculations, and returns 0. However, an empty <select> element
is rendered as if it contained a   and some padding gets added to it by
the UA stylesheet (forms.css). This causes reftest that check that
size-contained <select> elements and empty ones look the same.

This commit fixes this by also shortcircuiting the (inline) size calculations
and returning 0 for empty <select> elements.

Replacing the   by a zero width space would not have been enough, since
padding would still be added. It would have been possible to add it in the
inline size calculations of size-contained <select> elements as well, but this
padding serves not purpose when the element is empty, so removing it from there
has no downside, and shortcircuitig both cases is simpler (and marginally
faster) than adding the padding in both cases.

Test failures all look unrelated/known-intermittent (most of the orange is in M-fis which I think is normally hidden/disabled due to perma-failures still being flushed out in that configuration -- I need to tune my try queries to avoid getting those testruns).

I'll go ahead and r+ and land this -- thanks again Florian!

Attachment #9091330 - Attachment description: Bug 1562057 - Make size contained <select> elements and empty ones the same size. r=dholbert → Bug 1562057 - Make empty <select> elements the same size as size-contained ones. r=dholbert
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0579d0d5f0c0
Make empty <select> elements the same size as size-contained ones. r=dholbert

I made a slight tweak to the commit message before landing, to make it clearer that empty <select> elements are the thing whose behavior is being changed here. (and the only thing whose behavior is changing)

Thank you.

Is there anything left to do here, or are the remaining steps (both on the code side, to build and include this in a nightly, then in a beta, then in a release; and on the bug side, to mark it as fixed, to close it, etc) automated?

Everything's automated from here on out!

This has landed on our main "autoland" integration branch (comment 15), and as long as it's not backed out*, it'll be merged to mozilla-central (along with another batch of recent autoland changes) later today, and the bug will be closed at that point. (It stays open until then, since mozilla-central is the source of truth and is what nightly is built from.) And then it'll be included in the next nightly at that point. (Likely tomorrow's nightly, which is built in early morning (~1-4am, pacific time.)

*backout is unlikely at this point -- usually any issues that'd cause a backout would be caught within an hour or two.

https://hg.mozilla.org/mozilla-central/rev/0579d0d5f0c0

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Unfortunately this caused a regression. Reduced testcase:

data:text/html,<select><option></option><option>AAAA</option><option>BBBBBBBBBBBBBBBBBBBBBB</option></select>

That ^^ URL should render with a wide, blank select element. But in builds with this bug's patch, it renders with a skinny select element instead (until you choose a nonempty option, at which point it gets wide).

Before this bug's patch landed, we rendered this select element at a consistent wide side (wide enough for the widest option). We don't want to break that behavior.

In some cases we'd leave the patch in and track this issue in a followup, but the cost/benefit here leans in favor of just backing out and fixing this issue up-front so that we don't ship with this regression and potentially break site layouts for sites with empty initial-options for select elements.

Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/121042ab9849
Backed out changeset 0579d0d5f0c0 by dev's request. CLOSED TREE
Status: RESOLVED → REOPENED
status-firefox71: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla71 → ---

Backout is supposed to be merged to central in the next hour.

I will try to have a look at this (although I am currently attending TPAC, so kind of busy), but depending on how much more complex the good fix needs to be, it might end up being beyond my current experience with Gecko. In other words: I'll try, but if someone else wants to give it a shot, that's totally fine by me as well.

Flags: needinfo?(florian)

Daniel, I have updated the patch to detect empty <select> elements in a different way, which should not get tripped up like the previous one. New review appreciated.

I've run hg commit --amend and moz-phab, which did update https://phabricator.services.mozilla.com/D45144, but didn't put any new notification here. Let me know if that wasn't the right way to do this.

Thanks, Florian! (You did indeed do the right thing, process-wise, to update the patch. Phabricator rarely updates bugzilla, as you observed - only when patches appear/disappear/land.)

I think the patch still may not be quite ready, though. It breaks the invariant that these two testcases should render the same:

data:text/html,<select style="border: 1px solid black">
data:text/html,<select style="border: 1px solid black"><option>

(i.e. completely-empty-select vs. select-with-just-an-empty option -- with the new version of the patch, the former testcase is skinnier than the latter testcase)

So this is not quite ready to land. I don't know offhand what the best solution is, but I can try to take a look around in the next few days, if you don't come up with a solution before then. :)

Flags: needinfo?(dholbert)

Is it important that a an empty select is the same size as a select with any number of empty options (and no non empty one), or is a select with a single empty option the only thing we need to match?

I'd say that any number of empty options (zero or one or many) should all produce the same size.

At least, I'm pretty sure that's something that browsers agree on right now, and there's no clear reason to deviate from that.

Flags: needinfo?(dholbert)

Probably that's because I'm not overly familiar with Gecko's code, but I'm a little struggling with the two different ways this could go:

  • iterate through all the options in the select to see if all the non disabled ones are empty, set isSizedAsEmpty to true if so, and or false if any isn't empty. That seems a little expensive. Also, the objects at hand (mDropdownFrame) don't seem to have the built-in iteration primitives, so I'm not 100% sure how to navigate around that.
  • set displayISize to the padding rather than to 0 for size-contained select elements: I'm not sure where to grab the padding from from:, as far as I can tell that can be found on the ReflowInput object, but the code where the check is currently happening does not have access to a ReflowInput object , so I'm not sure how to get to it.

Advice welcome.

Sorry, I'm not familiar enough with the internals of our <select> widget implementation to have a useful suggestion right away.

I think I need to step through some scenarios in a debugger and/or spend some time staring at the code before I'll have an idea of what makes sense here... I was hoping to get to that this week but didn't manage to yet. Hopefully I can offer some some advice next week. :)

Flags: needinfo?(dholbert)
Attached file testcase 1 —
See Also: → 1587614

So it's actually a bit odd that we honor padding on option elements at all (which we do, as you noted). I filed bug 1587614 on this.

Here's one thing that would work here (not sure if it's the best solution):
(1) change the "placeholder" character here to be ZERO WIDTH SPACE (U+200B) or ZERO WIDTH NO-BREAK SPACE (U+FEFF)
(2) Change the CSS rule that adds default padding for select > option to instead be select[multiple] > option in forms.css, and remove the inline-axis padding on the *|*::-moz-display-comboboxcontrol-frame pseudoclass in forms.css

With that, our intrinsic sizing function's call to mDropdownFrame->GetPrefISize(...) should legitimately return 0 for empty select elements (and select elements that only contain empty options), and there'll be no need for the currently-posted patch's "isSizedAsEmpty" bool.

Also, if we do the above things, we'd probably want to...
(3) Possibly add some "secret" padding to the select element itself (somewhere), to counterbalance for the padding removals so that we've got some breathing room between characters and text for nonempty select elements.

ALTERNATELY, instead of (2)/(3) above (and perhaps better): we could adjust the "is-size-contained" codepath to be a little more subtle, and actually pick up this padding for the ::-moz-display-comboboxcontrol-frame (the nsComboboxDisplayFrame) helper.

This would probably involving changing the behavior of nsLayoutUtils::IntrinsicForAxis(), possibly by just adding a special-case for nsComboboxDisplayFrame there. The key piece that we need is the AddIntrinsicSizeOffset() call towards the end of that function -- that's what adds the resolved padding. But we want to skip anything that adds/computes the content size.

ALTERNATELY, given that we control the padding on ::-moz-display-comboboxcontrol-frame and we happen to know that it's a pixel value, maybe we don't need to bother getting that far for size-contained content. Maybe we can just change nsComboboxControlFrame::GetIntrinsicISize to (if we're size-contained) look up the inline-axis padding on mDropdownFrame, and add use that instead of calling mDropdownFrame->GetMinISize() etc.

I'm experimenting with the final "alternately" right now, to see if I can come up with something not-too-terrible...

Per the css-contain specification, size contained element must be sized as if
they were empty. Up until now, we've been handling that by just using "0" as
the intrinsic size of some components, but that doesn't actually match the size
of a "true" empty select, which has some width from:
(a) the default inline-axis padding on the display frame (added in a rule for
the ::-moz-display-comboboxcontrol-frame pseudo, in forms.css).

(b) the width (inline-size) of the display frame's "placeholder" space
character, which has a small intrinsic width (but which really only exists
for block-axis sizing and alignment, when no option is selected from
the dropdown).

This patch addresses issue (a) by explicitly adding the display frame's
inline-axis padding to size-contained elements, and it addresses issue (b) by
changing to a zero-width space character.

(I chose U+FEFF "zero-width non-breaking-space" since we were previously using
a non-breaking space character. I'm not sure if the non-breaking aspect matters,
but I figured I'd preserve that to be on the safe side.)

Attachment #9100056 - Attachment description: Bug 1562057: Make size-contained select elements use the display frame's padding, and change empty placeholder content to a zero-width space character. r?TYLin → Bug 1562057: Change size-contained & empty select elements to have the same inline-size. r?TYLin

I verified that the my patch's new WPT test fails (as-expected) in a build of mozilla-central with the original commit applied (the patch that landed in comment 19). The test passes in current mozilla-central, and in a build with my new patch. So, the test functioning correctly as a regression-test for that issue.

Flags: needinfo?(dholbert)

[Florian, I hope you don't mind me stealing this bug from you. In investigating what the right fix would be, it ended up being simplest just to write the fix as I investigated. :) ]

Assignee: florian → dholbert
Attachment #9090947 - Attachment is obsolete: true
Attachment #9091330 - Attachment is obsolete: true

Nice, thanks. That's one of the approach I was attempting to follow, but I couldn't figure out that "mDisplayFrame->IntrinsicISizeOffsets().hPadding" was the way to get that padding from this context.

Is there any documentation I could read to try and get familiar with how layout in Gecko works and how various bits of information are stored / passed around / calculated, assuming pretty good familiarity with the CSS way of doing things, and with C++, but not (yet) with the Mozilla code base?

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8ea98346a87
Change size-contained & empty select elements to have the same inline-size. r=TYLin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19643 for changes under testing/web-platform/tests

(In reply to Florian Rivoal from comment #37)

Nice, thanks. That's one of the approach I was attempting to follow, but I couldn't figure out that "mDisplayFrame->IntrinsicISizeOffsets().hPadding" was the way to get that padding from this context.

Yeah - your instincts were right in comment 29 that we should use the ReflowInput object, but this particular codepath is actually invoked when we're setting up the ReflowInput object, so we don't have resolved padding yet & we need to resolve it ourselves.

(I didn't initially know that this API was the right way to get the padding, either. Also, normally it'd be a bit more subtle, because we'd also need to pass in some sort of basis for percent resolution in case there were a percentage padding value, and there's some extra complexity/logic to determine the correct percent-basis to pass in. But fortunately in this case we can be sure that the pseudo-element in question is only styleable by our internal UA stylesheets, so we can just assume that we don't have to worry about percent handling, since our internal stylesheets use fixed px values for this padding and aren't likely to change. :) )

Is there any documentation I could read to try and get familiar with how layout in Gecko works and how various bits of information are stored / passed around / calculated, assuming pretty good familiarity with the CSS way of doing things, and with C++, but not (yet) with the Mozilla code base?

The best high-level overview is the "layout" section of this doc:
https://wiki.mozilla.org/Gecko:Overview#Layout

In this case, I ended up debugging this by "following the money" in GDB, to track where the nonzero intrinsic size was coming from, and it ended up landing me on a call to mDisplayFrame->IntrinsicISizeOffsets() that was being passed in to AddIntrinsicSizeOffset() and whose .hPadding component was producing a nonzero added size there. That, plus the knowledge that I didn't have to worry about providing a percent basis, made me decide on using this particular incantation to give me the same padding.

Attachment #9091330 - Attachment is obsolete: false
Attachment #9091330 - Attachment is obsolete: true
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging

That's one of the tests that was previously failing and was expected to start passing (and annotated as such) as part of this bug's patch.

The failure is Android-specific (not entirely surprising, given that our Android widget implementation has some quirks as I recall). I'm going to punt that failure to a followup, and annotate the Android-specific failure, and re-land here.

Flags: needinfo?(dholbert)
Blocks: 1588212
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25d95be82e95
Change size-contained & empty select elements to have the same inline-size. r=TYLin
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

https://hg.mozilla.org/mozilla-central/rev/25d95be82e95

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: