Bug 30319 – Lines on Chrome's horizontal scrollbars should be vertical

• Component: Forms
• Status: RESOLVED FIXED

[Evan Martin]

On Linux, the "grippies" on Webkit's horizontal scrollbars are oriented incorrectly.  They should be vertical lines, perpendicular to the scrollbar's direction of travel; instead, the same code is used to draw horizontal lines for both horizontal and vertical scrollbars.

[Tony Chang]

If you're feeling ambitious, you could change the painting of sliders too since it's a copy/paste of this code.

[Evan Martin]

Created attachment 41080 [details]
fix lines

[Peter Kasting]

Comment on attachment 41080 [details]
fix lines

> +        if (vertical) {
> +            drawHorizLine(canvas, midx - 2, midx + 2, midy,     paint);
> +            drawHorizLine(canvas, midx - 2, midx + 2, midy - 3, paint);
> +            drawHorizLine(canvas, midx - 2, midx + 2, midy + 3, paint);

Nit: Why not do these in (midy - 3, midy, midy + 3) order like the vertical lines?

Tony's comment about also changing the slider code seems apt here.

[Evan Martin]

I will fix the ordering.

To ease rebaselining, I'd prefer to do the slider change in a separate step.

[Evan Martin]

Created attachment 41142 [details]
fix drawing of lines

[David Levin]

Comment on attachment 41142 [details]
fix drawing of lines

r- on this patch for 3 reasons:
1. It does more than it says. It changes how the horizontal lines are drawn without explanation (nothing in the changelog and the bug only mentions the other direction).
2. It uses a lot of "magic" numbersy. It would be nice to make constants out of some of them to help inform the reader what they are.
3. It is unclear if there are any layout tests which cover this change.

[Evan Martin]

Created attachment 41145 [details]
fix drawing of lines

[Evan Martin]

Thanks for the review!

(In reply to comment #6)
> 1. It does more than it says. It changes how the horizontal lines are drawn
> without explanation (nothing in the changelog and the bug only mentions the
> other direction).

Updated the changelog.

> 2. It uses a lot of "magic" numbersy. It would be nice to make constants out of
> some of them to help inform the reader what they are.

I added some constants, but I was only adjusting the way the code was already and think they make the code less readable.

> 3. It is unclear if there are any layout tests which cover this change.

I noted in the changelog that this is covered by existing tests.

[WebKit Commit Bot]

Comment on attachment 41145 [details]
fix drawing of lines

Clearing flags on attachment: 41145

Committed r49553: <http://trac.webkit.org/changeset/49553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.