Skip to content

Commit

Permalink
Fully (?) fix overflow: auto with delayed scroll updates
Browse files Browse the repository at this point in the history
The scroll delaying code, used by Flexbox, never lays out the
parent after the child relayout. This is a problem especially
because the layout can add/remove a horizontal scrollbar,
which changes the height of the element. A vertical scrollbar
has this problem less often, though it sometimes does, especially
in case of flex items. As far as I can tell, this never worked
correctly since the scroll delaying code existed.

This patch changes it so that when we eventually do the scroll info
updates, we mark the ancestor chain for layout (up to the outer flexbox)
and then call layoutFlexItems again, to do the correct layout.

See also the discussion thread at
https://groups.google.com/a/chromium.org/d/topic/layout-dev/e7Wzvxu7qiA/discussion

BUG=579401,584363

Review URL: https://codereview.chromium.org/1734203002

Cr-Commit-Position: refs/heads/master@{#377826}
  • Loading branch information
cbiesinger authored and Commit bot committed Feb 26, 2016
1 parent c249547 commit 775378f
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ PASS hbox.parentNode.clientHeight is hbox.parentNode.scrollHeight
PASS hbox.clientHeight is hbox.scrollHeight
PASS intrinsicHeightBox.clientHeight is intrinsicHeightBox.scrollHeight
PASS scrollbarSize is not 0
PASS inner.clientWidth is 100 - scrollbarSize
PASS inner.offsetWidth is 100
PASS inner.clientWidth is 100
PASS inner.offsetWidth is 100 + scrollbarSize
PASS vbox.clientWidth is 100
PASS vbox.offsetWidth is 100 + scrollbarSize
PASS successfullyParsed is true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
}
.vbox {
overflow-y: auto;
max-height: 200px;
}
.intrinsic-height-box {
min-height: -webkit-min-content;
Expand Down Expand Up @@ -76,11 +77,9 @@
var scrollbarSize = measure.offsetWidth - measure.clientWidth;
shouldNotBe("scrollbarSize", "0");

// TODO(cbiesinger): These should really be 100 and 100 + scrollbarSize, but
// we currently have a bug that makes it not so.
var inner = document.getElementById('inner');
shouldBe("inner.clientWidth", "100 - scrollbarSize");
shouldBe("inner.offsetWidth", "100");
shouldBe("inner.clientWidth", "100");
shouldBe("inner.offsetWidth", "100 + scrollbarSize");

var vbox = document.querySelector('.vbox');
shouldBe("vbox.clientWidth", "100");
Expand Down
7 changes: 5 additions & 2 deletions third_party/WebKit/Source/core/layout/LayoutBlock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -837,8 +837,10 @@ void LayoutBlock::startDelayUpdateScrollInfo()
++gDelayUpdateScrollInfo;
}

void LayoutBlock::finishDelayUpdateScrollInfo()
bool LayoutBlock::finishDelayUpdateScrollInfo(SubtreeLayoutScope* layoutScope)
{
bool childrenMarkedForRelayout = false;

--gDelayUpdateScrollInfo;
ASSERT(gDelayUpdateScrollInfo >= 0);
if (gDelayUpdateScrollInfo == 0) {
Expand All @@ -849,10 +851,11 @@ void LayoutBlock::finishDelayUpdateScrollInfo()

for (auto* block : *infoSet) {
if (block->hasOverflowClip()) {
block->layer()->scrollableArea()->updateAfterLayout();
childrenMarkedForRelayout |= block->layer()->scrollableArea()->updateAfterLayout(layoutScope);
}
}
}
return childrenMarkedForRelayout;
}

void LayoutBlock::updateScrollInfoAfterLayout()
Expand Down
3 changes: 2 additions & 1 deletion third_party/WebKit/Source/core/layout/LayoutBlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,9 @@ class CORE_EXPORT LayoutBlock : public LayoutBox {
// descendant. If multiple calls are made to startDelayUpdateScrollInfo(),
// finishDelayUpdateScrollInfo() will do nothing until finishDelayUpdateScrollInfo()
// is called the same number of times.
// finishDelayUpdateScrollInfo returns true when it marked something for layout.
static void startDelayUpdateScrollInfo();
static void finishDelayUpdateScrollInfo();
static bool finishDelayUpdateScrollInfo(SubtreeLayoutScope*);

void updateScrollInfoAfterLayout();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ void LayoutDeprecatedFlexibleBox::layoutHorizontalBox(bool relayoutChildren)
}
} while (haveFlex);

