Bug 198230

Summary: [css-overflow] Implement clip value for overflow
Product: WebKit Reporter: jonjohnjohnson <hi>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: cathiechen, changseok, clopez, corsar89, crzwdjk, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, kai.hollberg, koivisto, kondapallykalyan, macpherson, menard, pdr, rbuis, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 227899, 236008    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description jonjohnjohnson 2019-05-24 13:46:45 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-08-16 15:46:15 PDT
<rdar://problem/54410709>
Comment 2 Kai 2021-03-15 10:35:43 PDT
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?
Comment 3 Simon Fraser (smfr) 2021-03-15 11:10:44 PDT
Code has diverged enough that porting changes like this is hard (and we don't always agree which how things are implemented in Blink).
Comment 4 Rob Buis 2021-07-09 07:08:46 PDT
Created attachment 433215 [details]
Patch
Comment 5 Rob Buis 2021-07-11 06:19:37 PDT
Created attachment 433287 [details]
Patch
Comment 6 Rob Buis 2021-07-12 05:40:29 PDT
Created attachment 433313 [details]
Patch
Comment 7 Rob Buis 2021-07-12 06:57:32 PDT
Created attachment 433315 [details]
Patch
Comment 8 Rob Buis 2021-07-12 09:03:43 PDT
Created attachment 433324 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-07-12 10:38:23 PDT
Comment on attachment 433324 [details]
Patch

Please do the isScrollContainer() rename in a separate patch.
Comment 10 Rob Buis 2021-07-13 07:53:25 PDT
(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.
Comment 11 Rob Buis 2021-07-14 02:30:13 PDT
Created attachment 433494 [details]
Patch
Comment 12 Rob Buis 2021-07-15 02:09:00 PDT
Created attachment 433569 [details]
Patch
Comment 13 Rob Buis 2021-07-15 09:28:01 PDT
Created attachment 433590 [details]
Patch
Comment 14 EWS Watchlist 2021-07-15 09:28:51 PDT
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
Comment 15 Rob Buis 2021-07-15 09:41:27 PDT
Created attachment 433592 [details]
Patch
Comment 16 Simon Fraser (smfr) 2021-07-15 09:54:39 PDT
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.
Comment 17 Rob Buis 2021-07-15 12:03:04 PDT
Created attachment 433607 [details]
Patch
Comment 18 Rob Buis 2021-07-15 12:07:18 PDT
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!
Comment 19 Rob Buis 2021-07-15 12:08:15 PDT
(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.
Comment 20 Simon Fraser (smfr) 2021-07-15 12:19:57 PDT
(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?
Comment 21 Rob Buis 2021-07-15 13:04:34 PDT
(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.
Comment 22 Rob Buis 2021-07-16 12:57:59 PDT
BTW does this need a runtime setting?
Comment 23 Darin Adler 2021-07-28 12:52:21 PDT
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.
Comment 24 Rob Buis 2021-07-29 07:21:08 PDT
(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.
Comment 25 Darin Adler 2021-07-29 10:34:26 PDT
(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.
Comment 26 Rob Buis 2021-07-29 10:46:17 PDT
Created attachment 434534 [details]
Patch
Comment 27 Rob Buis 2021-07-29 11:04:09 PDT
(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).
Comment 28 Darin Adler 2021-07-29 12:59:00 PDT
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.
Comment 29 Rob Buis 2021-07-29 20:23:42 PDT
Created attachment 434600 [details]
Patch
Comment 30 Rob Buis 2021-07-30 01:45:09 PDT
Created attachment 434612 [details]
Patch
Comment 31 Rob Buis 2021-07-30 07:32:00 PDT
(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.
Comment 32 Simon Fraser (smfr) 2021-07-30 10:51:18 PDT
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
Comment 33 Rob Buis 2021-07-30 22:02:21 PDT
Created attachment 434689 [details]
Patch
Comment 34 EWS 2021-07-30 23:07:17 PDT
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].
Comment 35 Rob Buis 2021-08-02 13:27:55 PDT
(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.
Comment 36 Arcady Goldmints-Orlov 2021-08-04 14:15:38 PDT
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.
Comment 37 Rob Buis 2021-11-24 08:20:50 PST
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.
Comment 38 Rob Buis 2021-12-07 06:15:51 PST
ping?
Comment 39 Simon Fraser (smfr) 2022-02-01 15:13:46 PST
Yes, we should enable it by default. Please do so via a new bug.
Comment 40 Rob Buis 2022-02-02 04:02:20 PST
(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