Summary: | [css-overflow] Implement clip value for overflow | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jonjohnjohnson <hi> | ||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | 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
jonjohnjohnson
2019-05-24 13:46:45 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? Code has diverged enough that porting changes like this is hard (and we don't always agree which how things are implemented in Blink). Created attachment 433215 [details]
Patch
Created attachment 433287 [details]
Patch
Created attachment 433313 [details]
Patch
Created attachment 433315 [details]
Patch
Created attachment 433324 [details]
Patch
Comment on attachment 433324 [details]
Patch
Please do the isScrollContainer() rename in a separate patch.
(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. Created attachment 433494 [details]
Patch
Created attachment 433569 [details]
Patch
Created attachment 433590 [details]
Patch
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 Created attachment 433592 [details]
Patch
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. Created attachment 433607 [details]
Patch
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! (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. (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? (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. BTW does this need a runtime setting? 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. (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. (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. Created attachment 434534 [details]
Patch
(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 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. Created attachment 434600 [details]
Patch
Created attachment 434612 [details]
Patch
(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 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 Created attachment 434689 [details]
Patch
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]. (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. 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. 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. ping? Yes, we should enable it by default. Please do so via a new bug. (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 |