third_party/WebKit/Source/core/dom/ElementTest.cpp - Issue 2647533002: Force compositing inputs to be clean for getBoundingClientRect

Unified Diff: third_party/WebKit/Source/core/dom/ElementTest.cpp

Issue 2647533002: Force compositing inputs to be clean for getBoundingClientRect (Closed)

Patch Set: Add LayoutTests to validate post-layout behavior Created 3 years, 11 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: third_party/WebKit/Source/core/dom/ElementTest.cpp

diff --git a/third_party/WebKit/Source/core/dom/ElementTest.cpp b/third_party/WebKit/Source/core/dom/ElementTest.cpp

index 30064c67ba3ee38b3cfacc842460acaf28119ddd..d1c2f6aac314ecc356d9d84de3ac6d082fe462c4 100644

--- a/third_party/WebKit/Source/core/dom/ElementTest.cpp

+++ b/third_party/WebKit/Source/core/dom/ElementTest.cpp

@@ -23,4 +23,72 @@ TEST(ElementTest, SupportsFocus) {

<< "<html> with designMode=on should be focusable.";

}

+// Verifies that calling getBoundingClientRect cleans compositor inputs.

+// Cleaning the compositor inputs is required so that position:sticky elements

+// and their descendants have the correct location.

+TEST(ElementTest, GetBoundingClientRectUpdatesCompositorInputs) {

+ std::unique_ptr<DummyPageHolder> pageHolder = DummyPageHolder::create();

+ Document& document = pageHolder->document();

+ document.view()->updateAllLifecyclePhases();

+ EXPECT_EQ(DocumentLifecycle::PaintClean, document.lifecycle().state());

+ document.body()->setInnerHTML("<div id='test'></div>");

+ EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

+ document.lifecycle().state());

+ document.body()->getBoundingClientRect();

+ EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

+// Verifies that calling scrollIntoView* cleans compositor inputs. Cleaning the

+// compositor inputs is required so that position:sticky elements and their

+// descendants have the correct location.

+TEST(ElementTest, ScrollIntoViewUpdatesCompositorInputs) {

+ std::unique_ptr<DummyPageHolder> pageHolder = DummyPageHolder::create();

+ Document& document = pageHolder->document();

+ document.view()->updateAllLifecyclePhases();

+ EXPECT_EQ(DocumentLifecycle::PaintClean, document.lifecycle().state());

+ document.body()->setInnerHTML("<div id='test'></div>");

+ EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

+ document.lifecycle().state());

+ document.body()->scrollIntoView();

+ EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

+ document.body()->setInnerHTML("<div id='test2'></div>");

+ EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

+ document.lifecycle().state());

+ document.body()->scrollIntoViewIfNeeded();

+ EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

+// Verifies that calling offsetTop/offsetLeft cleans compositor inputs.

+// Cleaning the compositor inputs is required so that position:sticky elements

+// and their descendants have the correct location.

+TEST(ElementTest, OffsetTopAndLeftUpdateCompositorInputs) {

+ std::unique_ptr<DummyPageHolder> pageHolder = DummyPageHolder::create();

+ Document& document = pageHolder->document();

+ document.view()->updateAllLifecyclePhases();

+ EXPECT_EQ(DocumentLifecycle::PaintClean, document.lifecycle().state());

+ document.body()->setInnerHTML("<div id='test'></div>");

+ EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

+ document.lifecycle().state());

+ document.body()->offsetTop();

+ EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

+ document.body()->setInnerHTML("<div id='test2'></div>");

+ EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

+ document.lifecycle().state());

+ document.body()->offsetLeft();

flackr 2017/01/23 16:43:50 It might be worth unit testing that the correct of

smcgruer 2017/01/24 15:26:30 Tried this, however for reasons I don't yet unders

flackr 2017/01/24 17:00:21 I think I know what's happening. This normally get

flackr 2017/01/24 18:20:54 I guess remove the assertion that compositing inpu

smcgruer 2017/01/24 20:09:42 Added a partially-complete test to make sure that