LayoutBlock::finishDelayUpdateScrollInfo();
LayoutBlock::finishDelayUpdateScrollInfo(nullptr);

if (remainingSpace > 0 && ((style()->isLeftToRightDirection() && style()->boxPack() != Start)
|| (!style()->isLeftToRightDirection() && style()->boxPack() != End))) {
Expand Down Expand Up @@ -783,7 +783,7 @@ void LayoutDeprecatedFlexibleBox::layoutVerticalBox(bool relayoutChildren)
}
} while (haveFlex);

LayoutBlock::finishDelayUpdateScrollInfo();
LayoutBlock::finishDelayUpdateScrollInfo(nullptr);

if (style()->boxPack() != Start && remainingSpace > 0) {
// Children must be repositioned.
Expand Down
3 changes: 2 additions & 1 deletion third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,8 @@ void LayoutFlexibleBox::layoutBlock(bool relayoutChildren)

layoutFlexItems(relayoutChildren, layoutScope);

LayoutBlock::finishDelayUpdateScrollInfo();
if (LayoutBlock::finishDelayUpdateScrollInfo(&layoutScope))
layoutFlexItems(false, layoutScope);

if (logicalHeight() != previousHeight)
relayoutChildren = true;
Expand Down
25 changes: 17 additions & 8 deletions third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -612,10 +612,12 @@ void PaintLayerScrollableArea::scrollToPosition(const DoublePoint& scrollPositio
ScrollableArea::setScrollPosition(newScrollPosition, scrollType, scrollBehavior);
}

void PaintLayerScrollableArea::updateAfterLayout()
bool PaintLayerScrollableArea::updateAfterLayout(SubtreeLayoutScope* delayedLayoutScope)
{
ASSERT(box().hasOverflowClip());

bool didMarkForDelayedLayout = false;

if (needsScrollbarReconstruction()) {
m_scrollbarManager.setCanDetachScrollbars(false);
setHasHorizontalScrollbar(false);
Expand Down Expand Up @@ -683,14 +685,19 @@ void PaintLayerScrollableArea::updateAfterLayout()
if ((horizontalScrollBarChanged && box().style()->overflowX() != OOVERLAY) || (verticalScrollBarChanged && box().style()->overflowY() != OOVERLAY)) {
if (!m_inOverflowRelayout) {
m_inOverflowRelayout = true;
SubtreeLayoutScope layoutScope(box());
layoutScope.setNeedsLayout(&box(), LayoutInvalidationReason::ScrollbarChanged);
if (box().isLayoutBlock()) {
LayoutBlock& block = toLayoutBlock(box());
block.scrollbarsChanged(horizontalScrollBarChanged, verticalScrollBarChanged);
block.layoutBlock(true);
if (delayedLayoutScope) {
delayedLayoutScope->setNeedsLayout(&box(), LayoutInvalidationReason::ScrollbarChanged);
didMarkForDelayedLayout = true;
} else {
box().layout();
SubtreeLayoutScope layoutScope(box());
layoutScope.setNeedsLayout(&box(), LayoutInvalidationReason::ScrollbarChanged);
if (box().isLayoutBlock()) {
LayoutBlock& block = toLayoutBlock(box());
block.scrollbarsChanged(horizontalScrollBarChanged, verticalScrollBarChanged);
block.layoutBlock(true);
} else {
box().layout();
}
}
LayoutObject* parent = box().parent();
if (parent && parent->isFlexibleBox())
Expand Down Expand Up @@ -727,6 +734,8 @@ void PaintLayerScrollableArea::updateAfterLayout()

DisableCompositingQueryAsserts disabler;
positionOverflowControls();

return didMarkForDelayedLayout;
}

ScrollBehavior PaintLayerScrollableArea::scrollBehaviorStyle() const
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,9 @@ class CORE_EXPORT PaintLayerScrollableArea final : public NoBaseWillBeGarbageCol
scrollToOffset(toDoubleSize(position), ScrollOffsetClamped, scrollBehavior, scrollType);
}

void updateAfterLayout();
// Returns true if a layout object was marked for layout. In such a case, the layout scope's root
// should be laid out again.
bool updateAfterLayout(SubtreeLayoutScope* = nullptr);
void updateAfterStyleChange(const ComputedStyle*);
void updateAfterOverflowRecalc();

Expand Down

0 comments on commit 775378f

Please sign in to comment.