Bugzilla

Comment 1

•

5 years ago

Attached file Test-case from reporter. — Details

Comment 2

•

5 years ago

This seems to be a regression.

Status: UNCONFIRMED → NEW

Ever confirmed: true

Comment 3

•

5 years ago

Mozregression gives me https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e4ea471e2d1c8db0c5f3a766bcb989bf95b76ad&tochange=861067332bac96a44bbf41ef366f58a30476057b, but I don't see anything very suspect there.

Keywords: regression

Comment 4

•

5 years ago

Attached file Test-case without theming. — Details

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.

Comment 5

•

5 years ago

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

Flags: needinfo?(dholbert)

Keywords: regression

Priority: -- → P3

Comment 6

•

5 years ago

Attached file testcase 3 (more extreme) — Details

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)

Comment 7

•

5 years ago

•

Edited

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

Updated

•

5 years ago

Assignee: nobody → emcdonough

Status: NEW → ASSIGNED

Comment 8

•

5 years ago

(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.)

Updated

•

5 years ago

Summary: Select content height is zero in a flex container → Select content height inco in a flex container

Updated

•

5 years ago

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

Assignee

Comment 9

•

5 years ago

Attached file Bug 1591925 - When resolving min-size:auto on the main axis of flex items, treat `overflow:-moz-hidden-unscrollable` the same as `overflow:visible`. — Details

Assignee

Comment 10

•

5 years ago

Attached file Bug 1591925 - Add reftests for select elements in zero-height flex containers — Details

Phabricator Automation

Updated

•

5 years ago

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`.

Phabricator Automation

Updated

•

5 years ago

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

Phabricator Automation

Updated

•

5 years ago

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

Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)

Comment 11

•

5 years ago

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).

Pulsebot

Comment 12

•

5 years ago

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

Comment 13

•

5 years ago

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

Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)

Comment 14

•

5 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)

Comment 15

•

5 years ago

Upstream PR was closed without merging

Oana Pop-Rus

Comment 16

•

5 years ago

Backed out 2 changesets (bug 1591925) for reftest failures on flexbox-min-bsize-keywords-vert-1.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=275700379&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=eb9d579fe07c3948a8201622c940502153490b16

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=275700379&repo=autoland&lineNumber=4315

Backout: https://hg.mozilla.org/integration/autoland/rev/9f2aadd2ad8fd807d8c152f8f5a9279fa9da70c1

Flags: needinfo?(emcdonough)

Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)

Assignee

Comment 17

•

5 years ago

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

Flags: needinfo?(emcdonough)

Oana Pop-Rus

Comment 18

•

5 years ago

Emily,

we also have devtools failures like: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=275707252&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=eb9d579fe07c3948a8201622c940502153490b16&searchStr=devtools

Pulsebot

Comment 19

•

5 years ago

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

Comment 20

•

5 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Natalia Csoregi [:nataliaCs]

Comment 21

•

5 years ago

Backed out for failures on /select-element-zero-height-001.html

backout: https://hg.mozilla.org/integration/autoland/rev/771c82ef46a350365f7aad056eb30cea2fae820c

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=b2d168294899bb4700cb3bd929c0bde85879fcea&selectedJob=276125998

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276125998&repo=autoland&lineNumber=16384

Flags: needinfo?(emcdonough)

Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)

Comment 22

•

5 years ago

Upstream PR was closed without merging

Updated

•

5 years ago

Comment 23

•

5 years ago

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).

Comment 24

•

5 years ago

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

Assignee

Comment 25

•

5 years ago

•

Edited

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)