Bug 91949 – [chromium] Show search tickmarks on css-styled scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

[chromium] Show search tickmarks on css-styled scrollbars

[Nico Weber]

Created attachment 153708 [details]
Patch

[Nico Weber]

Note: I haven't tried this on win/linux yet, so I'm not sure the vector drawing looks right. (It looks fine if I use that code on mac though.)

[Nico Weber]

I've now tested this on linux. Looks good.

The tickmark bitmap can be removed on the chromium side once this CL has landed and rolled into chromium.

[Adrienne Walker]

Comment on attachment 153708 [details]
Patch

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

This really needs a test.  Could you add a function to window.internals to call DocumentMarkerController::addTextMatchMarker and then write a layout test that adds a few markers so we can verify that these functions do the right thing?

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:232
> -    // Inset by 2 on the left and 3 on the right.
> +    // Inset a bit.
>      tickmarkTrackRect.setX(tickmarkTrackRect.x() + 2);
>      tickmarkTrackRect.setWidth(tickmarkTrackRect.width() - 5);

This is why we can't have nice comments.  ;)

> Source/WebCore/rendering/RenderScrollbarTheme.cpp:143
> +    ScrollbarTheme::theme()->paintTickmarks(context, scrollbar, rect);

Don't render theme scrollbars all look the same on all platforms? Can you convince me further why different platforms would need different tickmark drawing code?

The ScrollbarThemeChromiumMac::paintGivenTickmarks function looks nearly identical to ScrollbarThemeChromium::paintTickmarks.  I still wonder why it should be duplicated and not just moved down to ScrollbarThemeComposite.

[Nico Weber]

Comment on attachment 153708 [details]
Patch

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

Thanks. Will add a test (which would be a pixel test, right?)

>> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:232
>>      tickmarkTrackRect.setWidth(tickmarkTrackRect.width() - 5);
> 
> This is why we can't have nice comments.  ;)

Well, the comment was correct, I just changed it because it felt too specific. (I guess I could remove it altogether.)

>> Source/WebCore/rendering/RenderScrollbarTheme.cpp:143
>> +    ScrollbarTheme::theme()->paintTickmarks(context, scrollbar, rect);
> 
> Don't render theme scrollbars all look the same on all platforms? Can you convince me further why different platforms would need different tickmark drawing code?
> 
> The ScrollbarThemeChromiumMac::paintGivenTickmarks function looks nearly identical to ScrollbarThemeChromium::paintTickmarks.  I still wonder why it should be duplicated and not just moved down to ScrollbarThemeComposite.

No. They look different on chromium/mac and chromium/elsewhere for example.

[Nico Weber]

Also, it looks like addTextMatchMarker() is called on all ports (for normal search highlights), but most ports don't want tickmarks on their scrollbars.

[Nico Weber]

Created attachment 153905 [details]
Patch

[Nico Weber]

Created attachment 153907 [details]
Patch

[Nico Weber]

Created attachment 153914 [details]
Patch

[Nico Weber]

Created attachment 153915 [details]
Patch

[Adrienne Walker]

Comment on attachment 153914 [details]
Patch

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

Is that pixel output right? It looks like the scrollbar is all the way at the bottom, but the text hasn't scrolled.

> LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:19
> +<script>

Somewhere in this script block, please add:

if (window.testRunner)
    testRunner.dumpAsText(true);

This'll make the text output just an empty line, which will signal to other developers that only the pixels matter.  See: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

> LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:24
> +    elt.clientWidth;

Are you trying to force layout here? Could you just force layout in window.internals.addTextMatchMarker if this is needed?

> LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:55
> +<p>p0 asdf asdf
> +<p>p1 asdf asdf
> +<p>p2 asdf asdf
> +<p>p3 asdf asdf
> +<p>p4 asdf asdf
> +<p>p5 asdf asdf
> +<p>p6 asdf asdf
> +<p>p7 asdf asdf
> +<p>p8 asdf asdf
> +<p>p9 asdf asdf
> +
> +
> +<p>p10 asdf asdf
> +<p>p11 asdf asdf
> +<p>p12 asdf asdf
> +<p>p13 asdf asdf
> +<p>p14 asdf asdf
> +<p>p15 asdf asdf
> +<p>p16 <span id="elt">asdf</span> asdf
> +<p>p17 asdf asdf
> +<p>p18 asdf asdf
> +<p>p19 asdf asdf

A small test nit, but can you not put text output in this test? Even with font smoothing, this'll look different on different platforms.  You could replace this with a really tall document body and an absolutely positioned div that you can select.

[Nico Weber]

(In reply to comment #11)
> (From update of attachment 153914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153914&action=review
> 
> Is that pixel output right? It looks like the scrollbar is all the way at the bottom, but the text hasn't scrolled.

The green bit is the track; the thumb is gray and is at the top.

> 
> > LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:19
> > +<script>
> 
> Somewhere in this script block, please add:
> 
> if (window.testRunner)
>     testRunner.dumpAsText(true);

Done.

> > LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:24
> > +    elt.clientWidth;
> 
> Are you trying to force layout here? Could you just force layout in window.internals.addTextMatchMarker if this is needed?

Good point. Done.

> 
> > LayoutTests/fast/scrolling/scrollbar-tickmarks-styled.html:55
> > +<p>p0 asdf asdf
> > +<p>p1 asdf asdf
> > +<p>p2 asdf asdf
> > +<p>p3 asdf asdf
> > +<p>p4 asdf asdf
> > +<p>p5 asdf asdf
> > +<p>p6 asdf asdf
> > +<p>p7 asdf asdf
> > +<p>p8 asdf asdf
> > +<p>p9 asdf asdf
> > +
> > +
> > +<p>p10 asdf asdf
> > +<p>p11 asdf asdf
> > +<p>p12 asdf asdf
> > +<p>p13 asdf asdf
> > +<p>p14 asdf asdf
> > +<p>p15 asdf asdf
> > +<p>p16 <span id="elt">asdf</span> asdf
> > +<p>p17 asdf asdf
> > +<p>p18 asdf asdf
> > +<p>p19 asdf asdf
> 
> A small test nit, but can you not put text output in this test? Even with font smoothing, this'll look different on different platforms.  You could replace this with a really tall document body and an absolutely positioned div that you can select.

Does it matter? The tickmarks are platform-specific anyhow.

[Nico Weber]

Created attachment 153921 [details]
Patch

[Adrienne Walker]

(In reply to comment #12)

> > A small test nit, but can you not put text output in this test? Even with font smoothing, this'll look different on different platforms.  You could replace this with a really tall document body and an absolutely positioned div that you can select.
> 
> Does it matter? The tickmarks are platform-specific anyhow.

Some? When possible, I'd prefer not to test more than is necessary. If font rendering changes ever-so-slightly, this test shouldn't fail.  It's not that we don't have the tools to rebaseline changes like that, but I'd prefer not to add to the problem?

[Nico Weber]

Created attachment 153927 [details]
Patch

[Nico Weber]

Done.

[Adrienne Walker]

Comment on attachment 153927 [details]
Patch

R=me.  Thanks for taking the time to add the test.

[Nico Weber]

Created attachment 153929 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153929 [details]
Patch for landing

Clearing flags on attachment: 153929

Committed r123418: <http://trac.webkit.org/changeset/123418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.