Bug 119454 – Next layout after frameview load is completed should update scrollbars if necessary

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Roger Fong]

Sometimes a page may take longer to load than expected.
If the page changes the body::-webkit-scrollbar css, those changes won't take affect because we don't call updateScrollbars when we need to.
Right now we do it on the very first layout, which won't necessarily wait to happen until the page has finished loading. 
It also happens when resizing the view.

I think we have two options here:
1. in FrameView::handleLoadComplete, call resetScrollbars so that we update the scroll bars when the page is done loading
2. ensure that the first layout doesn't occur until handleLoadComplete has been called, then allow/force the first layout.
        - not sure about this one because I don't really have a good grasp of all the different situations in which we decide to attempt a layout call

The patch I'm loading is for 1)

[Roger Fong]

<rdar://problem/12363851>

[Roger Fong]

Created attachment 208038 [details]
patch

[Roger Fong]

Created attachment 208074 [details]
Patch

[Tim Horton]

Comment on attachment 208074 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2279
> +    if (!m_firstLayout) {

This seems… quite surprising.

Also its location relative to the comment is not right.

[kov's GTK+ EWS bot]

Comment on attachment 208074 [details]
Patch

Attachment 208074 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1326729

[Sam Weinig]

Comment on attachment 208074 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        When the page finishes loading, schedule another layout in which scrollbars will be updated.
> +        https://bugs.webkit.org/show_bug.cgi?id=119454.

This describes what the patch does, not why it necessary.  What is the issue that prompted making this change? Also, why can't it be tested?

[Simon Fraser (smfr)]

Comment on attachment 208074 [details]
Patch

Causing an extra layout is a big deal, and should be avoided. The real question is why any of this is apparently necessary on Windows, but not on Mac.

[Roger Fong]

(In reply to comment #7)
> (From update of attachment 208074 [details])
> Causing an extra layout is a big deal, and should be avoided. The real question is why any of this is apparently necessary on Windows, but not on Mac.

This problem happens on mac as well.
I have a simple test case that dynamically changes the scroll bar css on a timer.
(I originally thought dynamically changing the css was a different issue because I couldn't get the scroll bars to update at all with this test case - turns out I was resizing the window horizontally, which does not cause any sort of update to happen, resizing vertically does though).

Anyways, point is, this is a problem on both mac and windows, that changing the CSS dynamically does not result in the scrollbars being updated.

[Roger Fong]

Given this, FrameView::handleLoadCompleted is definitely not the right place for the fix. 
It should be somewhere related to the scrollbar css being changed.

[Roger Fong]

So, regardless of what happens, the fact remains that we are not updating the scroll bars somewhere where we should be.

This is what happens in the test case:
recalcStyle gets called when the scrollbar css is changed.
This results eventually in a layout timer getting fired.

There are two places in FrameView::layout where the scroll bars could possibly be updated.

One only happens if it's the first layout or if we're changing scroll modes.
The other only happens when we are resizing the view, which bails early if the view size doesn't actually change.

I'm not sure if it's better to add the css changing case to one or the other, or in a separate case all on it's own.
Perhaps making another case for "CSS being changed after first layout" would be best since the other code paths seem to contain logic for things that we don't need (we need only to call updateScrollBars in the CSS case)

[Roger Fong]

(In reply to comment #10)
> So, regardless of what happens, the fact remains that we are not updating the scroll bars somewhere where we should be.

Not sure what I meant by "regardless of what happens" -> internal dialogue filler perhaps.

[Roger Fong]

Created attachment 208456 [details]
patch

Note this is a workaround:
In this patch we update the scrollbars after the frameview has finished loading and only if there is a mismatch between the style of the scrollbar and the actual scrollbar (whether or not they're custom)

An enhanced workedaround was proposed that I could in the FrameView::layout method just check to see if the style and actual scrollbar matched and then update scrollbars as necessary then. That would not only solve this particularly delayed loading problem but also the problem of scrollbars not updating after dynamically changing the scrollbar css.

However, doing this causes a lot of unforeseen issues with the ordering during which layouts are called that seem to result in scrollbar updates being disabled altogether, which ultimately causes the document to adopt the default scrollbar modes (vertical: ALWAYS ON and horizontal: ALWAYS off, though the scrollbars are in the correct style).

Given that this patch is simply a temporary work around (and thus will be removed soon anyways), I think we should focus on just fixing the bug at hand and not dealing with the dynamic css changing issue for now.

[Beth Dakin]

Comment on attachment 208456 [details]
patch

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

r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close!

> Source/WebCore/page/FrameView.cpp:1295
> +                    

You should remove this new whitespace.

[Roger Fong]

(In reply to comment #13)
> (From update of attachment 208456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208456&action=review
> 
> r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close!

Hmm the constructor calls init() which calls reset() which initializes m_scrollbarUpdatePending.
Is that okay?

> 
> > Source/WebCore/page/FrameView.cpp:1295
> > +                    
> 
> You should remove this new whitespace.

Will do

[Beth Dakin]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 208456 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208456&action=review
> > 
> > r- because you are not initializing m_scrollbarUpdatePending in the FrameView constructor, but I think this is okay otherwise. So this is close!
> 
> Hmm the constructor calls init() which calls reset() which initializes m_scrollbarUpdatePending.
> Is that okay?
> 

That's a good point, but I don't think that there is any need to rely on that. That code could change. We should initialize this bool in the constructor just like we do for all of the other bools.

[Roger Fong]

Created attachment 208464 [details]
patch

[Roger Fong]

(In reply to comment #16)
> Created an attachment (id=208464) [details]
> patch

Fixed review comments. Thanks!

[Early Warning System Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1385421

[Early Warning System Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1350580

[EFL EWS Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1405345

[Build Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1353612

[Beth Dakin]

Comment on attachment 208464 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:207
> +    , m_scrollbarUpdatePending(true)

This should be initialized to false.

[Beth Dakin]

Comment on attachment 208464 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:207
> +    , m_scrollbarUpdatePending(true)

This should be initialized to false.

[EFL EWS Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1401356

[Build Bot]

Comment on attachment 208464 [details]
patch

Attachment 208464 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1369847

[Beth Dakin]

Comment on attachment 208464 [details]
patch

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

>>> Source/WebCore/page/FrameView.cpp:207
>>> +    , m_scrollbarUpdatePending(true)
>> 
>> This should be initialized to false.
> 
> This should be initialized to false.

Oh, also, it looks like the builds are failing because you are initializing this in the wrong spot. You need to initialize it in the same order that it is declared in the header. So it should be right after m_firstLayoutCallbackPending and right before m_firstLayout based on where you put it in the header.

[Darin Adler]

Comment on attachment 208464 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:2287
> +    Document* doc = m_frame->document();

Please don’t abbreviate to “doc” in new code. Use “document” instead.

> Source/WebCore/page/FrameView.cpp:2288
> +    Element* body = doc ? doc->body() : 0;

Document can’t be 0.

> Source/WebCore/page/FrameView.cpp:2292
> +    Element* docElement = doc ? doc->documentElement() : 0;

Same here, documentElement is better. And document can’t be zero.