Bug 62686 – [Chromium] Switching between overlay and opaque scrollbars causes glitches

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sailesh Agrawal]

1. Run Chromium
2. Open a page with scrollbars
3. Switch between overlay and opaque scrollbars

Expected:
Scrollbars repaint and no glitches on screen

Actual:
There are lots of glitches.

[Sailesh Agrawal]

Looks like bdakin just fixed this bug for me :-)
https://trac.webkit.org/changeset/88877

[Sailesh Agrawal]

Created attachment 104116 [details]
Patch

[Sailesh Agrawal]

bdakin please take a look, thanks.

[Sailesh Agrawal]

change looks ok on try bots:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/271
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1304
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/1146

[James Robinson]

Comment on attachment 104116 [details]
Patch

Where's the test?

[Sailesh Agrawal]

(In reply to comment #5)
> (From update of attachment 104116 [details])
> Where's the test?

Hey James, I'm not sure how to test this. All this code is only used if it's run on Lion. Are any bots running Lion right now?

[Sailesh Agrawal]

Created attachment 104414 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 104116 [details] [details])
> > Where's the test?
> 
> Hey James, I'm not sure how to test this. All this code is only used if it's run on Lion. Are any bots running Lion right now?

I've filed bug 66504 to track the work to add tests for this case.

[Nico Weber]

is scrollBarStyleChanged only called on 10.7? maybe you can trigger it by adding css to the scrollbar dynamically and use that to test? If so, just do it in this bug and close the one you just filed.

[Sailesh Agrawal]

(In reply to comment #9)
> is scrollBarStyleChanged only called on 10.7? maybe you can trigger it by adding css to the scrollbar dynamically and use that to test? If so, just do it in this bug and close the one you just filed.

It's only called on 10.7. It only called by the scroll animator when we get a callback that the scrollbar has switched from overlay to opaque mode.

[Nico Weber]

Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward.

jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else?

[Beth Dakin]

(In reply to comment #11)
> Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward.
> 
> jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else?

Hi Nico! I have not been adding tests for the many scrollbar-related bugs I have fixed, which is bad. I haven't been adding them because I couldn't come up with a way to test that was not blocked by 10.7. If you have an idea of how to extend LayoutTestController without relying on 10.7 bots and it works, then adding that would be great!

[James Robinson]

There's a full complement of 10.7 bots for the apple mac port on build.webkit.org, so if you can come up with a test that works there you should be able to just add a test and put it in on the Skipped list for pre-10.7 platforms and get test coverage on the bots.

[Sailesh Agrawal]

(In reply to comment #12)
> (In reply to comment #11)
> > Indeed. I wonder if this is really blocked on 10.7 – I guess we need to add a method "scrollbarStyleChanged" method to layoutTestController to tickle this anyway, and checking for invalidation doesn't require 10.7. Hooking this up to the layoutTestController should be fairly straightforward.
> > 
> > jamesr, bdakin: Is the right thing to do here to add a method to layoutTestController, to not have a test, or something else?
> 
> Hi Nico! I have not been adding tests for the many scrollbar-related bugs I have fixed, which is bad. I haven't been adding them because I couldn't come up with a way to test that was not blocked by 10.7. If you have an idea of how to extend LayoutTestController without relying on 10.7 bots and it works, then adding that would be great!

Hey Beth, jamesr and I discussed adding a hook into the scrollbar drawing code that would let the layout test choose which type of scrollbar it wants. This means making some changes to the ScrollbarTheme code.

Are you ok with this idea?

If so I don't mind making the change and adding the tests.

Thanks,

[Beth Dakin]

(In reply to comment #14)
> Hey Beth, jamesr and I discussed adding a hook into the scrollbar drawing code that would let the layout test choose which type of scrollbar it wants. This means making some changes to the ScrollbarTheme code.
> 

I think this is a good way to do it. Thanks!

[Sailesh Agrawal]

> I think this is a good way to do it. Thanks!

Awesome. jamesr, can i have a review pleaseee

[Nico Weber]

(In reply to comment #16)
> Awesome. jamesr, can i have a review pleaseee

Usually you include tests along with the fix, now that it looks like the tests aren't blocked on getting 10.7 bots (since a) this might be doable in a bot-agnostic way b) there are 10.7 bots for the mac port)

[Sailesh Agrawal]

(In reply to comment #17)
> (In reply to comment #16)
> > Awesome. jamesr, can i have a review pleaseee
> 
> Usually you include tests along with the fix, now that it looks like the tests aren't blocked on getting 10.7 bots (since a) this might be doable in a bot-agnostic way b) there are 10.7 bots for the mac port)

Writing up the test is going to require changing a lot of the drawing code. Currently all the drawing code looks up the scrollbar style from system settings.

I think it's better to decouple that from this change.

[Sailesh Agrawal]

Ping!

[Beth Dakin]

Comment on attachment 104414 [details]
Patch

r=me!

[WebKit Review Bot]

Comment on attachment 104414 [details]
Patch

Clearing flags on attachment: 104414

Committed r94243: <http://trac.webkit.org/changeset/94243>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.