Bug 198230 – [css-overflow] Implement clip value for overflow

• Component: CSS
• Status: RESOLVED FIXED

[jonjohnjohnson]

See spec:  https://www.w3.org/TR/css-overflow-3/#valdef-overflow-clip

See geckos ticket for implementing and renaming "-moz-hidden-unscrollable" value:  https://bugzilla.mozilla.org/show_bug.cgi?id=1531609

See contentious posSticky/overflow csswg ticket that would be be cleared up:  https://github.com/w3c/csswg-drafts/issues/865

I'm surprised this wasn't already filed, but guessing @smfr (simon fraser) already has a bunch of thoughts on how troublesome this could be in webkit.

[Radar WebKit Bug Importer]

<rdar://problem/54410709>

[Kai]

The clip value is coming with Chrome 90, Safari is the last one missing:
- https://caniuse.com/mdn-css_properties_overflow_clip
- https://developer.mozilla.org/en-US/docs/Web/CSS/overflow#browser_compatibility
- https://www.chromestatus.com/feature/5638444178997248
- https://caniuse.com/css-overflow (PR to update is pending) 

Maybe since it's in blink now a port could be easier than matching gecko?

[Simon Fraser (smfr)]

Code has diverged enough that porting changes like this is hard (and we don't always agree which how things are implemented in Blink).

[Rob Buis]

Created attachment 433215 [details]
Patch

[Rob Buis]

Created attachment 433287 [details]
Patch

[Rob Buis]

Created attachment 433313 [details]
Patch

[Rob Buis]

Created attachment 433315 [details]
Patch

[Rob Buis]

Created attachment 433324 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433324 [details]
Patch

Please do the isScrollContainer() rename in a separate patch.

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 433324 [details]
> Patch
> 
> Please do the isScrollContainer() rename in a separate patch.

Sure, I created https://bugs.webkit.org/show_bug.cgi?id=227899 for this.

[Rob Buis]

Created attachment 433494 [details]
Patch

[Rob Buis]

Created attachment 433569 [details]
Patch

[Rob Buis]

Created attachment 433590 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 433592 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433592&action=review

Do we need any requiresLayer()/RenderLayerScrollableArea changes for this?

> Source/WebCore/css/CSSProperties.json:3635
> +                "clip"

Move "clip" to after "hidden"

> Source/WebCore/css/CSSProperties.json:3650
> +                "clip"

Move "clip" to after "hidden"

> Source/WebCore/page/FrameView.cpp:652
> +    case Overflow::Clip:

Do we allow programmatic scrolling for an "overflow:clip" viewport? Should we?

> Source/WebCore/rendering/style/RenderStyleConstants.h:282
> +    Clip

Move "Clip" to after "Hidden"

> Source/WebCore/style/StyleAdjuster.cpp:430
> +    if (style.overflowY() != Overflow::Visible && style.overflowY() != Overflow::Clip) {
> +        if (style.overflowX() == Overflow::Visible) {
> +            // FIXME: Once we implement pagination controls, overflow-x should default to hidden
> +            // if overflow-y is set to -webkit-paged-x or -webkit-page-y. For now, we'll let it
> +            // default to auto so we can at least scroll through the pages.
> +            style.setOverflowX(Overflow::Auto);
> +        } else if (style.overflowX() == Overflow::Clip)
> +            style.setOverflowX(Overflow::Hidden);
> +    } else if (style.overflowX() != Overflow::Visible && style.overflowX() != Overflow::Clip) {
> +        if (style.overflowY() == Overflow::Visible)
> +            style.setOverflowY(Overflow::Auto);
> +        else if (style.overflowY() == Overflow::Clip)
> +            style.setOverflowY(Overflow::Hidden);
> +    }

I feel like this could be simplified using a lambda that you call for each axis.

[Rob Buis]

Created attachment 433607 [details]
Patch

[Rob Buis]

Comment on attachment 433592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433592&action=review

>> Source/WebCore/css/CSSProperties.json:3635
>> +                "clip"
> 
> Move "clip" to after "hidden"

Done.

>> Source/WebCore/css/CSSProperties.json:3650
>> +                "clip"
> 
> Move "clip" to after "hidden"

Done.

>> Source/WebCore/page/FrameView.cpp:652
>> +    case Overflow::Clip:
> 
> Do we allow programmatic scrolling for an "overflow:clip" viewport? Should we?

No the specification says we should not (https://drafts.csswg.org/css-overflow/#valdef-overflow-clip):
" In addition, unlike overflow: hidden which still allows programmatic scrolling, overflow: clip forbids scrolling entirely"

>> Source/WebCore/rendering/style/RenderStyleConstants.h:282
>> +    Clip
> 
> Move "Clip" to after "Hidden"

Done.

>> Source/WebCore/style/StyleAdjuster.cpp:430
>> +    }
> 
> I feel like this could be simplified using a lambda that you call for each axis.

Done. Much nicer indeed!

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 433592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433592&action=review
> 
> Do we need any requiresLayer()/RenderLayerScrollableArea changes for this?

I do not think so.

[Simon Fraser (smfr)]

(In reply to Rob Buis from comment #18)
> Comment on attachment 433592 [details]

> No the specification says we should not
> (https://drafts.csswg.org/css-overflow/#valdef-overflow-clip):
> " In addition, unlike overflow: hidden which still allows programmatic
> scrolling, overflow: clip forbids scrolling entirely"
 
Is that done in this patch? Does it work for macOS and iOS? Any tests for it?

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #20)
> (In reply to Rob Buis from comment #18)
> > Comment on attachment 433592 [details]
> 
> > No the specification says we should not
> > (https://drafts.csswg.org/css-overflow/#valdef-overflow-clip):
> > " In addition, unlike overflow: hidden which still allows programmatic
> > scrolling, overflow: clip forbids scrolling entirely"
>  
> Is that done in this patch? Does it work for macOS and iOS? Any tests for it?

Yes, it is tested by overflow-clip-cant-scroll.html.

[Rob Buis]

BTW does this need a runtime setting?

[Darin Adler]

Comment on attachment 433607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433607&action=review

> Source/WebCore/rendering/RenderObject.h:440
> +    bool hasPotentiallyScrollableOverflow() const { return hasNonVisibleOverflow() && style().overflowX() != Overflow::Clip && style().overflowX() != Overflow::Visible; }

Nothing in this name says "vertical scrolling"; it’s not obvious why it’s correct for this to check only overflowX.

[Rob Buis]

(In reply to Darin Adler from comment #23)
> Comment on attachment 433607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433607&action=review
> 
> > Source/WebCore/rendering/RenderObject.h:440
> > +    bool hasPotentiallyScrollableOverflow() const { return hasNonVisibleOverflow() && style().overflowX() != Overflow::Clip && style().overflowX() != Overflow::Visible; }
> 
> Nothing in this name says "vertical scrolling"; it’s not obvious why it’s
> correct for this to check only overflowX.

Right that may not be obvious right away. The thing is, internally 'visible' for one overflow dimension always has a 'visible' or 'clip' value in the other dimension, similarly 'clip' for one overflow dimension always has a 'visible' or 'clip' value in the other dimension, StyleAdjuster makes sure of this. Given that, it is enough to test either overflow dimension that neither of these two values are present, i.e. it covers all possible visible/clip combinations.

[Darin Adler]

(In reply to Rob Buis from comment #24)
> (In reply to Darin Adler from comment #23)
> > Nothing in this name says "vertical scrolling"; it’s not obvious why it’s
> > correct for this to check only overflowX.
> 
> Right that may not be obvious right away. The thing is, internally 'visible'
> for one overflow dimension always has a 'visible' or 'clip' value in the
> other dimension, similarly 'clip' for one overflow dimension always has a
> 'visible' or 'clip' value in the other dimension, StyleAdjuster makes sure
> of this. Given that, it is enough to test either overflow dimension that
> neither of these two values are present, i.e. it covers all possible
> visible/clip combinations.

This is the kind of thing that we want to make clear with comments or function names. And don’t want it in this kind of super-terse single-line function. Helps make the code maintainable in the future.

I like the idea of moving this function out of the class definition to the bottom of the file, and adding a quite terse and economical comment about why overflowX gives the answer for both directions.

Or abstracting it away into another function if there is more than one place that uses the same technique. But if hasPotentiallyScrollableOverflow is the only place that is taking advantage of this, then that’s not important.

[Rob Buis]

Created attachment 434534 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #25)
> This is the kind of thing that we want to make clear with comments or
> function names. And don’t want it in this kind of super-terse single-line
> function. Helps make the code maintainable in the future.
> 
> I like the idea of moving this function out of the class definition to the
> bottom of the file, and adding a quite terse and economical comment about
> why overflowX gives the answer for both directions.

That makes sense. I did that in my new patch, hard to make the comment terse, but I tried :)

> Or abstracting it away into another function if there is more than one place
> that uses the same technique. But if hasPotentiallyScrollableOverflow is the
> only place that is taking advantage of this, then that’s not important.

Pretty sure it is the only place (so far).

[Darin Adler]

Comment on attachment 434534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434534&action=review

Looks good to me, but I am not a layout expert.

We are doing this without a feature flag?

> Source/WebCore/rendering/RenderObject.h:1193
> +// We only need to test one overflow dimension since 'visible' and 'clip' always get accompanied
> +// with 'clip' or 'visible' in the other dimension (see Style::Adjuster::adjust).

Small subtle point, but I’d put this comment inside the function, since it’s about how the function works, not how you use it.

[Rob Buis]

Created attachment 434600 [details]
Patch

[Rob Buis]

Created attachment 434612 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #28)
> Comment on attachment 434534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434534&action=review
> 
> Looks good to me, but I am not a layout expert.
> 
> We are doing this without a feature flag?

Makes sense, I added a preference in the latest patch.
 
> > Source/WebCore/rendering/RenderObject.h:1193
> > +// We only need to test one overflow dimension since 'visible' and 'clip' always get accompanied
> > +// with 'clip' or 'visible' in the other dimension (see Style::Adjuster::adjust).
> 
> Small subtle point, but I’d put this comment inside the function, since it’s
> about how the function works, not how you use it.

Sure, done.

[Simon Fraser (smfr)]

Comment on attachment 434612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434612&action=review

So where do we actually reap the performance rewards from overflow:clip?

> Source/WebCore/style/StyleAdjuster.cpp:425
> +    if (style.overflowY() != Overflow::Visible && style.overflowY() != Overflow::Clip) {

I think the lambda could also include this test

[Rob Buis]

Created attachment 434689 [details]
Patch

[EWS]

Committed r280509 (240141@main): <https://commits.webkit.org/240141@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434689 [details].

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #32)
> Comment on attachment 434612 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434612&action=review
> 
> So where do we actually reap the performance rewards from overflow:clip?

We should through contain: paint, which will rely on overflow: clip. I started on some performance tests for that (https://bugs.webkit.org/show_bug.cgi?id=228696), expect to get some numbers soon.

[Arcady Goldmints-Orlov]

It appears that the imported/w3c/web-platform-tests/css/css-overflow/clip-006.html and imported/w3c/web-platform-tests/css/css-overflow/clip-007.html tests are failing on GTK and WPE since they were enabled by this change. For an example failure see: https://build.webkit.org/results/GTK-Linux-64-bit-Release-Tests/r280645%20(3061)/results.html

All the other css-overflow tests that were unskipped by this change appear to be fine.

[Rob Buis]

I think this is a feature that can be enabled, what do people think? Note that contain: paint support is not relying on overflow: clip to function.

[Rob Buis]

ping?

[Simon Fraser (smfr)]

Yes, we should enable it by default. Please do so via a new bug.

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #39)
> Yes, we should enable it by default. Please do so via a new bug.

Great! I made a new bug with patch here:
https://bugs.webkit.org/show_bug.cgi?id=236008