Bug 29612

Summary:

[Chromium] the drop-down is always left-aligned even for RTL element

Product:

WebKit

Reporter:

Xiaomei Ji <xji>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aharon, commit-queue, eric, jcampan, mitz, playmobil, xji

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://www.google.com/preferences?hl=iw

Attachments:

Description	Flags
Mock-up of the way a select would appear if each item were displayed aligned according to its directionality, both in the dropdown and when chosen.	none
Mock-up of the way a select would appear if each item were displayed aligned according to select's declared directionality, both in the dropdown and when chosen.	none
patch w/ manual test	eric: review+, commit-queue: commit-queue-
patch w/ manual test	none
window's autofill	none
window's select	none
linux select	none
Linux autofill	none
mac autofill	none

Xiaomei Ji

Reported 2009-09-21 13:57:57 PDT

For both drop-down in autofill and select/options, the RTL text is still displayed as left-aligned. 1. Go to the tested URL: http://www.google.com/preferences?hl=iw 2. Open Interface Language dropdown (top of page) 3. List of languages is left-aligned instead of right-aligned. 4. Numbers in second dropdown (Number of results) are also misaligned.

Attachments
Mock-up of the way a select would appear if each item were displayed aligned according to its directionality, both in the dropdown and when chosen. (16.10 KB, image/png) 2009-09-22 06:12 PDT, Aharon (Vladimir) Lanin	no flags	Details
Mock-up of the way a select would appear if each item were displayed aligned according to select's declared directionality, both in the dropdown and when chosen. (16.07 KB, image/png) 2009-09-22 06:14 PDT, Aharon (Vladimir) Lanin	no flags	Details
patch w/ manual test (5.84 KB, patch) 2009-09-23 10:37 PDT, Xiaomei Ji	eric: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
patch w/ manual test (5.84 KB, patch) 2009-09-25 12:25 PDT, Xiaomei Ji	no flags	Details Formatted Diff Diff
window's autofill (6.13 KB, image/jpeg) 2009-09-25 12:32 PDT, Xiaomei Ji	no flags	Details
window's select (22.56 KB, image/jpeg) 2009-09-25 12:33 PDT, Xiaomei Ji	no flags	Details
linux select (79.56 KB, image/png) 2009-09-25 12:34 PDT, Xiaomei Ji	no flags	Details
Linux autofill (2.77 KB, image/png) 2009-09-25 12:35 PDT, Xiaomei Ji	no flags	Details
mac autofill (6.99 KB, image/png) 2009-09-25 12:37 PDT, Xiaomei Ji	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Xiaomei Ji

Comment 1 2009-09-21 13:58:51 PDT

also reported in Chromium: http://crbug.com/6244 http://crbug.com/9839

Xiaomei Ji

Comment 2 2009-09-21 15:21:51 PDT

Hi Aharon, We had some disussion of the drop-down items' directionality in autofill and <select> in https://bugs.webkit.org/show_bug.cgi?id=27889. And the conclusion/current status of Chromium are: 1. for autofill, the directionality of the drop-down item is the same as the <input> field. 2. for <select> (looks like all browsers do not honor the dir in <options>), the directionality of the drop-down item is determined using heuristics -- the directionality of the first strong directional character. But the directionality of the selected item is displayed using <select>'s directionalty. So, currently, Chrome might display in-consistently for the same item in drop down and in select box. See https://bugs.webkit.org/attachment.cgi?id=14607&action=view for example (select the 2nd item, and you will see the inconsistency). This issue is about another bug in Chrome that the drop-down is always left-aligned, even for a RTL element. The alignment is easy for autofill, in which, items displayed in <input> field and in drop-down are consistent, and they can be displayed as right-aligned when the element is in dir=rtl. But I need to check with you on the expected behavior of <select>/<option>. Given the following example: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" > <head> <title>Untitled Page</title> </head> <body dir="rtl"> <select> <option>abc def</option> <option>שנב גקכ</option> <option dir="rtl">abc שנב def</option> <option dir="ltr">שנב abc גקכ</option> </select> </body> </html> While the 1st and 3rd items in drop-down are displayed as LTR, and 2nd and 4th items are displayed as RTL using heuristics, what should be their alignment? should they all be displayed as right-aligned, including the 4th item? Or .... What are the rule and the rational behind the rule? Thanks, xiaomei

Aharon (Vladimir) Lanin

Comment 3 2009-09-22 06:12:52 PDT

