Closed Bug 1591925 Opened 3 months ago Closed 2 months ago

select element incorrectly allows itself to shrink to a main-size of 0, in a flex container

Categories

(Core :: Layout: Flexbox, defect, P3)

71 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: niko.elmari, Assigned: alaskanemily)

References

Details

Attachments

(6 files)

Attached image Screenshot

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Put select element inside flexbox and apply some "pressure":
https://jsfiddle.net/Risord/dvky8w4m/4/

Actual results:

Select will be collapsed all the way zero height (in demo there is a bit more space so that select is easier to see).

Expected results:

Select should keep it's "obvious" height just like the div (and other form inputs etc.). Chrome and Edge also handle select correct way.

Component: Untriaged → Layout: Flexbox
Product: Firefox → Core

This seems to be a regression.

Status: UNCONFIRMED → NEW
Ever confirmed: true

Ah, so the regression range above is probably a bit of a fluke caused by the Linux-only scaling changes in bug 1431337.

If I use this test-case, I can see that it has pretty much always been broken.

Daniel, do you know what's supposed to be the right rendering here?

Flags: needinfo?(dholbert)
Keywords: regression
Priority: -- → P3

This is definitely a bug. We should be resolving a nonzero used value for min-height here, based on the content, and refusing to shrink the select-widget below that height.

Here's a more extreme example, with height:0 on the flex container, to shrink as much as possible (which for the affected select elements seems to be "all the way").

I would bet that the "themed" widgets are only being saved by this codepath (which imposes an additional sizing lower-bound that we really should only have to be falling back on in testcases with explicitly-specified lower min-width values):
https://searchfox.org/mozilla-central/rev/35873cfc312a6285f54aa5e4ec2d4ab911157522/layout/generic/nsFlexContainerFrame.cpp#1330-1334

Flags: needinfo?(dholbert)

Aha, so the issue is here:

  // NOTE: Technically we should be checking the 'overflow' subproperty in the
  // main axis. But since we only care whether it's 'visible', we can check
  // either subproperty -- because they must be BOTH 'visible' or BOTH
  // non-'visible' due to the way the subproperties interact.
  mNeedsMinSizeAutoResolution =
      IsAutoOrEnumOnBSize(mainMinSize, IsInlineAxisMainAxis()) &&
      disp->mOverflowX == StyleOverflow::Visible;
}

Searchfox link: https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/layout/generic/nsFlexContainerFrame.cpp#2101-2107

The "overflowX" check there is implementing the first sentence of this spec-text (which used to be worded in terms of the "overflow" property, but now is worded in terms of scroll containers) -- emphasis added by me:

To provide a more reasonable default minimum size for flex items, the used value of a main axis automatic minimum size on a flex item that is not a scroll container is a content-based minimum size; for scroll containers the automatic minimum size is zero, as usual.
https://drafts.csswg.org/css-flexbox-1/#min-size-auto

Select elements have overflow: -moz-hidden-unscrollable under the hood:
https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/layout/style/res/forms.css#231,245
...so they fail the overflow check there. But they're not scroll containers, so they shouldn't be bailing out of this check.

