Bug 30319 - Lines on Chrome's horizontal scrollbars should be vertical
Summary: Lines on Chrome's horizontal scrollbars should be vertical
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Evan Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-12 17:57 PDT by Evan Martin
Modified: 2009-10-13 20:52 PDT (History)
3 users (show)

See Also:


Attachments
fix lines (1.73 KB, patch)
2009-10-12 18:22 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
fix drawing of lines (1.74 KB, patch)
2009-10-13 17:32 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
fix drawing of lines (2.27 KB, patch)
2009-10-13 19:12 PDT, Evan Martin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Martin 2009-10-12 17:57:01 PDT
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.
Comment 1 Tony Chang 2009-10-12 18:05:32 PDT
If you're feeling ambitious, you could change the painting of sliders too since it's a copy/paste of this code.
Comment 2 Evan Martin 2009-10-12 18:22:38 PDT
Created attachment 41080 [details]
fix lines
Comment 3 Peter Kasting 2009-10-13 17:29:22 PDT
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.
Comment 4 Evan Martin 2009-10-13 17:31:19 PDT
I will fix the ordering.

To ease rebaselining, I'd prefer to do the slider change in a separate step.
Comment 5 Evan Martin 2009-10-13 17:32:34 PDT
Created attachment 41142 [details]
fix drawing of lines
Comment 6 David Levin 2009-10-13 17:52:28 PDT
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.
Comment 7 Evan Martin 2009-10-13 19:12:09 PDT
Created attachment 41145 [details]
fix drawing of lines
Comment 8 Evan Martin 2009-10-13 19:13:16 PDT
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.
Comment 9 WebKit Commit Bot 2009-10-13 20:52:12 PDT
Comment on attachment 41145 [details]
fix drawing of lines

Clearing flags on attachment: 41145

Committed r49553: <http://trac.webkit.org/changeset/49553>
Comment 10 WebKit Commit Bot 2009-10-13 20:52:16 PDT
All reviewed patches have been landed.  Closing bug.