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

Side by Side 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:

OLD	NEW
1 // Copyright 2016 The Chromium Authors. All rights reserved.	1 // Copyright 2016 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "core/dom/Element.h"	5 #include "core/dom/Element.h"

6	6

7 #include "core/dom/Document.h"	7 #include "core/dom/Document.h"

8 #include "core/frame/FrameView.h"	8 #include "core/frame/FrameView.h"

9 #include "core/html/HTMLHtmlElement.h"	9 #include "core/html/HTMLHtmlElement.h"

10 #include "core/testing/DummyPageHolder.h"	10 #include "core/testing/DummyPageHolder.h"

11 #include "testing/gtest/include/gtest/gtest.h"	11 #include "testing/gtest/include/gtest/gtest.h"

12 #include <memory>	12 #include <memory>

13	13

14 namespace blink {	14 namespace blink {

15	15

16 TEST(ElementTest, SupportsFocus) {	16 TEST(ElementTest, SupportsFocus) {

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

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

19 DCHECK(isHTMLHtmlElement(document.documentElement()));	19 DCHECK(isHTMLHtmlElement(document.documentElement()));

20 document.setDesignMode("on");	20 document.setDesignMode("on");

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

22 EXPECT_TRUE(document.documentElement()->supportsFocus())	22 EXPECT_TRUE(document.documentElement()->supportsFocus())

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

24 }	24 }

25	25

	26 // Verifies that calling getBoundingClientRect cleans compositor inputs.

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

	28 // and their descendants have the correct location.

	29 TEST(ElementTest, GetBoundingClientRectUpdatesCompositorInputs) {

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

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

	32

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

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

	35

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

	37 EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

	38 document.lifecycle().state());

	39

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

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

	42 }

	43

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

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

	46 // descendants have the correct location.

	47 TEST(ElementTest, ScrollIntoViewUpdatesCompositorInputs) {

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

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

	50

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

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

	53

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

	55 EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

	56 document.lifecycle().state());

	57

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

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

	60

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

	62 EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

	63 document.lifecycle().state());

	64

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

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

	67 }

	68

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

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

	71 // and their descendants have the correct location.

	72 TEST(ElementTest, OffsetTopAndLeftUpdateCompositorInputs) {

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

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

	75

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

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

	78

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

	80 EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

	81 document.lifecycle().state());

	82

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

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

	85

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

	87 EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,

	88 document.lifecycle().state());

	89

	90 document.body()->offsetLeft();
	flackr 2017/01/23 16:43:50 It might be worth unit testing that the correct of 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); smcgruer 2017/01/24 15:26:30 Tried this, however for reasons I don't yet unders Show quoted text 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(). flackr 2017/01/24 17:00:21 I think I know what's happening. This normally get Show quoted text 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. flackr 2017/01/24 18:20:54 I guess remove the assertion that compositing inpu Show quoted text 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. smcgruer 2017/01/24 20:09:42 Added a partially-complete test to make sure that Show quoted text 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 Show quoted text 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.
	91 EXPECT_EQ(DocumentLifecycle::CompositingClean, document.lifecycle().state());

	92 }

	93

26 } // namespace blink	94 } // namespace blink

OLD	NEW

« 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 »