We should add && disp->mOverflowX == StyleOverflow::MozHiddenUnscrollable here, to exclude that as well. (I think we can continue abusing mOverflowX here without caring about the actual axis, because if we had a scrollport-producing 'overflow' value in either axis, then I think it would force the other axis to 'auto' (away from visible/MozHiddenUnscrollable). That would be worth verifying, though.

I'm going to reserve this bug as a "good first/second bug" for Emily who joined our team today.

Summary: Select content height is zero → Select content height is zero in a flex container
Assignee: nobody → emcdonough
Status: NEW → ASSIGNED

(In reply to Daniel Holbert [:dholbert] from comment #7)

(I think we can continue abusing mOverflowX here without caring about the actual axis, because if we had a scrollport-producing 'overflow' value in either axis, then I think it would force the other axis to 'auto' (away from visible/MozHiddenUnscrollable).

I verified this locally in gdb -- though the unspecified axis gets promoted to "hidden", not "auto". (This is still fine for our purposes.)

(I verified by printing disp->mOverflowX and disp->mOverflowY in the code snippet linked in my previous comment. I tested a variety of combinations, specifying different values or no value for overflow-x and overflow-y. In every scenario that produced an overflow-y that was non-visible/-moz-hidden-unscrollable, our overflow-x was also non-visible/-moz-hidden-unscrollable.)

Summary: Select content height is zero in a flex container → Select content height inco in a flex container
Summary: Select content height inco in a flex container → select element incorrectly allows itself to shrink to a main-size of 0, in a flex container
Attachment #9107590 - Attachment description: Bug 1591925 - Fix zero-height select elements in flex frames → Bug 1591925 - When resolving min-size:auto on the main axis of flex items, treat `overflow:-moz-hidden-unscrollable` the same as `overflow:visible`.
Attachment #9107591 - Attachment description: Bug 1591925 - Add reftests for zero-height select elements with flex layout → Bug 1591925 - Add reftests for select elements zero-height flex containers
Attachment #9107591 - Attachment description: Bug 1591925 - Add reftests for select elements zero-height flex containers → Bug 1591925 - Add reftests for select elements in zero-height flex containers

Looks like nits are addressed. I'm happy to trigger Lando here to get this landed, whenever you think it's ready (if you're confident that it passes tests on Try).

Feel free to needinfo me here or ping me on slack when you're ready to land (or add the checkin-needed tag on phabricator and a sheriff will get to it within a day or so).

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d0f795c10a4
When resolving min-size:auto on the main axis of flex items, treat `overflow:-moz-hidden-unscrollable` the same as `overflow:visible`. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/eb9d579fe07c
Add reftests for select elements in zero-height flex containers r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20203 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging

I see what's happening here. Those are actual failures, I will fix my patch.

Flags: needinfo?(emcdonough)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbeb8f308193
When resolving min-size:auto on the main axis of flex items, treat `overflow:-moz-hidden-unscrollable` the same as `overflow:visible`. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b2d168294899
Add reftests for select elements in zero-height flex containers r=dholbert
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging

It looks like the only issue was that the -001 testcase unexpectedly passed on:
-"Linux x64 debug"

  • Windows 10 x64 (which didn't get a full test run on your push, but did get a "TVw" i.e. test-verify web-platform-test run for the newly-added test, which showed it unexpectedly passing)

And also, Android debug had a whole bunch of unrelated-looking unexpected passes. Those seem likely-unrelated to me, so I probably wouldn't attribute those to this patch, but we should sanity-check that on try before relanding.

So: RE the linux and windows issue -- it seems bug 1596255 doesn't affect all platforms (or at least all themes), probably because the native-theme-imposed minimum size is the same as the "preffered" size on some platforms.

We should perhaps annotate it as fails on mac & Android, passes on windows, and passes-or-fails on linux... (or we could just mark it as skipped, which we typically try to avoid but is perhaps fine here to avoid having to create a bespoke .ini file with all the various combinations, since we don't care about this failure super-much and we have a bug filed with a rough explanation for where in our code the issue likely lies).

Ah, the Android debug failures were an unrelated issue fixed by another backout -- phew / never mind about those.

I'm marking the native-theme reftest (select-element-zero-height-001.html) as [PASS, FAIL] since it's not really worth tracking down exactly what the platforms it fails/succeeds on. It could even depend on having a high DPI display on some platforms, since on OS X it appears to only differ by aliasing on a retina display.
We also have a bug to track the issue, and a second reftest which is not platform-dependent.

Flags: needinfo?(emcdonough)
See Also: → 1596627
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a6de47cc1f6
When resolving min-size:auto on the main axis of flex items, treat `overflow:-moz-hidden-unscrollable` the same as `overflow:visible`. r=dholbert
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd1fe84753a1
Add reftests for select elements in zero-height flex containers r=dholbert
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Upstream PR merged by moz-wptsync-bot
Regressions: 1606342
You need to log in before you can comment on or make changes to this bug.