Bug 29612

Summary: [Chromium] the drop-down is always left-aligned even for RTL element
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, commit-queue, eric, jcampan, mitz, playmobil, xji
Priority: P2    
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

Description Xiaomei Ji 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.
Comment 1 Xiaomei Ji 2009-09-21 13:58:51 PDT
also reported in Chromium:
http://crbug.com/6244
http://crbug.com/9839
Comment 2 Xiaomei Ji 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
Comment 3 Aharon (Vladimir) Lanin 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.
Comment 4 Aharon (Vladimir) Lanin 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.
Comment 5 Aharon (Vladimir) Lanin 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.
Comment 6 Xiaomei Ji 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
Comment 7 Eric Seidel 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.
Comment 8 Jeremy Moskovich 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.
Comment 9 Jay Campan 2009-09-24 11:37:38 PDT
I looked at the code and LGTM.
Comment 10 Eric Seidel 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.
Comment 11 Eric Seidel 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.
Comment 12 WebKit Commit Bot 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
Comment 13 Xiaomei Ji 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!
Comment 14 Eric Seidel 2009-09-25 12:26:25 PDT
Comment on attachment 40131 [details]
patch w/ manual test

LGTM.
Comment 15 Xiaomei Ji 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.
Comment 16 Xiaomei Ji 2009-09-25 12:32:35 PDT
Created attachment 40133 [details]
window's autofill
Comment 17 Xiaomei Ji 2009-09-25 12:33:35 PDT
Created attachment 40134 [details]
window's select
Comment 18 Xiaomei Ji 2009-09-25 12:34:55 PDT
Created attachment 40135 [details]
linux select
Comment 19 Xiaomei Ji 2009-09-25 12:35:49 PDT
Created attachment 40136 [details]
Linux autofill
Comment 20 Xiaomei Ji 2009-09-25 12:37:13 PDT
Created attachment 40137 [details]
mac autofill
Comment 21 WebKit Commit Bot 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
Comment 22 Eric Seidel 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. :(
Comment 23 WebKit Commit Bot 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
Comment 24 Xiaomei Ji 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!
Comment 25 Eric Seidel 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.
Comment 26 Eric Seidel 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
Comment 27 WebKit Commit Bot 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
Comment 28 Eric Seidel 2009-09-26 00:55:59 PDT
Comment on attachment 40131 [details]
patch w/ manual test

Sigh.  Another flakey test.  Bug 29322.
Comment 29 WebKit Commit Bot 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>
Comment 30 WebKit Commit Bot 2009-09-26 06:12:32 PDT
All reviewed patches have been landed.  Closing bug.