Bug 27889 – [Chromium] RTL autocomplete popup is not layout correctly.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Xiaomei Ji]

Created attachment 33887 [details]
test case

Steps to reproduce:

1. Open attached file "RTL_Form_Autofill_Pop-up.html" in Chrome.
2. Type some long text in the "First name" input box
   >>Say "abcd abcd abcd abcd abcd abcd"
3. Click "Submit"
4. Open the file again
5. Type the first letter "a"
6. The form autofill pop-up should show up now

Result:
The form autofill pop-up width is much longer than that of the input box, it is left-aligned, and extended all the way to the right.

Expected:
All other browsers (Fx, IE, Safari), the width of the form autofill pop-up is always the
same as the width of input box, and show ellipsis for the long text inside
the pop-up, so they don't have this problem, please refer the picture attached

[Xiaomei Ji]

related Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=7323

[Xiaomei Ji]

I am not sure what should be the correct behavior.
Following are my observations and my questions:

1. In windows, Safari, FireFox, and IE all behave the same. They all display the items in the popup list in left-to-right direction, no matter whether the input field is an LTR or RTL field.
Using the attached test case, I tried to submit the following RTL text and Bidi text which begins with 'ש' logically.
(from left-to-right is the logical order)  "ששש abc נננ"  
(logical order)                                   "ששש abcdefg נננ hijklmn"
(visual order)                                    "שששששש ננננננ בבבבבבבבב"

Attached the snapshot ( RTL_form_autofill_safari_windows.jpg) of Safari in Windows in autofill phase.

2. Safari in Mac and Chrome behave the same. They both extend the item display width beyond the input field width so no text truncation is needed.  And they use heuristics to determine the directionality of items in the popup list. The directionality is based on the first character with strong directionality.
They display the items correctly if the directionality of the first strong-directionality character matches the directionality of the input field, such as RTL input field with an item begins with Hebrew character. But they do not display items correctly if the directionalities do not match, such as an RTL input field with an item begins with English character.

Attached the snapshots of Chrome in Windows where the directionality matches (chrome_item_begin_with_Hebrew.jpg) and does not match (chrome_item_begin_with_a.jpg).


I am trying to change Chrome to behave the same (limit the display width to be the same as input field width by truncating item text) in Windows as other browsers. My questions are:

1. why all those browsers either assume the popup item's directionality as LTR or based on heuristics? Is there any specific reason? Should the popup item be displayed using the  input field's directionality?

2. What should be the correct behavior in Mac, should I leave Chrome as it is to be consistent with Safari in Mac. Or should I change it to be the same as FireFox in Mac? (FireFox always do text truncation to keep the item width the same as input field width, and it always assumes text item as LTR directionality, no matter in Windows or Mac)?  How about Linux?

3. Jay mentioned that we do not want to restrict the width of combo-box (<select> tag), but how about the text display directionality? Should it be different from autofill popup?


Thanks,
Xiaomei

[Xiaomei Ji]

Created attachment 33986 [details]
Safari in Windows (Fx and IE in windows are the same)

[Xiaomei Ji]

Created attachment 33987 [details]
Chrome, directionality of item (begins with Hebrew) and input field (RTL) are the same.

[Xiaomei Ji]

Created attachment 33988 [details]
Chrome, directionality of item (begins with 'a') and input field (RTL) are different

[Jay Campan]

Some answers:
>1. why all those browsers either assume the popup item's directionality as LTR
>or based on heuristics? Is there any specific reason? Should the popup item be
>displayed using the  input field's directionality?
I am not sure why that is the behavior of all browsers. It suspect just like in Chrome this is just not implemented and defaults to LTR.
I think that the popup item should be using the input field's directionality.

>2. What should be the correct behavior in Mac, should I leave Chrome as it is
>to be consistent with Safari in Mac. Or should I change it to be the same as
>FireFox in Mac? (FireFox always do text truncation to keep the item width the
>same as input field width, and it always assumes text item as LTR
>directionality, no matter in Windows or Mac)?  How about Linux?
I think Chrome on all platforms should limit the size of the popup to the size of the input field and use truncation.

