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.
also reported in Chromium: http://crbug.com/6244 http://crbug.com/9839
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
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.
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.
(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.
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
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.
Xiaomei: Thanks for taking this on! Could you please upload screenshots after applying this patch on Windows/Mac & Linux.
I looked at the code and LGTM.
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 on attachment 40006 [details] patch w/ manual test Setting commit-queue per Jeremy's request. I believe he and xji talked offline.
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
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 on attachment 40131 [details] patch w/ manual test LGTM.
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.
Created attachment 40133 [details] window's autofill
Created attachment 40134 [details] window's select
Created attachment 40135 [details] linux select
Created attachment 40136 [details] Linux autofill
Created attachment 40137 [details] mac autofill
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 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 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
(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 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.
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 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 on attachment 40131 [details] patch w/ manual test Sigh. Another flakey test. Bug 29322.
Comment on attachment 40131 [details] patch w/ manual test Clearing flags on attachment: 40131 Committed r48789: <http://trac.webkit.org/changeset/48789>
All reviewed patches have been landed. Closing bug.