Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706673002:
Mark elements as viewport constrained when they become sticky positioned (Closed)
|
Created:
3 years, 10 months ago by smcgruer Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMark elements as viewport constrained when they become sticky positioned
Previously changing an element from sticky to non-sticky would remove it
from the set of viewport constrained elements, but the inverse was not
honored. This caused broken behavior if you cycled the position for an
element between sticky and any other position.
BUG=685658
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2706673002
Cr-Commit-Position: refs/heads/master@{#452467}
Committed: https://chromium.googlesource.com/chromium/src/+/c4550918a382319dd463b8ca7735f5b32f39ccbe
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change approach as per reviewer comments #Patch Set 3 : Fix weirdly formatted comment #Patch Set 4 : Add LayoutTest #
Total comments: 12
Patch Set 5 : Address reviewer comments #
Total comments: 4
Patch Set 6 : Layout test; put the hidden overflow on html not body #Patch Set 7 : Address inner requestAnimationFrame nit #Patch Set 8 : Rebase #
Messages
Total messages: 29 (16 generated)
smcgruer@chromium.org changed reviewers: + flackr@chromium.org
I'm having trouble reproducing http://output.jsbin.com/xomuruk/60 as a LayoutTest that consistently fails on current Chrome (e.g. it seems like calling rAF is enough to tickle the before-patch code into working properly anyway), which may indicate that this patch is not the exact correct fix to apply. But it's a good starting point for a discussion at least :).
https://codereview.chromium.org/2706673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2706673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:402: frameView->addViewportConstrainedObject(this); There's a call to add sticky position items to the set of viewport constrained objects which happens during the compositing inputs update. Can you check why the sticky element isn't getting added then? Doing it here is incorrect as we don't know whether it is contained by an overflow scroller yet.
Description was changed from ========== Mark elements as viewport constrained when they become sticky positioned Previously changing an element from sticky to non-sticky would remove it from the set of viewport constrained elements, but the inverse was not honored. This caused broken behavior if you cycled the position for an element between sticky and any other position. BUG=685658 ========== to ========== Mark elements as viewport constrained when they become sticky positioned Previously changing an element from sticky to non-sticky would remove it from the set of viewport constrained elements, but the inverse was not honored. This caused broken behavior if you cycled the position for an element between sticky and any other position. BUG=685658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Still working on a LayoutTest; it currently reproduces the bug only 75% of the time which is strange. https://codereview.chromium.org/2706673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2706673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:402: frameView->addViewportConstrainedObject(this); On 2017/02/21 00:38:14, flackr wrote: > There's a call to add sticky position items to the set of viewport constrained > objects which happens during the compositing inputs update. Can you check why > the sticky element isn't getting added then? Doing it here is incorrect as we > don't know whether it is contained by an overflow scroller yet. Done.
I've added a layout test which does work (e.g. fails on ToT, succeeds after this patch), but there are some fun quirks to make it reliable. Take a look and lmk if you're happy with them or if we want to put some more thought into it.
https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change-expected.html (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change-expected.html:17: background-color: green; Can we merge .relative and .box since there is only a single element with both of these classes? https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:40: var x = sticky.offsetTop; nit: just call stickyoffsetTop; https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:47: // *looks* correct for the comparison. Expected? I think what happens is the change to position sticky sets needs compositing inputs, and if you scroll without waiting a frame then updating the compositing inputs also updates the position of the sticky layer to be correct. Use requestAnimationFrame instead of setTimeout. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:58: // TODO(smcgruer): For some reason, a rAF does not wait for this, but a 0 second timeout does. This sounds likely to flake, I think setTimeout is working because of the minimum delay before running. A single rAF only waits for the start of a frame which also doesn't require clean compositing inputs but if you rAF twice then your function will be run at the start of the next frame after a frame has been produced. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:7: #include <memory> I think convention is also to have a blank line between system includes and project includes. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:114: // viewport constrained anymore. This might be re-added below if the new I don't think the case in this comment can happen. If the previous overflow layer was the root layer and is not equal to the new overflow clip layer then it's not possible for the new one to be the root is it?
https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change-expected.html (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change-expected.html:17: background-color: green; On 2017/02/22 14:45:42, flackr wrote: > Can we merge .relative and .box since there is only a single element with both > of these classes? Done. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:40: var x = sticky.offsetTop; On 2017/02/22 14:45:42, flackr wrote: > nit: just call stickyoffsetTop; Done. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:47: // *looks* correct for the comparison. Expected? On 2017/02/22 14:45:42, flackr wrote: > I think what happens is the change to position sticky sets needs compositing > inputs, and if you scroll without waiting a frame then updating the compositing > inputs also updates the position of the sticky layer to be correct. > > Use requestAnimationFrame instead of setTimeout. Done. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:58: // TODO(smcgruer): For some reason, a rAF does not wait for this, but a 0 second timeout does. On 2017/02/22 14:45:42, flackr wrote: > This sounds likely to flake, I think setTimeout is working because of the > minimum delay before running. A single rAF only waits for the start of a frame > which also doesn't require clean compositing inputs but if you rAF twice then > your function will be run at the start of the next frame after a frame has been > produced. Done. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameViewTest.cpp:7: #include <memory> On 2017/02/22 14:45:42, flackr wrote: > I think convention is also to have a blank line between system includes and > project includes. Done. https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp (right): https://codereview.chromium.org/2706673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:114: // viewport constrained anymore. This might be re-added below if the new On 2017/02/22 14:45:42, flackr wrote: > I don't think the case in this comment can happen. If the previous overflow > layer was the root layer and is not equal to the new overflow clip layer then > it's not possible for the new one to be the root is it? Done.
https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html (right): https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:13: overflow: hidden; /* hide scrollbars */ I just noticed this, isn't this making the body an overflow scroller which means we're not exercising the frameview scrolling code? https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:57: doTest(); nit: just requestAnimationFrame(doTest); for the inner raf :-)
https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html (right): https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:13: overflow: hidden; /* hide scrollbars */ On 2017/02/22 15:43:02, flackr wrote: > I just noticed this, isn't this making the body an overflow scroller which means > we're not exercising the frameview scrolling code? As per discussion, this is actually ok because the body overflow is passed over to the root instead, but to avoid confusion moved to html. https://codereview.chromium.org/2706673002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-style-change.html:57: doTest(); On 2017/02/22 15:43:01, flackr wrote: > nit: just requestAnimationFrame(doTest); for the inner raf :-) Done.
lgtm
The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
smcgruer@chromium.org changed reviewers: + chrishtr@chromium.org
Adding Chris for blink OWNERS
pdr@chromium.org changed reviewers: + pdr@chromium.org
LGTM
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2706673002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487851241670560,
"parent_rev": "2391315fa51475da9ea5314ed81990e709644659", "commit_rev":
"c4550918a382319dd463b8ca7735f5b32f39ccbe"}
Message was sent while issue was closed.
Description was changed from ========== Mark elements as viewport constrained when they become sticky positioned Previously changing an element from sticky to non-sticky would remove it from the set of viewport constrained elements, but the inverse was not honored. This caused broken behavior if you cycled the position for an element between sticky and any other position. BUG=685658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Mark elements as viewport constrained when they become sticky positioned Previously changing an element from sticky to non-sticky would remove it from the set of viewport constrained elements, but the inverse was not honored. This caused broken behavior if you cycled the position for an element between sticky and any other position. BUG=685658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2706673002 Cr-Commit-Position: refs/heads/master@{#452467} Committed: https://chromium.googlesource.com/chromium/src/+/c4550918a382319dd463b8ca7735... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/c4550918a382319dd463b8ca7735... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
