Bug 83213 - RenderLayer scrollbars' updates should be split between layout induced and style change induced
Summary: RenderLayer scrollbars' updates should be split between layout induced and st...
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-04-04 14:02 PDT by Julien Chaffraix
Modified: 2012-04-04 21:30 PDT (History)
3 users (show)

See Also:


Attachments
Proposed refactoring, split the 2 concepts for better readibility. (14.33 KB, patch)
2012-04-04 14:24 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (14.14 KB, patch)
2012-04-04 19:08 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-04-04 14:02:08 PDT
The current code in RenderLayer::updateScrollInfoAfterLayout handles the 2 types of scrollbars' changes which is wrong. Several of the updates are only tied to style changes and should be done in RenderLayer::styleChanged().

This makes the code confusing to follow and adds more code to updateScrollInfoAfterLayout than necessary.

Patch forthcoming.
Comment 1 Julien Chaffraix 2012-04-04 14:24:04 PDT
Created attachment 135682 [details]
Proposed refactoring, split the 2 concepts for better readibility.
Comment 2 Simon Fraser (smfr) 2012-04-04 16:34:41 PDT
Comment on attachment 135682 [details]
Proposed refactoring, split the 2 concepts for better readibility.

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

> Source/WebCore/rendering/RenderLayer.cpp:2418
> +    // FIXME: This works only for overflow: scroll as it removes the scrollbar from the available area.

I don't find this comment particularly clear.

> Source/WebCore/rendering/RenderLayer.cpp:2507
> +        // Layout may cause us to be in an invalid scroll position. In this case we need

I'd say "at an invalid scroll position"

> Source/WebCore/rendering/RenderLayer.cpp:4626
> +    if (m_hBar && !overflowCanHaveAScrollbar(overflowX))

Shouldn't you use the hasVerticalScrollbar() method here?

> Source/WebCore/rendering/RenderLayer.cpp:4632
> +    // Overflow: scroll only enables / disables scrollbars. Changing overflow: scroll
> +    // to overflow: auto should re-enable the scrollbars. See bug 11985.

This comment is a bit confusing too. I think you mean that with overflow:scroll, scrollbars are always visible but might be disabled.
Comment 3 Julien Chaffraix 2012-04-04 18:12:21 PDT
Comment on attachment 135682 [details]
Proposed refactoring, split the 2 concepts for better readibility.

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

>> Source/WebCore/rendering/RenderLayer.cpp:2418
>> +    // FIXME: This works only for overflow: scroll as it removes the scrollbar from the available area.
> 
> I don't find this comment particularly clear.

I will just remove it as it is not a super important comment. Overflow: scroll use this function to enable / disable scrollbars so it needs to remove the scrollbar from the available height (thus the use of client height). In other context, removing the scrollbar's size is not advisable and the cause of bug 71541 AFAICT.

>> Source/WebCore/rendering/RenderLayer.cpp:2507
>> +        // Layout may cause us to be in an invalid scroll position. In this case we need
> 
> I'd say "at an invalid scroll position"

Sure.

>> Source/WebCore/rendering/RenderLayer.cpp:4632
>> +    // to overflow: auto should re-enable the scrollbars. See bug 11985.
> 
> This comment is a bit confusing too. I think you mean that with overflow:scroll, scrollbars are always visible but might be disabled.

Good call, let me rephrase that.
Comment 4 Julien Chaffraix 2012-04-04 19:08:16 PDT
Created attachment 135744 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-04-04 21:30:42 PDT
Comment on attachment 135744 [details]
Patch for landing

Clearing flags on attachment: 135744

Committed r113289: <http://trac.webkit.org/changeset/113289>
Comment 6 WebKit Review Bot 2012-04-04 21:30:46 PDT
All reviewed patches have been landed.  Closing bug.