>3. Jay mentioned that we do not want to restrict the width of combo-box
>(<select> tag), but how about the text display directionality? Should it be
>different from autofill popup?
I was making that point because the code for the popup is shared by the autofill and <select> combobox. So we want different behavior for the 2 cases: combox, no width restriction, autofill, width restricted to the input field's width.
The directionality of the combobox should be consistent with field's directionality as well.

[Aharon (Vladimir) Lanin]

To prevent confusion on the directionality issue, I would suggest using simpler test strings: "hi!" and "אא!". The directionality is LTR if the exclamation mark comes out on the right, and RTL if it comes out on the left.

> 1. In windows, Safari, FireFox, and IE all behave the same. They all display
> the items in the popup list in left-to-right direction, no matter whether the
> input field is an LTR or RTL field.

That is not the result that I get:

Firefox 3.0.11: always in browser language directionality (i.e. LTR)
Safari 4.0.2: always in element directionality (i.e. RTL for the test page)
Opera 9.64: did not auto-complete
IE 7: did not auto-complete

Thus, Firefox, Safari, and Chrome on Windows all do different things.

It is important to point out that displaying a value in the right directionality is not a nice-to-have. In the wrong directionality, the value often comes out simply garbled. For example, displaying "17 Main St." as ".Main St 17", which is what comes out when it is displayed RTL, is simply wrong.

BTW, the current Chrome bug that the drop-down is always left-aligned, even for a dir=rtl align=right element, is also a bug. No one else does that. Chrome has the same bug for <select> elements.

In my opinion, it is Chrome's current behavior re directionality here that is the most useful. In most cases, a text input needs to be capable of accepting - and displaying correctly - both LTR and RTL values. Thus, our BiDi support recommendations for web pages is to add directionality auto-detection scripts to most inputs that after each keystroke check the directionality of the current input value and adjust its dir and align values accordingly. Thus, both LTR and RTL values could have been submitted in the past, and all need to be displayed correctly, so checking the directionality of each value in the dropdown as Chrome does is the best. Failing that, Safari's approach of using the element's current directionality for all the items in the dropdown is still pretty much ok, since the likelihood that any given user will have entered both LTR and RTL values for a single field is not very high. In fact, it is in fact probably the better approach when the page does not have the directionality auto-detection scripts I mentioned earlier. But Firefox's approach of always using the browser language's directionality is totally nonsensical: RTL pages taking consistently RTL inputs will always have the dropdown values garbled.

[Xiaomei Ji]

Hi Aharon,

Thanks for your reply.
Please see my comments inline.

