Bug 92689 – Remove overflow: scroll handling in block flow layout methods

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

The logic to set-up overflow: scroll scrollbars is implemented in layout for all RenderBlocks and its children. This was probably done to compensate for RenderLayer's lack of logic to handle that. It is now repeated in 4 classes (RenderBlock, RenderGrid, RenderFlexibleBox and RenderDeprecatedFlexibleBox).

Setting the scrollbars during layout is completely wrong as overflow: scroll only changes at style change and layout doesn't impact the presence of scrollbars in this case. On top of that, we now have the right hook in RenderLayer::updateScrollbarsAfterStyleChange to handle it.

[Julien Chaffraix]

Created attachment 155391 [details]
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code.

[Tony Chang]

I like the idea of this change.

I don't know much about RenderLayer.  Is there one per RenderBlock?  If not, don't we need to update child RenderBlocks?

Also, does updateScrollbarsAfterStyleChange run before layout?

[Simon Fraser (smfr)]

Comment on attachment 155391 [details]
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code.

View in context: https://bugs.webkit.org/attachment.cgi?id=155391&action=review

> Source/WebCore/rendering/RenderLayer.cpp:2530
> +    if (UNLIKELY(box->style()->appearance() == ListboxPart))

I don't think UNLIKELY is useful here.

> Source/WebCore/rendering/RenderLayer.cpp:4847
> +    if (UNLIKELY(box->style()->appearance() == ListboxPart))

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:4856
> +    if (!overflowCanHaveAScrollbar(overflowX))
>          setHasHorizontalScrollbar(false);
> -    if (hasVerticalScrollbar() && !overflowCanHaveAScrollbar(overflowY))
> +    else if (overflowRequiresAScrollbar(overflowX))
> +        setHasHorizontalScrollbar(true);

Having this if/else is confusing, since overflowCanHaveAScrollbar and overflowRequiresAScrollbar both consult the same EOverflow value.

[Julien Chaffraix]

> I don't know much about RenderLayer.  Is there one per RenderBlock?

Not necessarily. First it's at most one per RenderBoxModelObject and it is allocated on style change if needed. If you have some overflow clip, you will have a RenderLayer (see RenderBox::requiresLayer()).

>  If not, don't we need to update child RenderBlocks?

No need for that.

> Also, does updateScrollbarsAfterStyleChange run before layout?

updateScrollbarsAfterStyleChange is called during style change after the layer was created. If the style change required a layout, it will occur later (which should be the case for some 'overflow' transition as we need to relayout).

[Julien Chaffraix]

Comment on attachment 155391 [details]
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code.

View in context: https://bugs.webkit.org/attachment.cgi?id=155391&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:4856
>> +        setHasHorizontalScrollbar(true);
> 
> Having this if/else is confusing, since overflowCanHaveAScrollbar and overflowRequiresAScrollbar both consult the same EOverflow value.

Thinking further, we can probably get away with:

setHasHorizontalScrollbar(overflowRequiresAScrollbar(overflowX));
setHasVerticalScrollbar(overflowRequiresAScrollbar(overflowY));

This has the big downside of destroying the automatic scrollbar(s) during any style change - regardless of whether we actually changed 'overflow' and thus would likely force a relayout later. A better fix would be to be more conservative by keeping automatic scrollbars and forcing overflow: scroll ones. Let me think more about this.

[Julien Chaffraix]

Created attachment 155409 [details]
Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars.

[Simon Fraser (smfr)]

Comment on attachment 155409 [details]
Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars.

View in context: https://bugs.webkit.org/attachment.cgi?id=155409&action=review

> Source/WebCore/rendering/RenderLayer.cpp:4829
> +static bool overflowRequiresAScrollbar(EOverflow overflow)

Please call this overflowRequiresScrollbar().

[Julien Chaffraix]

Created attachment 155421 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155421 [details]
Patch for landing

Clearing flags on attachment: 155421

Committed r124168: <http://trac.webkit.org/changeset/124168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.