Created attachment 39917 [details] Mock-up of the way a select would appear if each item were displayed aligned according to its directionality, both in the dropdown and when chosen.

Aharon (Vladimir) Lanin

Comment 4 2009-09-22 06:14:43 PDT

Created attachment 39918 [details] Mock-up of the way a select would appear if each item were displayed aligned according to select's declared directionality, both in the dropdown and when chosen.

Aharon (Vladimir) Lanin

Comment 5 2009-09-22 06:17:33 PDT

(In reply to comment #2) > We had some disussion of the drop-down items' directionality in autofill and > <select> in https://bugs.webkit.org/show_bug.cgi?id=27889. > > And the conclusion/current status of Chromium are: > [...] > 2. for <select> (looks like all browsers do not honor the dir in <options>), > the directionality of the drop-down item is determined using heuristics -- the > directionality of the first strong directional character. > But the directionality of the selected item is displayed using <select>'s > directionalty. > So, currently, Chrome might display in-consistently for the same item in drop > down and in select box. > See https://bugs.webkit.org/attachment.cgi?id=14607&action=view for example > (select the 2nd item, and you will see the inconsistency). The url does not work for me. As I said in https://bugs.webkit.org/show_bug.cgi?id=27889, the behavior I like for <select> is that the directionality be estimated for each option separately, with each option displayed in its own directionality both in the drop-down and when chosen. The select's directionality should only affect the position of the arrow button (and possibly the alignment of the items, in both the dropdown and the select, which is the real issue at hand). This way, there is no inconsistency between what the user sees in the dropdown and the chosen item. I do not like Chrome's current behavior for <select>, where the item's directionality is ignored when displaying it after it is chosen. > This issue is about another bug in Chrome that the drop-down is always > left-aligned, even for a RTL element. > [...] > What are the rule and the rational behind the rule? Setting alignment for each option individually has the potential advantage of greater readability, since text is usually more readable in its natural alignment. But it has the disadvantage of a potentially "jagged" display, with items appearing far away from where the user expects to see them, especially for a wide select. Furthermore, there is potential for inconsistency in alignment between an option as it appears in the dropdown and as it is appears in the select when chosen. I am attaching mock-ups for the two possibilities. There are different opinions on which way to go here. In my opinion, the jaggedness is just too much, and it is better to keep uniform alignment according to the select's dir, both in the dropdown and for the chosen item in the select.

Xiaomei Ji

Comment 6 2009-09-23 10:37:19 PDT

Created attachment 40006 [details] patch w/ manual test Jay, Could you please review the patch since you are familiar with the code? Although you are not webkit reviewer, your LGTM will make the review easy for other chromium reviewers. Thanks, Xiaomei

Eric Seidel (no email)

Comment 7 2009-09-23 17:33:05 PDT

Yes, it would be much easier for me to r+ this if someone familiar with RTL or Chromium popup code could give an LGTM, even if they aren't an official webkit reviewer.

Jeremy Moskovich

Comment 8 2009-09-24 11:16:56 PDT

Xiaomei: Thanks for taking this on! Could you please upload screenshots after applying this patch on Windows/Mac & Linux.

Jay Campan

Comment 9 2009-09-24 11:37:38 PDT

I looked at the code and LGTM.

Eric Seidel (no email)

Comment 10 2009-09-24 11:43:09 PDT

Comment on attachment 40006 [details] patch w/ manual test Code looks OK to me. Waiting to set commit-queue until after xji replies to Jeremy.

Eric Seidel (no email)

Comment 11 2009-09-24 17:24:10 PDT

Comment on attachment 40006 [details] patch w/ manual test Setting commit-queue per Jeremy's request. I believe he and xji talked offline.

WebKit Commit Bot

Comment 12 2009-09-25 08:57:56 PDT

Comment on attachment 40006 [details] patch w/ manual test Rejecting patch 40006 from commit-queue. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 Last 500 characters of output: _alignment.html M WebCore/platform/chromium/PopupMenuChromium.cpp A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output: The following files contain tab characters: trunk/WebCore/platform/chromium/PopupMenuChromium.cpp Please use spaces instead to indent. If you must commit a file with tabs, use svn propset to set the "allow-tabs" property. at /usr/local/libexec/git-core//git-svn line 469

Xiaomei Ji

Comment 13 2009-09-25 12:25:23 PDT

Created attachment 40131 [details] patch w/ manual test The only difference between this patch and the patch 40006 is removing TAB. Sorry about this, Eric. And Thanks again!

Eric Seidel (no email)

Comment 14 2009-09-25 12:26:25 PDT

Comment on attachment 40131 [details] patch w/ manual test LGTM.

Xiaomei Ji

Comment 15 2009-09-25 12:31:02 PDT

Per Jeremy's suggestion, I am attaching the snapshots of <select> and autofill in 3 platforms. 1. Windows: the patch fixes alignment problem for both <select> and autofill 2. Linux: ditto 3. Mac: the patch fixes alignment problem for autofill only. Though debugging, seems that the painting of drop-down items for <select> in Mac does not stop at PopupMenuChromium.cpp Ln 829 (PopupListBox::paintRow()), which the fix kicks in. I would like to landing the patch (after review), and we can work on <select> in Mac in another CL.

Xiaomei Ji

Comment 16 2009-09-25 12:32:35 PDT

Created attachment 40133 [details] window's autofill

Xiaomei Ji

Comment 17 2009-09-25 12:33:35 PDT

Created attachment 40134 [details] window's select

Xiaomei Ji

Comment 18 2009-09-25 12:34:55 PDT

Created attachment 40135 [details] linux select

Xiaomei Ji

Comment 19 2009-09-25 12:35:49 PDT

Created attachment 40136 [details] Linux autofill

Xiaomei Ji

Comment 20 2009-09-25 12:37:13 PDT

Created attachment 40137 [details] mac autofill

WebKit Commit Bot

Comment 21 2009-09-25 13:41:25 PDT

Comment on attachment 40131 [details] patch w/ manual test Rejecting patch 40131 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11329 test cases. fast/media/mq-transform-02.html -> failed Exiting early after 1 failures. 7358 tests run. 150.08s total testing time 7357 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output

Eric Seidel (no email)

Comment 22 2009-09-25 13:44:12 PDT

Comment on attachment 40131 [details] patch w/ manual test Sorry, this was bug 28603. I had some junk in the CommitQueue checkout causing every run to fail. :(

WebKit Commit Bot

Comment 23 2009-09-25 13:54:10 PDT

Comment on attachment 40131 [details] patch w/ manual test Rejecting patch 40131 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11334 test cases. fast/media/mq-transform-02.html -> failed Exiting early after 1 failures. 7363 tests run. 150.31s total testing time 7362 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output

Xiaomei Ji

Comment 24 2009-09-25 14:49:49 PDT

(In reply to comment #23) > (From update of attachment 40131 [details]) > Rejecting patch 40131 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', > '--quiet', '--exit-after-n-failures=1']" exit_code: 1 > Running build-dumprendertree > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Testing 11334 test cases. > fast/media/mq-transform-02.html -> failed > > Exiting early after 1 failures. 7363 tests run. > 150.31s total testing time > > 7362 test cases (99%) succeeded > 1 test case (<1%) had incorrect layout > 3 test cases (<1%) had stderr output The test failes again, and I assume that is after the cleaning up of commit-queue (comment #22). Which test is the "> 1 test case (<1%) had incorrect layout"? I did not have extra test failures in my local build. And it should not since the code change is chromium platform specific. Thanks for trying it again!

Eric Seidel (no email)

Comment 25 2009-09-25 15:14:23 PDT

Comment on attachment 40131 [details] patch w/ manual test Sorry, we were bitten by bug 29751. I'll fix the commit-queue and this will land correctly this time. Apologies again.

Eric Seidel (no email)

Comment 26 2009-09-25 15:25:12 PDT

This won't end up landing for a while anyway since the buildbots are having kittens as we speak. http://webkit-commit-queue.appspot.com/ http://build.webkit.org/waterfall

WebKit Commit Bot

Comment 27 2009-09-25 19:04:47 PDT

Comment on attachment 40131 [details] patch w/ manual test Rejecting patch 40131 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11331 test cases. http/tests/xmlhttprequest/cross-origin-authorization.html -> crashed Exiting early after 1 failures. 8966 tests run. 434.90s total testing time 8965 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output

Eric Seidel (no email)

Comment 28 2009-09-26 00:55:59 PDT

Comment on attachment 40131 [details] patch w/ manual test Sigh. Another flakey test. Bug 29322.

WebKit Commit Bot

Comment 29 2009-09-26 06:12:27 PDT

Comment on attachment 40131 [details] patch w/ manual test Clearing flags on attachment: 40131 Committed r48789: <http://trac.webkit.org/changeset/48789>

WebKit Commit Bot

Comment 30 2009-09-26 06:12:32 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.