(In reply to comment #7)
> To prevent confusion on the directionality issue, I would suggest using simpler
> test strings: "hi!" and "אא!". The directionality is LTR if the exclamation
> mark comes out on the right, and RTL if it comes out on the left.
> 
> > 1. In windows, Safari, FireFox, and IE all behave the same. They all display
> > the items in the popup list in left-to-right direction, no matter whether the
> > input field is an LTR or RTL field.
> 
> That is not the result that I get:
> 
> Firefox 3.0.11: always in browser language directionality (i.e. LTR)
> Safari 4.0.2: always in element directionality (i.e. RTL for the test page)
> Opera 9.64: did not auto-complete
> IE 7: did not auto-complete

I attached the display of "שש!" in Safari 4.0.2 in Windows.
It is displayed as RTL (the element directionality) in input field, but it is displayed as LTR in the pop-up list.

Looks IE8 supports auto-complete, but it displays "hi!" and "שש!" in the pop-up list as LTR using the test page.



> 
> Thus, Firefox, Safari, and Chrome on Windows all do different things.
> 
> It is important to point out that displaying a value in the right
> directionality is not a nice-to-have. In the wrong directionality, the value
> often comes out simply garbled. For example, displaying "17 Main St." as ".Main
> St 17", which is what comes out when it is displayed RTL, is simply wrong.
> 
> BTW, the current Chrome bug that the drop-down is always left-aligned, even for
> a dir=rtl align=right element, is also a bug. No one else does that. Chrome has
> the same bug for <select> elements.

There are Chrome bugs on the wrong alignment for both autofill and <select>, and I will fix them in another patch.


> 
> In my opinion, it is Chrome's current behavior re directionality here that is
> the most useful. In most cases, a text input needs to be capable of accepting 
> and displaying correctly - both LTR and RTL values. 
> Thus, our BiDi support
> recommendations for web pages is to add directionality auto-detection scripts
> to most inputs that after each keystroke check the directionality of the
> current input value and adjust its dir and align values accordingly. Thus, both
> LTR and RTL values could have been submitted in the past, and all need to be
> displayed correctly, so checking the directionality of each value in the
> dropdown as Chrome does is the best.



Do you mean even if the text input field is marked as RTL,
it should display "hi!" in such input field as LTR?
and/or it should displayed "hi!" as LTR in the pop-up list? 

If that is the case, what is the point of setting "dir" in the input field?
Or you mean "dir" should not be needed for autofill and <select>, a webpage should be using auto-detect script to detect/display its text directionality automatically?


If the auto-detect only applies to display text in pop-up list, that will cause in-consistency of the text displayed in input (or <select> box) and text displayed in the the pop-up list.

For example, the following <select>page:
https://bugs.webkit.org/attachment.cgi?id=14607&action=view

If you select the 2nd item, you can see the text displayed in the option box (with "English" at very right) and text displayed in the pop-up list ("English" displayed at the very left) are laid-out different.

The text in option box is displayed as LTR (because the dir of <select> is LTR), and the same text in the pop-up list is displayed as RTL (because directionality is auto-detected using the first strong directional character's directionality).

In such case, what should be the correct behavior?
(In fact, Safari 4.0.2 display the text in RTL directionality in both option box and pop-up list, although I do not know how Safari does it? And why it can do such in <select> but not in autofill).
 

Thanks,
Xiaomei

> Failing that, Safari's approach of using
> the element's current directionality for all the items in the dropdown is still
> pretty much ok, since the likelihood that any given user will have entered both
> LTR and RTL values for a single field is not very high. In fact, it is in fact
> probably the better approach when the page does not have the directionality
> auto-detection scripts I mentioned earlier.
> But Firefox's approach of always
> using the browser language's directionality is totally nonsensical: RTL pages
> taking consistently RTL inputs will always have the dropdown values garbled.

[Xiaomei Ji]

Created attachment 34161 [details]
Safari 4.0.2 display "!BA"

[Aharon (Vladimir) Lanin]

(In reply to comment #8)
Please see my comments inline.

> I attached the display of "שש!" in Safari 4.0.2 in Windows.
> It is displayed as RTL (the element directionality) in input field, but it is
> displayed as LTR in the pop-up list.

My mistake, you are right. I was looking at the input itself - I did not get the dropdown until I entered a few more values starting the same way.

> Looks IE8 supports auto-complete, but it displays "hi!" and "שש!" in the pop-up
> list as LTR using the test page.

I don't have IE8 yet. I guess it looks like all the browsers are using the browser language directionality, which I think is just terrible.

> > In my opinion, it is Chrome's current behavior re directionality here that is
> > the most useful. In most cases, a text input needs to be capable of accepting 
> > and displaying correctly - both LTR and RTL values. 
> > Thus, our BiDi support
> > recommendations for web pages is to add directionality auto-detection scripts
> > to most inputs that after each keystroke check the directionality of the
> > current input value and adjust its dir and align values accordingly. Thus, both
> > LTR and RTL values could have been submitted in the past, and all need to be
> > displayed correctly, so checking the directionality of each value in the
> > dropdown as Chrome does is the best.
> 
> 
> 
> Do you mean even if the text input field is marked as RTL,
> it should display "hi!" in such input field as LTR?
> and/or it should displayed "hi!" as LTR in the pop-up list? 

I meant that it should be displayed as LTR in the autofill dropdown, but RTL in the input - unless a page script changes input's dir at that point - as it does now.

> If that is the case, what is the point of setting "dir" in the input field?

To control how the input itself is displayed, not the dropdown. That, of course raises the logical question: if that browser does directionality auto-detection on the dropdown values, shouldn't it do it for the input itself?

Jeremy raised this question outside of the dropdown context. My response was and is that this would be a good thing - but only if the page requested that the browser do so, e.g. via a new dir=auto value. There is a discussion going on re adding dir=auto or something like that to HTML5. I do not think it would be a good idea to do directionality auto-detection on input values without dir=auto, since it would interfere with the scripts that we put in to do that right now, and we would have to modify the scripts to make an exception for Chrome. Furthermore, doing auto-detection in inputs does not do anything for when the value is later displayed in another page. And I do not think it would be a good idea to implement dir=auto in Chrome before its design has been completed, since it would interfere with whatever the eventual standard will be. 

The reason that I do like directionality auto-detection on the autofill dropdown values is that unlike the input itself, the page has absolutely no control over these, and can not do auto-detection for them in script.

> Or you mean "dir" should not be needed for autofill and <select>, a webpage
> should be using auto-detect script to detect/display its text directionality
> automatically?

Yes, I do think that in the current HTML standard, a webpage should be doing its own auto-detection in script on inputs. For <select>, whoever prepares the HTML of the <option>s - either the server or page script - should be using LRE|RLE and PDF to declare the ones that have directionality opposite to that of the <select>. (The dir attribute does not seem to work on <option> and you certainly don't want script fooling with it on the <select>, since it controls on which side the down arrow is displayed.) For autofill, however, the page simply has no control over the way the autofill values are displayed.

> If the auto-detect only applies to display text in pop-up list, that will cause
> in-consistency of the text displayed in input (or <select> box) and text
> displayed in the the pop-up list.

Let's leave <select> out of it, it's different (see below). But for autofill, yes, the result is inconsistent when the page does not have script to update the input's dir. So, should we do what's best for the pages that don't have such script (unfortunately, that's most pages), or what's best for the pages that do? I am not sure... I don't see a way to please both. The simple answer is to do what's best for the majority, but that leaves no way for the page to achieve the truly correct behavior.

So for autofill my answer, unfortunately, is "I don't know". However, I do know that the current behavior in the other browsers is bad. If you decide to take away the auto-detection in the autofill dropdown, please use the <input>'s directionality for them, not the browser's.

Now, on to <select>.

> 
> For example, the following <select>page:
> https://bugs.webkit.org/attachment.cgi?id=14607&action=view
> 
> If you select the 2nd item, you can see the text displayed in the option box
> (with "English" at very right) and text displayed in the pop-up list ("English"
> displayed at the very left) are laid-out different.
> 
> The text in option box is displayed as LTR (because the dir of <select> is
> LTR), and the same text in the pop-up list is displayed as RTL (because
> directionality is auto-detected using the first strong directional character's
> directionality).
>
> In such case, what should be the correct behavior?
>
> (In fact, Safari 4.0.2 display the text in RTL directionality in both option
> box and pop-up list, although I do not know how Safari does it? And why it can
> do such in <select> but not in autofill).

That really is quite interesting. I do not know what they are doing, but I like the outcome very much. It is exactly what I want. Note that when you select the second value in Safari, it gets displayed as RTL in the <select> too - there is no inconsistency. And what's best, I do not think that LRE|RLE + PDF in the <option> values, when present, would interfere with what Safari is doing.

It is quite possible that Safari chose to do this for <select>, but not for text inputs, precisely because of the collision with the text input's dir value, which is not an issue for <select>.

[Xiaomei Ji]

Created attachment 34234 [details]
patch w/ manual test

Since the change only affects Chromium's <select> and autofill, I do not think it can be auto-tested.
Please let me know otherwise.

[Xiaomei Ji]

Created attachment 34240 [details]
patch w/ manual test

1. Since the change only affects Chromium's <select> and autofill, I do not think it can be auto-tested.
Please let me know otherwise.

2. For autofill, I am using the input element's direction to display the item in drop-down list, mainly to keep the display in drop-down list consistent with the display in input field.
for <select>, I leave the directionality of item in drop-down list as auto-detected (as first strong directional character's direction).

[Eric Seidel (no email)]

Comment on attachment 34234 [details]
patch w/ manual test

Please obsolete old patches when uploading new ones.  bugzilla-tool will do this for you if you use it to post your patches.  See bugzilla-tool post-diff and bugzilla-tool post-commits.

[Eric Seidel (no email)]

Comment on attachment 34240 [details]
patch w/ manual test

This constructor will run at library load time and slow down Chromium's launch time:
+static const String kEllipses = "...";

Style violation:
 55             : label(label), type(type), yOffset(0), ellipsesWidth(0) { }

Style:
 349         if ( totalWidth > width)

Style violations, no argument names when not needed:
 410         void drawBidiTextWithinWidth(const Font& font, const TextRun& run, const FloatPoint& point, int width);

I don't think I can actually review the code here.  I would need to study these files more.  r- for the style stuff.  Make sure to run check-webkit-style if you haven't already. :)

[Xiaomei Ji]

Hi Eric,

Thanks for your review!

(In reply to comment #14)
> (From update of attachment 34240 [details])
> This constructor will run at library load time and slow down Chromium's launch
> time:
> +static const String kEllipses = "...";

changed to use string literal locally (instead of using static const char*).


> 
> Style violation:
>  55             : label(label), type(type), yOffset(0), ellipsesWidth(0) { }
> 
> Style:
>  349         if ( totalWidth > width)
> 
> Style violations, no argument names when not needed:
>  410         void drawBidiTextWithinWidth(const Font& font, const TextRun& run,
> const FloatPoint& point, int width);

Done the style changes. Unfortunately, check-webkit-style can not catch them.


> 
> I don't think I can actually review the code here.  I would need to study these
> files more.  r- for the style stuff.  Make sure to run check-webkit-style if
> you haven't already. :)

I guess mitz could review the changes in GraphicsContext.
Jay (jcampan) probably could review the changes in PopupMenuChromium. Not sure whether Jay is webkit reviewer. But he is probably familiar with this piece of code, and I could certainly use his comments.

Thanks,
Xiaomei

[Xiaomei Ji]

Created attachment 34313 [details]
patch w/ manual test

[Eric Seidel (no email)]

Jay is not a webkit reviewer, but his comments here would be most welcome.  Please ask him to comment on this bug as to if he believes this patch is correct or not.

[Jay Campan]

Here are my comments on the patch:

WebCore/platform/chromium/PopupMenuChromium.cpp
I think it would be better to make the RTL detection behavior not dependent on m_settings.restrictWidthOfListBox and instead being a new parameter of m_settings.
The idea would be to keep PopupMenuChromium generic and let the client decides which heuristic to use.

In paintRow() regarding the ellipsis code. That code seems non-specific to the autofill. Would it make sense to have GraphicsContext::drawBidiTextWithinWidth() draw the ellipsis (may be the method could have an extra-parameter specifying whether or not an ellipsis is desired)?
I am assuming the ellipsis ... is the same in all languages. If it is not, may be the extra-param could be a string of the ellipsis to use? (empty string for no ellipsis)

[Xiaomei Ji]

Hi Jay,

Thanks for your comments!
Please see my reply inline.

(In reply to comment #18)
> Here are my comments on the patch:
> 
> WebCore/platform/chromium/PopupMenuChromium.cpp
> I think it would be better to make the RTL detection behavior not dependent on
> m_settings.restrictWidthOfListBox and instead being a new parameter of
> m_settings.
> The idea would be to keep PopupMenuChromium generic and let the client decides
> which heuristic to use.

Make sense. I've updated.

> 
> In paintRow() regarding the ellipsis code. That code seems non-specific to the
> autofill. Would it make sense to have
> GraphicsContext::drawBidiTextWithinWidth() draw the ellipsis (may be the method
> could have an extra-parameter specifying whether or not an ellipsis is
> desired)?
> I am assuming the ellipsis ... is the same in all languages. If it is not, may
> be the extra-param could be a string of the ellipsis to use? (empty string for
> no ellipsis)

Make sense. I've moved the code inside GraphaicsContext::drawBidiText(WithinWidth).

And per Mitz's suggestion, I used StringTruncator instead of manually truncating the string and placing ellipses. The code is much elegant. One thing to note is that in StringTruncator, horizontal ellipses (U+2026) will always be drawn when text is truncated. I think that should be fine.

Thanks,
Xiaomei

[Xiaomei Ji]

Created attachment 34536 [details]
patch w/ manual test

updated patch per Jay and Mitz's suggestions:
1. add a new flag to indicate in what directionality text in popup will be drawn.
2. move draw ellispses with draw truncated text from PopupMenuChromium to GraphicsContext
3. use StringTruncator to prepare truncated text along with ellipses to be drawn.

Comment on attachment 34536 [details]
patch w/ manual test

Switching to StringTruncator is a move in the right direction, but I don’t think the GraphicsContext changes are good or necessary. GraphicsContext is not the place to create Strings and truncate them. The logical structure where drawBidiText calls drawBidiTextWithinWidth, which calls back into drawBidiText does not make much sense either. Also, if width != -1 (that is, the caller wants truncation), calling Font::floatWidth() once seems wasteful, as the StringTruncator should have the smarts to detect the case where no truncation is necessary.

I think your PopupListBox should handle the truncation, create a TextRun from the potentially-truncated String, and call drawBidiText with that. I also don’t understand why you need the ellipsesNeeded member. It seems like you could just always use the StringTruncator. It won’t truncate when unnecessary. If you find out that you need to optimize, then you can cache the actual truncated strings (rather than just a bit telling you whether to truncate).

[Xiaomei Ji]

Created attachment 34585 [details]
patch w/ manual test

updated per Mitz's suggestion:
1. revert changes in GraphicsContext
2. remove ellipseseNeeded in PopupItem. When restrcitWidthOfListBox, truncate string first and drawBidiText on truncated string in PopupMenuChromium.

[Xiaomei Ji]

Hi Mitz:

One question on the ellipses appended in StringTruncator.
StringTruncator always append horizontal ellipses (U+2026) along with the truncated string. Will the Unicode character cause display problem?

Thanks,
xiaomei

(In reply to comment #23)
> StringTruncator always append horizontal ellipses (U+2026) along with the
> truncated string. Will the Unicode character cause display problem?

drawBidiText() will use a fallback font if the specific font does not have a horizontal ellipses glyph. Text truncation in the render tree is also implemented using horizontal ellipses.

[Jeremy Moskovich]

Comment on attachment 34585 [details]
patch w/ manual test

Really, really great work on this - a few minor comments.

> @@ -75,7 +76,10 @@ static const PopupContainerSettings drop
>      true, // focusOnShow
>      true, // setTextOnIndexChange
>      true, // acceptOnAbandon
> -    false // loopSelectionNavigation
> +    false, // loopSelectionNavigation
> +    false,  // restrictWidthOfListBox
Please align the comments with the ones above.

> @@ -817,25 +821,32 @@ void PopupListBox::paintRow(GraphicsCont
> +    // FIXME: http://b/1210481 We should get the padding of individual option
While you're here, could you change this to point at a bug on the public tracker?


> @@ -87,6 +91,27 @@ namespace WebCore {
> +        // Wheter restrict the width of the PopupListBox or not. We restrict
Perhaps:
"""
Should we restrict the width of the PopupListBox?
Autocomplete popups are restricted, combo-boxes (select tags) aren't.
"""
> +        // We could either display the items in the drop-down using its DOM element's
> +        // directionality, or we could display the items in the drop-down using heuristics:
could->can
> +        // https://bugs.webkit.org/show_bug.cgi?id=27889 for detail.
details

[Xiaomei Ji]

Comment on attachment 34585 [details]
patch w/ manual test

was r+'ed by eric and landed.
I am resetting the flag due to bugzilla data lose.

[Xiaomei Ji]

Following comments were lost due to "Mac OS Forge experienced database corruption during our maintenance reboot this evening and had to roll back to a backup from this  morning, August 20 at 2am."

And I am cleaning the review flag and set this bug as fixed.


--- Comment #27 from Jeremy Moskovich <[e-mail]>  2009-08-20 15:05:23 PDT ---
As far as I'm concerned, LGTM!


--- Comment #28 from Eric Seidel <[e-mail]>  2009-08-20 15:07:56 PDT ---
(From update of attachment 35229)
With Jeremy's endorsement I'm comfortable r+ing this.  It only affects
chromium.


--- Comment #29 from Eric Seidel <[e-mail]>  2009-08-20 15:22:49 PDT ---
(From update of attachment 35229)
Clearing flags on attachment: 35229

Committed r47599: <http://trac.webkit.org/changeset/47599>


--- Comment #30 from Eric Seidel <[e-mail]>  2009-08-20 15:22:57 PDT ---
All reviewed patches have been landed.  Closing bug.

[Xiaomei Ji]

Created attachment 38381 [details]
patch w/ manual test

I just realized that original comment #26 with the landed patch was lost too.
Here it is, the last and landed patch. 
This is the one r+'ed and landed, the one referred as attachment 35229 in comments 27-30.
It is based on attachment 34585 [details] with the only difference as incorporating Jeremy's suggestion in comment #25.