Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825343003:
Clean compositing inputs for location APIs for sticky-affected elements. (Closed)
|
Created:
3 years, 8 months ago by smcgruer Modified:
3 years, 7 months ago CC:
fs, blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean compositing inputs for location APIs for sticky-affected elements.
After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.
BUG=672457
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2825343003
Cr-Commit-Position: refs/heads/master@{#471221}
Committed: https://chromium.googlesource.com/chromium/src/+/383d1bb3f30b49fcdf3918cefc588637aa678ec0
Patch Set 1 #
Total comments: 6
Patch Set 2 : Switch to using an inherited StyleRareInheritedData #Patch Set 3 : Address reviewer comments #
Total comments: 8
Patch Set 4 : Address reviewer comments #Patch Set 5 : Also fix the other layout test #
Total comments: 7
Patch Set 6 : Add test for sticky subtrees #
Total comments: 24
Patch Set 7 : First pass addressing reviewer comments #
Total comments: 8
Patch Set 8 : Remove UNLIKELY #Patch Set 9 : Fix StyleAdjuster.cpp code #Patch Set 10 : Improve unit-tests #Patch Set 11 : Rename EnsureLifecycleValidForLocationAPIsForNode #
Total comments: 2
Patch Set 12 : Rebase #Patch Set 13 : Add intermediate sticky in StickySubtreesAreTrackedCorrectly test #
Total comments: 2
Patch Set 14 : More bikeshedding #Patch Set 15 : Change bit name #Patch Set 16 : Rebase #Messages
Total messages: 69 (31 generated)
smcgruer@chromium.org changed reviewers: + flackr@chromium.org
Description was changed from ========== Clean compositing inputs for location APIs for sticky-affected elements. After layout, the location of position:sticky elements and their descendants will be incorrect until compositing inputs have been cleaned. In most cases this is not an issue since Blink will run a full lifecycle after page change, including compositing. However if the user modifies layout during a script and then calls a location-based API without yielding, compositing inputs would still be dirty. This patch corrects that behavior by ensuring that compositing inputs are clean for location-related APIs. BUG=672457 ========== to ========== Clean compositing inputs for location APIs for sticky-affected elements. After layout, the location of position:sticky elements and their descendants will be incorrect until compositing inputs have been cleaned. In most cases this is not an issue since Blink will run a full lifecycle after page change, including compositing. However if the user modifies layout during a script and then calls a location-based API without yielding, compositing inputs would still be dirty. This patch corrects that behavior by ensuring that compositing inputs are clean for location-related APIs. BUG=672457 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
smcgruer@chromium.org changed reviewers: + chrishtr@chromium.org
Chris, Rob, This is a re-implementation of PS2 from https://codereview.chromium.org/2647533002/. As discussed in that code review there are a number of issues with this approach[0], but I felt it was a good place to start a discussion. I'm interested in any ideas you have on how we can best detect and track sticky sub-trees for this. [0] Known problems: * StyleWillChange is not called at the right time, so the Parent() appears to still be <null> for everything; * There is possibly a walk involved with the sticky checking (not sure). * sticky-position-works-with-scroll-apis.html fails and I'm not 100% sure why yet.
On 2017/04/19 14:36:39, smcgruer wrote: > Chris, Rob, > > This is a re-implementation of PS2 from > https://codereview.chromium.org/2647533002/. As discussed in that code review > there are a number of issues with this approach[0], but I felt it was a good > place to start a discussion. I'm interested in any ideas you have on how we can > best detect and track sticky sub-trees for this. > > [0] Known problems: > > * StyleWillChange is not called at the right time, so the Parent() appears to > still be <null> for everything; > * There is possibly a walk involved with the sticky checking (not sure). For these two issues, something based on Element::recalcStyle is probably the right place / time. When a top-level stickiness bit changes, Element::RecalcOwnStyle can return at least IndependentInherit so that we will visit all descendants to update their flags. > * sticky-position-works-with-scroll-apis.html fails and I'm not 100% sure why > yet.
Looks generally good except that those unittests fail, and the other comments below. https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:4343: view->UpdateLifecycleToCompositingInputsClean(); Add a comment explaining why this is needed. https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:483: (Parent() && Parent()->EnclosingBoxModelObject() && Shouldn't this use the containing block chain, not the parent chain? https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:859: InvalidateAllStickyConstraints(); This looks like an independent bug. Does it deserve its own CL?
On 2017/04/20 06:32:00, flackr wrote: > For these two issues, something based on Element::recalcStyle is probably the > right place / time. When a top-level stickiness bit changes, > Element::RecalcOwnStyle can return at least IndependentInherit so that we will > visit all descendants to update their flags. Working on this. Seems that RecalcStyle is not always called (or isn't called on initial style). My super-simple testcase of http://jsbin.com/tukihos/edit?html,css only triggers the following output: $ ./out/Release/content_shell http://output.jsbin.com/tukihos/quiet [1:1:0424/112805.763872:2679492407473:INFO:Element.cpp(1881)] "HTML": RecalcStyle, change=5 [1:1:0424/112806.353544:2679492997174:INFO:Element.cpp(1881)] "HTML": RecalcStyle, change=0 Will continue digging to see if I can understand the style lifecycle better :)
Switched to using an inherited rare data css style which appears to work. This may result in push-back when we go to wider review, but I'm not sure how else we could do this. Also addressed Chris' comments. There are still a few things to tidy up, but this should be good for re-review, thanks. https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:4343: view->UpdateLifecycleToCompositingInputsClean(); On 2017/04/20 17:40:34, chrishtr wrote: > Add a comment explaining why this is needed. Done. https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:483: (Parent() && Parent()->EnclosingBoxModelObject() && On 2017/04/20 17:40:34, chrishtr wrote: > Shouldn't this use the containing block chain, not the parent chain? No longer relevant; see new PS. https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2825343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:859: InvalidateAllStickyConstraints(); On 2017/04/20 17:40:34, chrishtr wrote: > This looks like an independent bug. Does it deserve its own CL? Yes, it probably does. Removed the scroll related stuff, will create a new CL for it. This means that scrollIntoView/scrollIntoViewIfNeeded will still be broken after this CL.
I think an alternative is to make RecalcStyle get called by dirtying the style on attachment if the attachment point is in a sticky subtree. That being said, this seems like an okay way to inherit the property if style people are okay with it. It will be overly applied (as we only need to know if the element is in a sticky subtree which offsets its position) but that's probably not worth optimizing for. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html (right): https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:43: var scroller = document.getElementById('scroller'); nit: Let's call the id's scroller1 and scroller2 for consistency and to keep the id's unique from the class scroller. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:49: scroller.insertBefore(sticky, document.getElementById('padding')); Same as above for padding. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:65: assert_equals(sticky.getBoundingClientRect().left, 50); Just to make it really obvious where these numbers come from perhaps call getBoundingClientRect() on the scroller and then add 20 and 50 to the top and left of that bounding client rect? https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:1678: SET_VAR(rare_inherited_data_, is_in_sticky_subtree_, b); Does setting this property, even if to false, mean that we always create the rare inerited properties? Maybe we need to avoid creating it if b == false?
https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html (right): https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:43: var scroller = document.getElementById('scroller'); On 2017/04/26 15:08:00, flackr wrote: > nit: Let's call the id's scroller1 and scroller2 for consistency and to keep the > id's unique from the class scroller. Done. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:49: scroller.insertBefore(sticky, document.getElementById('padding')); On 2017/04/26 15:08:00, flackr wrote: > Same as above for padding. Done. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html:65: assert_equals(sticky.getBoundingClientRect().left, 50); On 2017/04/26 15:08:00, flackr wrote: > Just to make it really obvious where these numbers come from perhaps call > getBoundingClientRect() on the scroller and then add 20 and 50 to the top and > left of that bounding client rect? Done. https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2825343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:1678: SET_VAR(rare_inherited_data_, is_in_sticky_subtree_, b); On 2017/04/26 15:08:00, flackr wrote: > Does setting this property, even if to false, mean that we always create the > rare inerited properties? Maybe we need to avoid creating it if b == false? As discussed, SET_VAR only creates a new rare inherited data for a given node if the value set is different from the parent value. Otherwise it just reuses the parent rare inherited data.
https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); I patched this in and I only see StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and only on the elements which have had position explicitly set on them (i.e. #a and #b on http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting correctly?
https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/27 15:50:02, flackr wrote: > I patched this in and I only see > StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and only on > the elements which have had position explicitly set on them (i.e. #a and #b on > http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting correctly? Did you set position to inherit? That is the case where this method would be called.
https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/27 16:05:28, smcgruer wrote: > On 2017/04/27 15:50:02, flackr wrote: > > I patched this in and I only see > > StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and only on > > the elements which have had position explicitly set on them (i.e. #a and #b on > > http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting correctly? > > Did you set position to inherit? That is the case where this method would be > called. But if the methods don't get called automatically on the descendants then won't we have the same bug if you ask for the offset of a descendant of a sticky element? (i.e. we won't have updated the IsInStickySubtree property to know that).
https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/27 17:17:01, flackr wrote: > On 2017/04/27 16:05:28, smcgruer wrote: > > On 2017/04/27 15:50:02, flackr wrote: > > > I patched this in and I only see > > > StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and only > on > > > the elements which have had position explicitly set on them (i.e. #a and #b > on > > > http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting > correctly? > > > > Did you set position to inherit? That is the case where this method would be > > called. > > But if the methods don't get called automatically on the descendants then won't > we have the same bug if you ask for the offset of a descendant of a sticky > element? (i.e. we won't have updated the IsInStickySubtree property to know > that). Looks like it doesn't get inherited to the descendant - see http://output.jsbin.com/gamisi/quiet
https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/27 17:20:53, flackr wrote: > On 2017/04/27 17:17:01, flackr wrote: > > On 2017/04/27 16:05:28, smcgruer wrote: > > > On 2017/04/27 15:50:02, flackr wrote: > > > > I patched this in and I only see > > > > StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and > only > > on > > > > the elements which have had position explicitly set on them (i.e. #a and > #b > > on > > > > http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting > > correctly? > > > > > > Did you set position to inherit? That is the case where this method would be > > > called. > > > > But if the methods don't get called automatically on the descendants then > won't > > we have the same bug if you ask for the offset of a descendant of a sticky > > element? (i.e. we won't have updated the IsInStickySubtree property to know > > that). > > Looks like it doesn't get inherited to the descendant - see > http://output.jsbin.com/gamisi/quiet Nevermind I was mistakenly assuming the offsetTop of the child should change, and you do have a test for the child in ElementTest.cpp. Can you add a couple tests to make sure adding and removing isInStickySubtree is updated correctly for the correct elements? https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementTest.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementTest.cpp:251: Can you test isInStickySubtree addition (which of course works given above tests) and removal (i.e. when top-level sticky element is no longer sticky)?
FYI once you are both happy with this (Chris, Rob), then I will write up a quick summary doc and add some style/animations folks to make sure they are happy with the change. https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/27 19:20:51, flackr wrote: > On 2017/04/27 17:20:53, flackr wrote: > > On 2017/04/27 17:17:01, flackr wrote: > > > On 2017/04/27 16:05:28, smcgruer wrote: > > > > On 2017/04/27 15:50:02, flackr wrote: > > > > > I patched this in and I only see > > > > > StyleBuilderFunctions::applyValueCSSPropertyPosition being called, and > > only > > > on > > > > > the elements which have had position explicitly set on them (i.e. #a and > > #b > > > on > > > > > http://output.jsbin.com/gamisi/quiet). Are you sure it's inheriting > > > correctly? > > > > > > > > Did you set position to inherit? That is the case where this method would > be > > > > called. > > > > > > But if the methods don't get called automatically on the descendants then > > won't > > > we have the same bug if you ask for the offset of a descendant of a sticky > > > element? (i.e. we won't have updated the IsInStickySubtree property to know > > > that). > > > > Looks like it doesn't get inherited to the descendant - see > > http://output.jsbin.com/gamisi/quiet > > Nevermind I was mistakenly assuming the offsetTop of the child should change, > and you do have a test for the child in ElementTest.cpp. Can you add a couple > tests to make sure adding and removing isInStickySubtree is updated correctly > for the correct elements? Done.
chrishtr@chromium.org changed reviewers: + meade@chromium.org
+meade for style changes.
Drive-by https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:972: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); Do you even need this? Your bit should essentially be "Inherited: yes" (by regular CSS spec parlance), so this bit would be updated by ComputedStyle::InheritFrom regardless. (Make test if unsure.) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); Ditto. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:657: EnsureLifecycleValidForLocationAPIs(); I think OffsetWidth/Height needs this too (because of pixelsnapping.) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); Potentially a lot of other methods needs this treatment, or? (BoundsInViewport for example, which almost identical to this method...) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:4331: void Element::EnsureLifecycleValidForLocationAPIs() { Maybe this method would be better placed in Document (close to its similar peers) and with a ...ForNode suffix (and taking a Node* of course)? https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:1144: EnsureLifecycleValidForLocationAPIs(); I think you need this in offsetWidth/Height too (because of the pixelsnapping.)
https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:987: state.Style()->SetIsInStickySubtree(position == EPosition::kSticky || Thinking about it some more, maybe this should rather go in StyleAdjuster (since that can override/change 'position', and you want to reduce the pessimism in this flag much as possible I think.)
meade@chromium.org changed reviewers: + shend@chromium.org
+shend, who's working on ComputedStyle right now fs had some good comments - I'm going to wait until you've addressed them before reviewing further.
On 2017/05/01 at 06:59:48, meade wrote: > +shend, who's working on ComputedStyle right now > > fs had some good comments - I'm going to wait until you've addressed them before reviewing further. lgtm for style/
https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:972: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/29 09:59:06, fs wrote: > Do you even need this? Your bit should essentially be "Inherited: yes" (by > regular CSS spec parlance), so this bit would be updated by > ComputedStyle::InheritFrom regardless. (Make test if unsure.) Done. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:979: state.Style()->SetIsInStickySubtree(state.ParentStyle()->IsInStickySubtree()); On 2017/04/29 09:59:06, fs wrote: > Ditto. Done. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:987: state.Style()->SetIsInStickySubtree(position == EPosition::kSticky || On 2017/04/29 12:10:31, fs wrote: > Thinking about it some more, maybe this should rather go in StyleAdjuster (since > that can override/change 'position', and you want to reduce the pessimism in > this flag much as possible I think.) Done. Based on my tests it appears to work, so I assume StyleAdjuster is called recursively if a parent style changes? https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:657: EnsureLifecycleValidForLocationAPIs(); On 2017/04/29 09:59:06, fs wrote: > I think OffsetWidth/Height needs this too (because of pixelsnapping.) Done, though I'm having trouble coming up with a unittest that shows this pixel-snapping. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); On 2017/04/29 09:59:06, fs wrote: > Potentially a lot of other methods needs this treatment, or? (BoundsInViewport > for example, which almost identical to this method...) Added to BoundsInViewport, but I suspect that getting the complete list of all API call-sites that need this will be very tricky. We don't want to fix unnecessary call-sites due to the performance hit. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:4331: void Element::EnsureLifecycleValidForLocationAPIs() { On 2017/04/29 09:59:06, fs wrote: > Maybe this method would be better placed in Document (close to its similar > peers) and with a ...ForNode suffix (and taking a Node* of course)? Done. I welcome bike-shedding on the name, it's pretty terrible :) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:1144: EnsureLifecycleValidForLocationAPIs(); On 2017/04/29 09:59:06, fs wrote: > I think you need this in offsetWidth/Height too (because of the pixelsnapping.) Again done, again I'm not sure how to best test this :)
fs@opera.com changed reviewers: + fs@opera.com
looks good to me (w/ some additional nits and poor bikeshed colors.) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:987: state.Style()->SetIsInStickySubtree(position == EPosition::kSticky || On 2017/05/01 at 20:15:15, smcgruer wrote: > On 2017/04/29 12:10:31, fs wrote: > > Thinking about it some more, maybe this should rather go in StyleAdjuster (since > > that can override/change 'position', and you want to reduce the pessimism in > > this flag much as possible I think.) > > Done. Based on my tests it appears to work, so I assume StyleAdjuster is called recursively if a parent style changes? Yes, since the flag is part of (rare) inherited, any difference to it should cause a change (to 'position' essentially) to trigger recalc for the subtree. CompustedStyle::StylePropagationDiff and NonIndependentInheritedEqual being the key ingredients I believe. (Naturally this might also be a cause of alarm/new perf differences...) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:657: EnsureLifecycleValidForLocationAPIs(); On 2017/05/01 at 20:15:15, smcgruer wrote: > On 2017/04/29 09:59:06, fs wrote: > > I think OffsetWidth/Height needs this too (because of pixelsnapping.) > > Done, though I'm having trouble coming up with a unittest that shows this pixel-snapping. Yes, effects would very likely be minor since only the fractional part of the location would come into play. Could be that fractional positioning is tricky with 'sticky' (I guess you know that part better than I), but have a look at SnapSizeToPixel to build an idea of what it'd take to get it to manifest. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); On 2017/05/01 at 20:15:15, smcgruer wrote: > On 2017/04/29 09:59:06, fs wrote: > > Potentially a lot of other methods needs this treatment, or? (BoundsInViewport > > for example, which almost identical to this method...) > > Added to BoundsInViewport, but I suspect that getting the complete list of all API call-sites that need this will be very tricky. We don't want to fix unnecessary call-sites due to the performance hit. Yes, I'm a bit worried that we're heading into a game of whack-a-mole with this... I.e it'll be non-obvious to know that you should be using this new-fangled method when you want to query a location of something (whether for something directly web-exposed or not.) (I incidently came across such an instance while looking at a bug of my own which involved SVGGraphicsElement.getScreenCTM... and how about the shadier bits of MouseEvent? [offsetX, offsetY]) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:4331: void Element::EnsureLifecycleValidForLocationAPIs() { On 2017/05/01 at 20:15:15, smcgruer wrote: > On 2017/04/29 09:59:06, fs wrote: > > Maybe this method would be better placed in Document (close to its similar > > peers) and with a ...ForNode suffix (and taking a Node* of course)? > > Done. I welcome bike-shedding on the name, it's pretty terrible :) Yes, naming... the lifecycle "duality" makes it even trickier. Part of me wants to stay close to the UpdateStyleAndLayout...-lineage, but that could mean a mile long name I suppose. I guess the most interesting part of the name is really the "Location" bit, so might want to keep that. UpdateStyle[And]LayoutAndAuxiliaryLocationDataForNode(...) =P. https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:520: style.SetIsInStickySubtree(parent_style.IsInStickySubtree() || if (style.GetPosition() == EPosition::kSticky) style.SetIsInStickySubtree(true); should be enough here (inheritance has happened already, so the parent-check is redundant.) https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:2362: if (UNLIKELY(View() && node->GetLayoutObject() && Nit: Did you see any effect of the UNLIKELY here? Considering that only the last part of the expression is "unlikely", and there's no else it seems it becomes a matter of some I$ misses (or not.) (And AFAIU we shouldn't be sprinkling UNLIKELY unless they matter - and they don't even do anything on Windows last I checked...) https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:2363: node->GetLayoutObject()->Style()->IsInStickySubtree())) { Nit: StyleRef().
https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:657: EnsureLifecycleValidForLocationAPIs(); On 2017/05/01 21:19:34, fs wrote: > On 2017/05/01 at 20:15:15, smcgruer wrote: > > On 2017/04/29 09:59:06, fs wrote: > > > I think OffsetWidth/Height needs this too (because of pixelsnapping.) > > > > Done, though I'm having trouble coming up with a unittest that shows this > pixel-snapping. > > Yes, effects would very likely be minor since only the fractional part of the > location would come into play. Could be that fractional positioning is tricky > with 'sticky' (I guess you know that part better than I), but have a look at > SnapSizeToPixel to build an idea of what it'd take to get it to manifest. After looking at this for some time, I believe that the sticky offset is only relevant to OffsetHeight for non-LayoutBox elements (e.g. inline elements). LayoutBox::PixelSnappedOffsetHeight relies on LayoutBox::OffsetHeight(), LayoutBox::Location(), and LayoutBox::ClientTop(), all of which appear to me to be unaffected by sticky/scroll. (This was surprising, but in my tests the Location is (0,100) no matter how far you scroll and where the element is sticking to). On the other hand, LayoutBoxModelObject::PixelSnappedOffsetHeight relies on LayoutBoxModelObject::OffsetTop, which can definitely change based on the sticky offset. I was able to construct a (horrendous) test case where running compositing inputs update changed the PixelSnappedOffsetHeight from 1 to 2 for an element. Someone stop me if the block elements part sounds insane, otherwise I will try and tidy up the inline test case and add that. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); On 2017/05/01 21:19:34, fs wrote: > On 2017/05/01 at 20:15:15, smcgruer wrote: > > On 2017/04/29 09:59:06, fs wrote: > > > Potentially a lot of other methods needs this treatment, or? > (BoundsInViewport > > > for example, which almost identical to this method...) > > > > Added to BoundsInViewport, but I suspect that getting the complete list of all > API call-sites that need this will be very tricky. We don't want to fix > unnecessary call-sites due to the performance hit. > > Yes, I'm a bit worried that we're heading into a game of whack-a-mole with > this... I.e it'll be non-obvious to know that you should be using this > new-fangled method when you want to query a location of something (whether for > something directly web-exposed or not.) (I incidently came across such an > instance while looking at a bug of my own which involved > SVGGraphicsElement.getScreenCTM... and how about the shadier bits of MouseEvent? > [offsetX, offsetY]) I've no experience to rely on here, so if someone feels strongly that definite correctness is more important than the potential performance hit, I can go lookup as many uses of 'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode' as I can find and update them without mercy... but my preferred approach is to say that this is 'good enough' for now ;) https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:4331: void Element::EnsureLifecycleValidForLocationAPIs() { On 2017/05/01 21:19:33, fs wrote: > On 2017/05/01 at 20:15:15, smcgruer wrote: > > On 2017/04/29 09:59:06, fs wrote: > > > Maybe this method would be better placed in Document (close to its similar > > > peers) and with a ...ForNode suffix (and taking a Node* of course)? > > > > Done. I welcome bike-shedding on the name, it's pretty terrible :) > > Yes, naming... the lifecycle "duality" makes it even trickier. Part of me wants > to stay close to the UpdateStyleAndLayout...-lineage, but that could mean a mile > long name I suppose. I guess the most interesting part of the name is really the > "Location" bit, so might want to keep that. > UpdateStyle[And]LayoutAndAuxiliaryLocationDataForNode(...) =P. Sgtm; if nobody has any objections I will update the name to that before commit. https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:520: style.SetIsInStickySubtree(parent_style.IsInStickySubtree() || On 2017/05/01 21:19:34, fs wrote: > if (style.GetPosition() == EPosition::kSticky) > style.SetIsInStickySubtree(true); > > should be enough here (inheritance has happened already, so the parent-check is > redundant.) I'm not sure I follow here. If our parent/ancestor is sticky, we may not be sticky but still want to be marked as in a sticky sub-tree, e.g.: <div id="parent" style="position: sticky"> <div id="child"></div> </div> Here, we want *both* parent and child to have IsInStickySubtree() return true, but only parent will have style.GetPosition() == EPosition::kSticky . The child would have style.GetPosition() == EPosition::kStatic afaik. https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:2362: if (UNLIKELY(View() && node->GetLayoutObject() && On 2017/05/01 21:19:34, fs wrote: > Nit: Did you see any effect of the UNLIKELY here? Considering that only the last > part of the expression is "unlikely", and there's no else it seems it becomes a > matter of some I$ misses (or not.) (And AFAIU we shouldn't be sprinkling > UNLIKELY unless they matter - and they don't even do anything on Windows last I > checked...) I did not, it was mostly speculative. Removed. https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:2363: node->GetLayoutObject()->Style()->IsInStickySubtree())) { On 2017/05/01 21:19:34, fs wrote: > Nit: StyleRef(). Done.
https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); On 2017/05/02 at 15:25:41, smcgruer wrote: > On 2017/05/01 21:19:34, fs wrote: > > On 2017/05/01 at 20:15:15, smcgruer wrote: > > > On 2017/04/29 09:59:06, fs wrote: > > > > Potentially a lot of other methods needs this treatment, or? > > (BoundsInViewport > > > > for example, which almost identical to this method...) > > > > > > Added to BoundsInViewport, but I suspect that getting the complete list of all > > API call-sites that need this will be very tricky. We don't want to fix > > unnecessary call-sites due to the performance hit. > > > > Yes, I'm a bit worried that we're heading into a game of whack-a-mole with > > this... I.e it'll be non-obvious to know that you should be using this > > new-fangled method when you want to query a location of something (whether for > > something directly web-exposed or not.) (I incidently came across such an > > instance while looking at a bug of my own which involved > > SVGGraphicsElement.getScreenCTM... and how about the shadier bits of MouseEvent? > > [offsetX, offsetY]) > > I've no experience to rely on here, so if someone feels strongly that definite correctness is more important than the potential performance hit, I can go lookup as many uses of 'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode' as I can find and update them without mercy... but my preferred approach is to say that this is 'good enough' for now ;) Well, you don't necessarily need to do any (additional) changes, but it'd probably be good to do something like that (listing in a bug or so) and then take a breif look to see if they could be relevant for updating (getScreenCTM for instance doesn't use that exact method, so wouldn't be caught by that. I'll change it to use that when I get the time though...) Every case where we're inconsistent (for no good reason, or?) can turn into a pain point for authors. (And if you're really unlucky, eventually even a compat issue... :-() https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:520: style.SetIsInStickySubtree(parent_style.IsInStickySubtree() || On 2017/05/02 at 15:25:41, smcgruer wrote: > On 2017/05/01 21:19:34, fs wrote: > > if (style.GetPosition() == EPosition::kSticky) > > style.SetIsInStickySubtree(true); > > > > should be enough here (inheritance has happened already, so the parent-check is > > redundant.) > > I'm not sure I follow here. If our parent/ancestor is sticky, we may not be sticky but still want to be marked as in a sticky sub-tree, e.g.: > > <div id="parent" style="position: sticky"> > <div id="child"></div> > </div> > > Here, we want *both* parent and child to have IsInStickySubtree() return true, but only parent will have style.GetPosition() == EPosition::kSticky . The child would have style.GetPosition() == EPosition::kStatic afaik. If parent is sticky, the inheritance mechanism will have marked the child as being in a sticky subtree already before we even get here. So here we only care about the condition that (potentially, I take it 'sticky' within 'sticky' is rare) makes the current element create a new sticky subtree - which is the '... == EPosition::kSticky' condition. I.e that covers the false => true transition, and all other cases are handled via inheritance.
https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1135: EnsureLifecycleValidForLocationAPIs(); On 2017/05/02 15:58:07, fs wrote: > On 2017/05/02 at 15:25:41, smcgruer wrote: > > On 2017/05/01 21:19:34, fs wrote: > > > On 2017/05/01 at 20:15:15, smcgruer wrote: > > > > On 2017/04/29 09:59:06, fs wrote: > > > > > Potentially a lot of other methods needs this treatment, or? > > > (BoundsInViewport > > > > > for example, which almost identical to this method...) > > > > > > > > Added to BoundsInViewport, but I suspect that getting the complete list of > all > > > API call-sites that need this will be very tricky. We don't want to fix > > > unnecessary call-sites due to the performance hit. > > > > > > Yes, I'm a bit worried that we're heading into a game of whack-a-mole with > > > this... I.e it'll be non-obvious to know that you should be using this > > > new-fangled method when you want to query a location of something (whether > for > > > something directly web-exposed or not.) (I incidently came across such an > > > instance while looking at a bug of my own which involved > > > SVGGraphicsElement.getScreenCTM... and how about the shadier bits of > MouseEvent? > > > [offsetX, offsetY]) > > > > I've no experience to rely on here, so if someone feels strongly that definite > correctness is more important than the potential performance hit, I can go > lookup as many uses of 'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode' as > I can find and update them without mercy... but my preferred approach is to say > that this is 'good enough' for now ;) > > Well, you don't necessarily need to do any (additional) changes, but it'd > probably be good to do something like that (listing in a bug or so) and then > take a breif look to see if they could be relevant for updating (getScreenCTM > for instance doesn't use that exact method, so wouldn't be caught by that. I'll > change it to use that when I get the time though...) Every case where we're > inconsistent (for no good reason, or?) can turn into a pain point for authors. > (And if you're really unlucky, eventually even a compat issue... :-() Ack, opened http://crbug.com/717601 to track this. https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2825343003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:520: style.SetIsInStickySubtree(parent_style.IsInStickySubtree() || On 2017/05/02 15:58:07, fs wrote: > On 2017/05/02 at 15:25:41, smcgruer wrote: > > On 2017/05/01 21:19:34, fs wrote: > > > if (style.GetPosition() == EPosition::kSticky) > > > style.SetIsInStickySubtree(true); > > > > > > should be enough here (inheritance has happened already, so the parent-check > is > > > redundant.) > > > > I'm not sure I follow here. If our parent/ancestor is sticky, we may not be > sticky but still want to be marked as in a sticky sub-tree, e.g.: > > > > <div id="parent" style="position: sticky"> > > <div id="child"></div> > > </div> > > > > Here, we want *both* parent and child to have IsInStickySubtree() return true, > but only parent will have style.GetPosition() == EPosition::kSticky . The child > would have style.GetPosition() == EPosition::kStatic afaik. > > If parent is sticky, the inheritance mechanism will have marked the child as > being in a sticky subtree already before we even get here. So here we only care > about the condition that (potentially, I take it 'sticky' within 'sticky' is > rare) makes the current element create a new sticky subtree - which is the '... > == EPosition::kSticky' condition. I.e that covers the false => true transition, > and all other cases are handled via inheritance. Ok, the inheritance code is definitely confirmed magic at this point ;). Thanks, removed the unnecessary part.
lgtm for core/css and core/style changes
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...
Thanks for the lgtm meade, shend. I have updated the unit-tests to be better (imo), and made a valiant attempt at re-naming. https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2825343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:4331: void Element::EnsureLifecycleValidForLocationAPIs() { On 2017/05/02 15:25:41, smcgruer wrote: > On 2017/05/01 21:19:33, fs wrote: > > On 2017/05/01 at 20:15:15, smcgruer wrote: > > > On 2017/04/29 09:59:06, fs wrote: > > > > Maybe this method would be better placed in Document (close to its similar > > > > peers) and with a ...ForNode suffix (and taking a Node* of course)? > > > > > > Done. I welcome bike-shedding on the name, it's pretty terrible :) > > > > Yes, naming... the lifecycle "duality" makes it even trickier. Part of me > wants > > to stay close to the UpdateStyleAndLayout...-lineage, but that could mean a > mile > > long name I suppose. I guess the most interesting part of the name is really > the > > "Location" bit, so might want to keep that. > > UpdateStyle[And]LayoutAndAuxiliaryLocationDataForNode(...) =P. > > Sgtm; if nobody has any objections I will update the name to that before commit. So, I realized that this CL actually replaces the specific method 'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode', and so the extended name would be 'UpdateStyleAndLayoutAndAuxiliaryLocationDataIgnorePendingStylesheetsForNode', which is **75** characters long, which seems a bit much... For now I renamed to EnsureAuxiliaryLocationDataValidForNode with a moderate sized comment in Document.h, but lmk if you prefer a different name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
https://codereview.chromium.org/2825343003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementTest.cpp (right): https://codereview.chromium.org/2825343003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementTest.cpp:216: // Now switch the parent back to being non-sticky - all descendants should be What if there is an intermediate sticky element? i.e. maybe change your test to #ancestor > #sticky > #stickyChild > #stickyGrandchild > #sticky2 > #stickyGrandgrandchild but with nicer names :-). This way you can test that #stickyGrandchild loses isInStickySubtree but #sticky2 and #stickyGrandgrandchild does not.
https://codereview.chromium.org/2825343003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementTest.cpp (right): https://codereview.chromium.org/2825343003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementTest.cpp:216: // Now switch the parent back to being non-sticky - all descendants should be On 2017/05/04 14:40:59, flackr wrote: > What if there is an intermediate sticky element? i.e. maybe change your test to > #ancestor > #sticky > #stickyChild > #stickyGrandchild > #sticky2 > > #stickyGrandgrandchild > > but with nicer names :-). This way you can test that #stickyGrandchild loses > isInStickySubtree but #sticky2 and #stickyGrandgrandchild does not. Done.
The general change LGTM, I have a suggestion around naming consistency with the method and rare data bit. https://codereview.chromium.org/2825343003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2825343003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:525: void EnsureAuxiliaryLocationDataValidForNode(const Node*); Maybe we should call this EnsurePaintLocationDataValidForNode? Since sticky is a paint effect, and the only one which affects location it is the only affect requiring cleaning of paint related inputs. I also have a preference for keeping the naming consistent. I.e. EnsurePaintLocationDataValidForNode and calling the bit location_requires_paint_clean (or something to that effect) or EnsureStickyLocationDataValidForNode and location_requires_sticky_data.
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 checked by smcgruer@chromium.org to run a CQ dry run
Ok, it looks like the comments are starting to dry up, so if nobody brings up further issues I will submit this in 48 hours (Wed morning). https://codereview.chromium.org/2825343003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2825343003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:525: void EnsureAuxiliaryLocationDataValidForNode(const Node*); On 2017/05/04 19:00:51, flackr wrote: > Maybe we should call this EnsurePaintLocationDataValidForNode? Since sticky is a > paint effect, and the only one which affects location it is the only affect > requiring cleaning of paint related inputs. > > I also have a preference for keeping the naming consistent. I.e. > EnsurePaintLocationDataValidForNode and calling the bit > location_requires_paint_clean (or something to that effect) or > EnsureStickyLocationDataValidForNode and location_requires_sticky_data. Done.
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.
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org, meade@chromium.org, flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2825343003/#ps280001 (title: "Change bit name")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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, meade@chromium.org, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2825343003/#ps300001 (title: "Rebase")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by smcgruer@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by smcgruer@chromium.org
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": 300001, "attempt_start_ts": 1494560009500660,
"parent_rev": "a90e416a49db962781bf5e3fc5cfd263346e5375", "commit_rev":
"383d1bb3f30b49fcdf3918cefc588637aa678ec0"}
Message was sent while issue was closed.
Description was changed from ========== Clean compositing inputs for location APIs for sticky-affected elements. After layout, the location of position:sticky elements and their descendants will be incorrect until compositing inputs have been cleaned. In most cases this is not an issue since Blink will run a full lifecycle after page change, including compositing. However if the user modifies layout during a script and then calls a location-based API without yielding, compositing inputs would still be dirty. This patch corrects that behavior by ensuring that compositing inputs are clean for location-related APIs. BUG=672457 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Clean compositing inputs for location APIs for sticky-affected elements. After layout, the location of position:sticky elements and their descendants will be incorrect until compositing inputs have been cleaned. In most cases this is not an issue since Blink will run a full lifecycle after page change, including compositing. However if the user modifies layout during a script and then calls a location-based API without yielding, compositing inputs would still be dirty. This patch corrects that behavior by ensuring that compositing inputs are clean for location-related APIs. BUG=672457 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2825343003 Cr-Commit-Position: refs/heads/master@{#471221} Committed: https://chromium.googlesource.com/chromium/src/+/383d1bb3f30b49fcdf3918cefc58... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/383d1bb3f30b49fcdf3918cefc58... |
