Bug 92210 - [Chromium] an RTL <select> element should have a left-hand scrollbar
Summary: [Chromium] an RTL <select> element should have a left-hand scrollbar
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-24 22:23 PDT by Hironori Bono
Modified: 2013-04-09 05:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch v0 (14.31 KB, patch)
2012-07-26 03:09 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff
Patch v0' (for getting layout-test results from EWS bots) (33.91 KB, patch)
2012-07-27 00:32 PDT, Hironori Bono
abarth: review-
Details | Formatted Diff | Diff
Patch v0'' (for investigating why my change caused an infinite loop) (380.36 KB, patch)
2012-08-23 01:12 PDT, Hironori Bono
hbono: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hironori Bono 2012-07-24 22:23:10 PDT
(Copied from <http://crbug.com/85883>.)

Chrome Version       : 12.0.742.91
URLs (if applicable) :
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 5: -
  Firefox 4.x: OK
       IE 7/8/9: OK

What steps will reproduce the problem?
1. Open the attached file with Chrome

What is the expected result?
The vertical scrollbar should be on the left side

What happens instead?
The vertical scrollbar is on the right side.

Please provide any additional information below. Attach a screenshot if possible.

This is another continuation of bug 54623, for the case of scrollbar on a multiple <select> element. Even though my r109537 moves the vertical scroll bar of the RenderLayer class to the left side, multiple <select> elements use the RenderListBox class to render their vertical scrollbars, i.e. my r109537 does not cover multiple <select> elements. To move the vertical scrollbar of a multiple <select> element, we need to change the RenderListBox class.

Regards,

Hironori Bono
Comment 1 Hironori Bono 2012-07-26 03:09:49 PDT
Created attachment 154598 [details]
Patch v0

Greetings,

I have quickly written a change that changes the RenderListBox class to move the vertical scrollbars of RTL select elements. It may be better to add a pixel test?

Regards,

Hironori Bono
Comment 2 Tony Chang 2012-07-26 11:12:24 PDT
Comment on attachment 154598 [details]
Patch v0

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

> Source/WebCore/rendering/RenderListBox.cpp:276
> +    return LayoutRect(left,
>                     additionalOffset.y() + borderTop() + paddingTop() + itemHeight() * (index - m_indexOffset),
>                     contentWidth(), itemHeight());

Nit: I think the indent is meant to line up with the opening (.
Comment 3 Tony Chang 2012-07-26 11:13:29 PDT
(In reply to comment #1)
> It may be better to add a pixel test?

If no existing pixel test covers this, then it's probably worth adding one in addition to the JS test in the patch.
Comment 4 Hironori Bono 2012-07-27 00:32:22 PDT
Created attachment 154864 [details]
Patch v0' (for getting layout-test results from EWS bots)

Greetings Tony,

Many thanks for your quick review and approval. EWS bots notice that there are a couple of pixel tests for RTL select elements (fast/text/international/bidi-listbox.html and fast/test/international/bidi-listbox-atsui.html) and I have rebaselined these test images for Windows. (I'm going to get linux results from EWS bots and re-upload a change.)

Regards,

Hironori Bono
Comment 5 WebKit Review Bot 2012-07-27 05:02:50 PDT
Comment on attachment 154598 [details]
Patch v0

Cleared Tony Chang's review+ from obsolete attachment 154598 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 6 Adam Barth 2012-08-02 13:39:13 PDT
Comment on attachment 154864 [details]
Patch v0' (for getting layout-test results from EWS bots)

This patch is spinning in the cr-linux.
Comment 7 Eric Seidel (no email) 2012-08-02 14:06:11 PDT
That's most often caused by flakiness.  Your patch likely causes some set of tests to be flaky, causing the queue to fail to get consistent results and keep trying.  Clearly a bug in the queue that this went on for days, but I think the patch will need modification to land.
Comment 8 Hironori Bono 2012-08-23 01:12:12 PDT
Created attachment 160108 [details]
Patch v0'' (for investigating why my change caused an infinite loop)

Greetings,

I would like to try my change (again) to investigate why my previous change caused an infinite loop. (This change adds rebaselined results to avoid it.)

Regards,

Hironori Bono
Comment 9 Tony Chang 2012-08-23 10:16:02 PDT
Looks like it's causing the ews bot to loop again: http://queues.webkit.org/patch/160108
Comment 10 Hironori Bono 2012-08-23 21:10:12 PDT
Greetings Tony,

Thanks for noticing it and sorry for disturbing you.
I remember my new test depends on my fix for Bug 85856 and it fails without it. (I wonder why it causes an infinite loop, though.)

Regards,

Hironori Bono

(In reply to comment #9)
> Looks like it's causing the ews bot to loop again: http://queues.webkit.org/patch/160108
Comment 11 Aharon (Vladimir) Lanin 2013-04-02 05:31:13 PDT
Any chance that work on this bug can continue?
Comment 12 Aharon (Vladimir) Lanin 2013-04-08 22:40:54 PDT
Why WONTFIX?
Comment 13 Stephen Chenney 2013-04-09 05:24:21 PDT
(In reply to comment #12)
> Why WONTFIX?

Because it's Chromium specific and there is no longer Chromium development in WebKit. Move discussion to:
https://code.google.com/p/chromium/issues/detail?id=85833