On 2017/01/24 18:20:54, flackr wrote: > On 2017/01/24 17:00:21, flackr wrote: > > On 2017/01/24 15:26:30, smcgruer wrote: > > > On 2017/01/23 16:43:50, flackr wrote: > > > > It might be worth unit testing that the correct offset is returned here, > > maybe > > > > after expecting CompositingClean, assert that the sticky > > LayoutBoxModelObject > > > no > > > > longer needs compositing inputs update and that the original offset > matches > > > the > > > > new offset? i.e. > > > > > > > > // Modify DOM, expect compositing inputs to be dirty. > > > > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate()); > > > > offset = Element->offsetLeft(); > > > > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate()); > > > > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...) > > > > EXPECT_EQ(Element->offsetLeft(), offset); > > > > > > Tried this, however for reasons I don't yet understand, the following > failed: > > > > > > padding->setInnerHTML("<div id='test'></div>"); > > > EXPECT_EQ(DocumentLifecycle::VisualUpdatePending, > > > document.lifecycle().state()); > > > > > > > > > > > > EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate()); > > > <-- this is false, not true. > > > int offsetTop = sticky->offsetTop(); > > > > > > Not sure why, but setInnerHTML (on anything, I tried it on the sticky > element > > as > > > well as the padding) doesn't cause anything to flip the LMBO to > > > needsCompositingInputsUpdate(). > > > > I think I know what's happening. This normally gets set when the layout within > > the scrollable area changes by > > PaintLayerScrollableArea::invalidateAllStickyConstraints: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... > > > > But layout is probably still dirty at this point. > > I guess remove the assertion that compositing inputs are dirty before the change > and just assert afterwards that inputs are clean + a position dependent on the > layout change.

Added a partially-complete test to make sure that I understand what you're looking for. Please ack the test and let me know if its not what you were thinking of.

flackr 2017/01/24 20:22:28 Acknowledged. That's exactly what I was suggesting

On 2017/01/24 20:09:42, smcgruer wrote: > On 2017/01/24 18:20:54, flackr wrote: > > On 2017/01/24 17:00:21, flackr wrote: > > > On 2017/01/24 15:26:30, smcgruer wrote: > > > > On 2017/01/23 16:43:50, flackr wrote: > > > > > It might be worth unit testing that the correct offset is returned here, > > > maybe > > > > > after expecting CompositingClean, assert that the sticky > > > LayoutBoxModelObject > > > > no > > > > > longer needs compositing inputs update and that the original offset > > matches > > > > the > > > > > new offset? i.e. > > > > > > > > > > // Modify DOM, expect compositing inputs to be dirty. > > > > > EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate()); > > > > > offset = Element->offsetLeft(); > > > > > EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate()); > > > > > EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...) > > > > > EXPECT_EQ(Element->offsetLeft(), offset); > > > > > > > > Tried this, however for reasons I don't yet understand, the following > > failed: > > > > > > > > padding->setInnerHTML("<div id='test'></div>"); > > > > EXPECT_EQ(DocumentLifecycle::VisualUpdatePending, > > > > document.lifecycle().state()); > > > > > > > > > > > > > > > > > > EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate()); > > > > <-- this is false, not true. > > > > int offsetTop = sticky->offsetTop(); > > > > > > > > Not sure why, but setInnerHTML (on anything, I tried it on the sticky > > element > > > as > > > > well as the padding) doesn't cause anything to flip the LMBO to > > > > needsCompositingInputsUpdate(). > > > > > > I think I know what's happening. This normally gets set when the layout > within > > > the scrollable area changes by > > > PaintLayerScrollableArea::invalidateAllStickyConstraints: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... > > > > > > But layout is probably still dirty at this point. > > > > I guess remove the assertion that compositing inputs are dirty before the > change > > and just assert afterwards that inputs are clean + a position dependent on the > > layout change. > > Added a partially-complete test to make sure that I understand what you're > looking for. Please ack the test and let me know if its not what you were > thinking of.

Acknowledged. That's exactly what I was suggesting.

+ EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

} // namespace blink

« third_party/WebKit/Source/core/dom/Element.cpp ('K') | « third_party/WebKit/Source/core/dom/Element.cpp ('k') | no next file » | no next file with comments »