Bug 83213 – RenderLayer scrollbars' updates should be split between layout induced and style change induced

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 135682 [details]
Proposed refactoring, split the 2 concepts for better readibility.

[Simon Fraser (smfr)]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 135744 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135744 [details]
Patch for landing

Clearing flags on attachment: 135744

Committed r113289: <http://trac.webkit.org/changeset/113289>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.