Bug 66238 – Chromium Mac: Fix issue where scrollbar wouldn't be drawn until page finished loading

• Component: New Bugs
• Status: RESOLVED FIXED

[Sailesh Agrawal]

Chromium Mac: Force scrollbar to be drawn if user scrolls page while shouldSuspendScrollAnimations() is true

[Sailesh Agrawal]

Created attachment 103925 [details]
Patch

[Sailesh Agrawal]

The chromium bug for this is:
http://code.google.com/p/chromium/issues/detail?id=92602

[Nico Weber]

Do you know how Safari / WebKit2 handles this (or why they're not affected by it)?

[Sailesh Agrawal]

(In reply to comment #3)
> Do you know how Safari / WebKit2 handles this (or why they're not affected by it)?

The short answer is that I don't know.

As far as I can tell there are two differences between Chromium Mac and Safari:
  1. in Safari smooth scrolling is enabled so ScrollAnimatorMac::scroll() ends up calling -[NSScrollAnimationHelper scrollToPoint:]. Maybe this ends up changing the alpha some how?

  2. In Chromium Mac I added extra code to ScrollAnimatorChromiumMac::initialScrollbarPaintTimerFired() to keep the timer alive if we still hadn't loaded the page. See bug 65586. This makes it easier to reproduce the bug since we end up disabling drawing the scrollbar for longer.

So I guess another question is how Safari handles bug 65586. Theoretically any site that takes longer than 0.1 second to load should cause Safari not to flash the scrollbar on load. But this doesn't happen, Safari flashes the scrollbar correctly on all sites that I tried.

[Nico Weber]

Can you find out?

[Sailesh Agrawal]

Created attachment 103996 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #5)
> Can you find out?

Thanks for asking me to find out. After I dug deeper I found a bug in my implementation of wkScrollbarPainterForceFlashScrollers().

I have a disassembly of the GM version of all the wk* functions. I went back and verified that implementation matches the disassembly so there shouldn't be any more bugs of this sort.

[Nico Weber]

Comment on attachment 103996 [details]
Patch

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

Say "This reverts http://trac.webkit.org/changeset/92316" somewhere in the changelog.

LGTM

> Source/WebCore/ChangeLog:11
> +          3. ScrollAnimatorChromiumMac::scroll() is called before the ScrollAnimatorChromiumMac::m_initialScrollbarPaintTimer is fired.          4. At this point the scrollbar painter assumes the scrollbar is already visible (because of 1.) so the scrollbar's alpha stays at 0. Thus the scrollbar isn't visible until the page finishes loading.

Linebreak before "4."

> Source/WebCore/ChangeLog:14
> +        Also, now that wkScrollbarPainterForceFlashScrollers is working correctly I don't the extra logic I added to the initialScrollbarPainterTimer handler. That logic restarted the timer if shouldSuspendScrollAnimations() was true. But isn't necessary since calling wkScrollbarPainterForceFlashScrollers() causes -[ScrollbarPainterDelegate setUpAnimation:...] to be called which does the exact same thing.

"I don't *need*", "But *it* isn't"

[Nico Weber]

(In reply to comment #7)
> I went back and verified that implementation matches the disassembly so there shouldn't be any more bugs of this sort.

Does that mean this comment "// TODO(sail): This doesn't match the implementation in WebKitSystemInterface." can be deleted? (not in this bug)

[Sailesh Agrawal]

Comment on attachment 103996 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +          3. ScrollAnimatorChromiumMac::scroll() is called before the ScrollAnimatorChromiumMac::m_initialScrollbarPaintTimer is fired.          4. At this point the scrollbar painter assumes the scrollbar is already visible (because of 1.) so the scrollbar's alpha stays at 0. Thus the scrollbar isn't visible until the page finishes loading.
> 
> Linebreak before "4."

Fixed.

>> Source/WebCore/ChangeLog:14
>> +        Also, now that wkScrollbarPainterForceFlashScrollers is working correctly I don't the extra logic I added to the initialScrollbarPainterTimer handler. That logic restarted the timer if shouldSuspendScrollAnimations() was true. But isn't necessary since calling wkScrollbarPainterForceFlashScrollers() causes -[ScrollbarPainterDelegate setUpAnimation:...] to be called which does the exact same thing.
> 
> "I don't *need*", "But *it* isn't"

Fixed.

[Sailesh Agrawal]

Created attachment 104059 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #9)
> (In reply to comment #7)
> > I went back and verified that implementation matches the disassembly so there shouldn't be any more bugs of this sort.
> 
> Does that mean this comment "// TODO(sail): This doesn't match the implementation in WebKitSystemInterface." can be deleted? (not in this bug)

Oops, that still needs to be fixed. I'll work on that next. Filed bug 66311 to track this.

[Sailesh Agrawal]

change looks ok on try bots:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/272
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1305
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/builds/417

[Nico Weber]

Looks great. jamesr / dglazkov, can one of you r+, please?

[WebKit Review Bot]

Comment on attachment 104059 [details]
Patch

Clearing flags on attachment: 104059

Committed r93197: <http://trac.webkit.org/changeset/93197>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.