1531609 - [css-overflow] implement overflow:clip by renaming overflow:-moz-hidden-unscrollable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[jonjohnjohnson]

I'm primarily chiming in here in hopes of getting something out in the wild to encourage other implementers.

Fingers crossed the renaming of -moz-hidden-unscrollable fits right into the definition of https://drafts.csswg.org/css-overflow-3/#valdef-overflow-clip and we can see if the heavily debated position:sticky issue https://github.com/w3c/csswg-drafts/issues/865 can be done away with.

Any moz devs able to describe what is still needed relating to the "dev-doc-needed" keyword here?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I think the dev-doc-needed is just meant to apply when we fix it, so mdn compatibility docs get updated. The work here is:

• Checking that the -moz-hidden-unscrollable matches the spec.
• Confirm with the CSSWG that the feature is ready to ship.
• Do the renaming in the code, and probably tests as well.

I can mentor the code part of it (not sure I'd have cycles to tackle it myself right now).

Comment 3

[Sebastian Zartner [:sebo]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

I think the dev-doc-needed is just meant to apply when we fix it, so mdn compatibility docs get updated.

Correct. And it would be a great help for the MDN team if the implementers could set it right when filing bugs for implementing web platform features.

Sebastian

Comment 4

[Jeremy Ir]

I'd be interested in picking up this bug, if you all would be amiable to the idea. I've taken the initiative to familiarize myself with the CSS spec for overflow as well as attempted to somewhat expose myself to the implementation in the layout frame code.

Regarding Comment 1, based on what I'm seeing within the Gecko implementation of overflow, it appears that the position: sticky property would behave the same with MozHiddenUnscrollable as it does with auto/hidden. Is this a two-part bug in which we want to formally support overflow: clip as well as resolve the sticky issue? Or should that be addressed as a separate bug?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jeremy Ir from comment #4)

I'd be interested in picking up this bug, if you all would be amiable to the idea.

Yes, that'd be lovely!

Regarding Comment 1, based on what I'm seeing within the Gecko implementation of overflow, it appears that the position: sticky property would behave the same with MozHiddenUnscrollable as it does with auto/hidden. Is this a two-part bug in which we want to formally support overflow: clip as well as resolve the sticky issue? Or should that be addressed as a separate bug?

I don't think that's true, what makes you think that? In particular, Overflow::MozHiddenUnscrollable behaves as non-scrollable:

https://searchfox.org/mozilla-central/rev/b2015fdd464f598d645342614593d4ebda922d95/layout/style/nsStyleStruct.h#2126

Which means that it should not create a scroll frame, and thus position: sticky wouldn't be constrained by it. Though there may be some bugs around that... If so either separate bug or posting multiple patches to the bug should be acceptable.

Comment 6

[Jeremy Ir]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

ate bug?

I don't think that's true, what makes you think that? In particular, Overflow::MozHiddenUnscrollable behaves as non-scrollable:

I modified the repro example from the github thread (https://codepen.io/valtlai/pen/XVrbPY) with overflow-x:-moz-hidden-unscrollable in place of overflow-x: auto and was seeing the same behavior where the table headers were not sticking to the top of the scroll frame (FF 66.0.4, 64-bit, Win7). From that, I extrapolated that the sticky bug was at least partially unrelated to the non-scrollability of the element.

I may be missing something blatant that is producing this behavior, so please let me know, as this would impact my ability to test any changes I make.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jeremy Ir from comment #6)

I modified the repro example from the github thread (https://codepen.io/valtlai/pen/XVrbPY) with overflow-x:-moz-hidden-unscrollable in place of overflow-x: auto and was seeing the same behavior where the table headers were not sticking to the top of the scroll frame (FF 66.0.4, 64-bit, Win7). From that, I extrapolated that the sticky bug was at least partially unrelated to the non-scrollability of the element.

I may be missing something blatant that is producing this behavior, so please let me know, as this would impact my ability to test any changes I make.

Yeah, you're getting fooled by this rule:

https://drafts.csswg.org/css-overflow/#overflow-properties

Computed Value: as specified, except with visible/clip computing to auto/hidden (respectively) if one of overflow-x or overflow-y is neither visible nor clip.

Which is implemented here:

https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/servo/components/style/style_adjuster.rs#434

(Ignore the "deprecated" bit, I guess, since it was ressurrected on specs and there are valid use-cases for it :-))

So you need overflow: clip, not just overflow-x: clip.

Comment 8

[Jeremy Ir]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

So you need overflow: clip, not just overflow-x: clip.

Yep, that did the trick.

Would you mind helping me clarify something from the spec/code?

Computed Value: as specified, except with visible/clip computing to auto/hidden (respectively) if one of overflow-x or overflow-y is neither visible nor clip.

As I understand it, the initial/default value for overflow-y is visible. Based on the changes I used in Comment 6, the set value of overflow-x would be clip and the set value of overflow-y would visible, so the computed value should maintain the overflow-x: clip. Is my understanding of the spec itself correct?

The spec seems to allow for combinations of clip & visible. However the implementation in style_adjuster.rs seems to require both overflow directions to be set to visible for either direction to be set to visible (suggesting my understanding of the spec to be incorrect).

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Hmm, I think your reading of the spec is correct. There's various code all around that make assumptions that changing that would break, e.g.:

https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/layout/generic/nsFrame.cpp#9197

Seems that wording was introduced in https://github.com/w3c/csswg-drafts/issues/1971, relatively recently.

It doesn't seem insanely impossible to handle, though there are a lot of places relying on that, see all the callers of:

https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/layout/generic/nsFrame.h#588

Extend3DContext is the one where I'm not quite sure what's supposed to happen. Also, conceptually, clipping happens in rects. I assume you can define overflow: clip visible as clipping on a rectangle with definite bounds in one axis, and infinite bounds in the other.

Matt, David or Mats (cc'd) are probably much more familiar with overflow during painting and computation, so can probably have a better judgement on whether visible and clip combinations are reasonable to support.

Comment 10

[Matt Woodrow (:mattwoodrow)]

I think that sounds feasible.

Clipping does happen to a rect, but it seems like we could use the frame's pre-transform overflow area as the source of the axis for ones that are unclipped.

It looks like our computation of the overflow rect includes any necessary clipping, so using that as the clip (to enforce that the rendered overflow matches what was computed) should do the right thing.

For Extend3DContext, it looks like the latest draft (css-transforms-2 ED, transform-style:preserve-3d) suggests that 'clip' shouldn't stop the function from returning true (it just mentions overflow, not the per-axis versions). I don't think that's correct though, any form of clipping is super hard to implement in 3d space. I think any value that is not 'visible' (for either axis) should make Extend3DContext return false. I'm working on some rewrites for 3d transforms, will fix this.

Comment 11

[Mats Palmgren (inactive)]

Tab mentions both uses cases which indicates to me that they'll add what we currently do for -moz-hidden-unscrollable as a separate new value eventually. So I'm updating the title of this bug accordingly. To be clear: we should leave all code related to -moz-hidden-unscrollable as is and write new code for clip.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #11)

Tab mentions both uses cases which indicates to me that they'll add what we currently do for -moz-hidden-unscrollable as a separate new value eventually. So I'm updating the title of this bug accordingly. To be clear: we should leave all code related to -moz-hidden-unscrollable as is and write new code for clip.

Pretty sure -moz-hidden-unscrollable doesn't create a scroll container and behaves like clip is supposed to behave. Tab wants yet another thing.

Comment 13

[Mats Palmgren (inactive)]

Oh, my mistake. I thought we created a scroll frame but suppressed scrolling for that thing.
OK, then we should definitely just rename it and make it deal with potentially having visible in one axis.

Comment 14

[Jeremy Ir]

Alright, I'll implement this in 2 diffs, first with the name change, then with the functionality change.

Also, would someone mind changing the Assignee field to me please? I seem to not have permissions to do so.

Comment 15

[Jeremy Ir]

Attachment: Bug 1531609 - implement overflow:clip by renaming overflow:-moz-hidden-unscrollable, r=emilio

Renamed all instances of -moz-hidden-unscrollable with clip for the overflow
CSS property, as well as the related enumeration.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jir.opensource, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[Jeremy Ir]

The patch should have nothing to do with this bug. No code has been submitted from this and it's not a land-blocking bug.

Comment 18

[Jeremy Ir]

I may have to forgo fixing the outstanding bug for this item. There are a lot of places that ShouldApplyOverflowClipping touches and I've unfortunately struggled to get a grasp of how all of the calculations function.

Comment 19

[Tim Nguyen :ntim]

Emilio, what's the status of this patch ?

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Per spec overflow: clip should work in only one of the two axes, as long as the other is visible, so we need to make that work before landing the patch.

Comment 21

[Mats Palmgren (inactive)]

I'll take a stab at finishing this since I have an idea of how to implement the layout part correctly...

Comment 22

[Mats Palmgren (inactive)]

Attachment: Bug 1531609 part 1 - Rename overflow:-moz-hidden-unscrollable to overflow:clip. r=emilio

Comment 23

[Mats Palmgren (inactive)]

Attachment: Bug 1531609 part 2 - Implement overflow:clip/visible combinations. r=emilio

Depends on D73716

Comment 24

[Mats Palmgren (inactive)]

Attachment: Testcase: -moz-hidden-unscrollable/clip propagation to the viewport.

So if I understand the spec[1] correctly, the overflow value on <body> is supposed to be propagated to the viewport in this case. That is, you should see a large blue rectangle that is clipped by the viewport on the right/bottom side (with no scrollbars, since it should behave as hidden). It should not be clipped by <body> (like we currently do) because the spec says:

The element from which the value is propagated must then have a used overflow value of visible.

[1]
https://drafts.csswg.org/css-overflow/#overflow-propagation

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Hmm, that is a bit unfortunate implementation-wise :(

Comment 26

[Mats Palmgren (inactive)]

Indeed, I suspect we need a global frame bit for this. Anyway, I filed bug 1635473 to fix that separately first.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think it's worth asking whether that's intentional -- another reasonable option is that only the scrollbar-related ones propagate to the viewport.

What do other implementations do?

Comment 28

[Mats Palmgren (inactive)]

Chrome and Safari don't implement clip (nor some -webkit- version of it) AFAICT.

I don't feel strongly either way, but the spec behavior was resolved somewhat recently by the WG:
https://github.com/w3c/csswg-drafts/issues/1971#issuecomment-380148099

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that resolution was about something slightly different: it was already assuming that it does propagate, and answering the question of what happens when the value gets to the viewport (which must have a scrolling mechanism and possibly scrollbars).

Comment 30

[Mats Palmgren (inactive)]

Resolving what happens after it has propagated is effectively a resolution that it should propagate IMO. Otherwise those discussions would be rather pointless, or at least would have come to the opposite conclusion that it shouldn't propagate in the first place. So the resolution being what it is gives me the impression that the WG's consensus is that clip does propagate and then also decides what happens when it does.

Feel free to raise a spec issue though if you think it's worth revisiting.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think you're reading an awful lot into a discussion that took about 30 seconds (this line and the two following) and involved 2 people. Probably most of the rest of the room hadn't understood what issue was being discussed.

Comment 32

[Mats Palmgren (inactive)]

The propagation was also discussed in some detail by Florian and Tab in the issue before that, so it still appears like a deliberate decision to me.

Comment 33

[Cameron McCormack (:heycam)]

Document::IsPotentiallyScrollable (which I'm going to make a change to in bug 1654099) will need to be updated to check for clip values.

Comment 34

[Mats Palmgren (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #33)

Document::IsPotentiallyScrollable (which I'm going to make a change to in bug 1654099) will need to be updated to check for clip values.

It looks like you've already changed it to using IsScrollableOverflow() instead of checking individual style values, so I think we're fine.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e64a61869cdb
part 1 - Rename overflow:-moz-hidden-unscrollable to overflow:clip.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/1e7b32808be8
part 2 - Implement overflow:clip/visible combinations.  r=emilio

Comment 36

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

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 37

[Atila Butkovits]

Backed out for reftest failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b734942650d428f3950f547fda4a1f030ce280c1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1e7b32808be8fdce40874dcdf417beecedbdc644&searchStr=Windows%2C10%2Cx64%2CWebRender%2Cdebug%2CReftests%2Ctest-windows10-64-qr%2Fdebug-reftest-e10s%2CR4&selectedTaskRun=Ky1JKXllSICM7nQhN_P2uA.0

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

Comment 38

[Mats Palmgren (inactive)]

Those failures appears to be small differences in text anti-aliasing in some XUL tests.
I don't think this indicates a bug so I'll just add a fuzz factor for these tests in the reftest.list manifest...

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf1609de8842
part 1 - Rename overflow:-moz-hidden-unscrollable to overflow:clip.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/e0fcac18ed9a
part 2 - Implement overflow:clip/visible combinations.  r=emilio

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cf1609de8842
https://hg.mozilla.org/mozilla-central/rev/e0fcac18ed9a

Comment 41

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

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

Comment 42

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

Upstream PR merged by moz-wptsync-bot

Comment 43

[Rachel Andrew]

I've updated the MDN page https://wiki.developer.mozilla.org/en-US/docs/Web/CSS/overflow

BCD has been merged so will show up eventually.