Bug 83213

Summary:

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

Product:

WebKit

Reporter:

Julien Chaffraix <jchaffraix>

Component:

Layout and Rendering

Assignee:

Julien Chaffraix <jchaffraix>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

jamesr, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Proposed refactoring, split the 2 concepts for better readibility.	none
Patch for landing	none

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.