Bug 92689 - Remove overflow: scroll handling in block flow layout methods
Summary: Remove overflow: scroll handling in block flow layout methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-30 15:48 PDT by Julien Chaffraix
Modified: 2012-07-30 20:39 PDT (History)
5 users (show)

See Also:


Attachments
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code. (10.93 KB, patch)
2012-07-30 16:13 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars. (11.31 KB, patch)
2012-07-30 17:47 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (11.31 KB, patch)
2012-07-30 19:15 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-07-30 15:48:10 PDT
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.
Comment 1 Julien Chaffraix 2012-07-30 16:13:37 PDT
Created attachment 155391 [details]
Proposed refactoring: move the code to updateScrollbarsAfterStyleChange, add list box part handling and remove duplicated code.
Comment 2 Tony Chang 2012-07-30 16:30:08 PDT
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?
Comment 3 Simon Fraser (smfr) 2012-07-30 16:33:14 PDT
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.
Comment 4 Julien Chaffraix 2012-07-30 16:40:51 PDT
> 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).
Comment 5 Julien Chaffraix 2012-07-30 17:08:50 PDT
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.
Comment 6 Julien Chaffraix 2012-07-30 17:47:39 PDT
Created attachment 155409 [details]
Updated updateScrollbarsAfterStyleChange to be less confusing while keeping existing automatic scrollbars.
Comment 7 Simon Fraser (smfr) 2012-07-30 17:55:24 PDT
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().
Comment 8 Julien Chaffraix 2012-07-30 19:15:13 PDT
Created attachment 155421 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-07-30 20:39:09 PDT
Comment on attachment 155421 [details]
Patch for landing

Clearing flags on attachment: 155421

Committed r124168: <http://trac.webkit.org/changeset/124168>
Comment 10 WebKit Review Bot 2012-07-30 20:39:14 PDT
All reviewed patches have been landed.  Closing bug.