Bug 29612

Summary: [Chromium] the drop-down is always left-aligned even for RTL element
Product: WebKit Reporter: Xiaomei Ji <xji@chromium.org>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon@google.com, commit-queue@webkit.org, eric@webkit.org, jcampan@google.com, mitz@webkit.org, playmobil@google.com, xji@chromium.org
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 From 2009-09-21 13:57:57 PST
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 From 2009-09-21 13:58:51 PST -------
also reported in Chromium:
http://crbug.com/6244
http://crbug.com/9839
------- Comment #2 From 2009-09-21 15:21:51 PST -------
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 From 2009-09-22 06:12:52 PST -------
Created an attachment (id=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 From 2009-09-22 06:14:43 PST -------
Created an attachment (id=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 From 2009-09-22 06:17:33 PST -------
(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 From 2009-09-23 10:37:19 PST -------
Created an attachment (id=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 From 2009-09-23 17:33:05 PST -------
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 From 2009-09-24 11:16:56 PST -------
Xiaomei: Thanks for taking this on!

Could you please upload screenshots after applying this patch on Windows/Mac & Linux.
------- Comment #9 From 2009-09-24 11:37:38 PST -------
I looked at the code and LGTM.
------- Comment #10 From 2009-09-24 11:43:09 PST -------
(From update of attachment 40006 [details])
Code looks OK to me.  Waiting to set commit-queue until after xji replies to Jeremy.
------- Comment #11 From 2009-09-24 17:24:10 PST -------
(From update of attachment 40006 [details])
Setting commit-queue per Jeremy's request.  I believe he and xji talked offline.
------- Comment #12 From 2009-09-25 08:57:56 PST -------
(From update of attachment 40006 [details])
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 From 2009-09-25 12:25:23 PST -------
Created an attachment (id=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 From 2009-09-25 12:26:25 PST -------
(From update of attachment 40131 [details])
LGTM.
------- Comment #15 From 2009-09-25 12:31:02 PST -------
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 From 2009-09-25 12:32:35 PST -------
Created an attachment (id=40133) [details]
window's autofill
------- Comment #17 From 2009-09-25 12:33:35 PST -------
Created an attachment (id=40134) [details]
window's select
------- Comment #18 From 2009-09-25 12:34:55 PST -------
Created an attachment (id=40135) [details]
linux select
------- Comment #19 From 2009-09-25 12:35:49 PST -------
Created an attachment (id=40136) [details]
Linux autofill
------- Comment #20 From 2009-09-25 12:37:13 PST -------
Created an attachment (id=40137) [details]
mac autofill
------- Comment #21 From 2009-09-25 13:41:25 PST -------
(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 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 From 2009-09-25 13:44:12 PST -------
(From update of attachment 40131 [details])
Sorry, this was bug 28603.  I had some junk in the CommitQueue checkout causing every run to fail. :(
------- Comment #23 From 2009-09-25 13:54:10 PST -------
(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
------- Comment #24 From 2009-09-25 14:49:49 PST -------
(In reply to comment #23)
> (From update of attachment 40131 [details] [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 From 2009-09-25 15:14:23 PST -------
(From update of attachment 40131 [details])
Sorry, we were bitten by bug 29751.  I'll fix the commit-queue and this will land correctly this time.  Apologies again.
------- Comment #26 From 2009-09-25 15:25:12 PST -------
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 From 2009-09-25 19:04:47 PST -------
(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 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 From 2009-09-26 00:55:59 PST -------
(From update of attachment 40131 [details])
Sigh.  Another flakey test.  Bug 29322.
------- Comment #29 From 2009-09-26 06:12:27 PST -------
(From update of attachment 40131 [details])
Clearing flags on attachment: 40131

Committed r48789: <http://trac.webkit.org/changeset/48789>
------- Comment #30 From 2009-09-26 06:12:32 PST -------
All reviewed patches have been landed.  Closing bug.