Closed Bug 649840 Opened 13 years ago Closed 12 years ago

RTL on forms inputs autocomplete

Categories

(Toolkit :: Autocomplete, defect)

Product:
Toolkit
Component:
Autocomplete
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla6

People

(Reporter: ebrahim, Assigned: linostar)

References

(
URL
)

Details

(Keywords: rtl, Whiteboard: [fixed in mozilla6, with test updated in mozilla13])

Attachments

(4 files, 10 obsolete files)

screenshot of autocomplete in an RTL input box
3.75 KB, image/png
Details
modified patch [Checked in: Comment 34]
3.84 KB, patch
neil
: review+
Details | Diff | Splinter Review
Extra
8.30 KB, image/png
Details
Tweaked patch [Checked in: See comment 46+55]
1.92 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110413 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110413 Firefox/6.0a1

Hi, I think when an input is RTL, autocomplete menu of that input must be in RTL.

Reproducible: Always
Makes sense.
Component: General → Autocomplete
Keywords: rtl
Product: Firefox → Toolkit
QA Contact: general → autocomplete
Version: unspecified → Trunk
Justin, can you please point me to the code responsible for the auto-complete widget we show for input elements?  I'm not sure which piece of code is responsible for those widgets!  :-)
Is this really valid? I tested with the url above and I obtained a normal behavior (see the screenshot).
Sorry. Didn't notice that the bug is windows7-specific. Tested on Windows 7 and found it was valid.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → linux.anas
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — — Splinter Review 

        Attachment #525957 -
        Flags: review?

    
  

        Attachment #525957 -
        Flags: review? → review?(gavin.sharp)

    
    

    
  
Comment on attachment 525957 [details] [diff] [review]
patch v1

This fix is not correct.

:-moz-locale-dir only works in XUL documents, and its value changes based on the direction of the browser UI, not the direction of the input element in the web page.

The correct fix may be to modify this binding <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#577> (the xul:tree inside it) based on the computed direction property of this.input.

        Attachment #525957 -
        Flags: review?(gavin.sharp) → review-

    
    

    
  
I wonder if inspecting "dir" value of <html> and <body> tags (in css) would be a possible alternative, since an input textbox inherits "dir" from those?

    
    

    
  
(In reply to comment #7)
> I wonder if inspecting "dir" value of <html> and <body> tags (in css) would be
> a possible alternative, since an input textbox inherits "dir" from those?

That's not a good idea, since the dir attribute might be set on the input element explicitly.  You should probably use window.getComputedStyle on the input element itself.

    
    

    
  
Note that the autocomplete widget is, somewhat bizarrely, partly shared between the form history autocomplete and URL bar autocomplete. Please make sure that fixing one doesn't break the other. :)

    
    

    
  
It seems that this is not windows7-specific. Linux builds suffer suffer from this bug, but in a slightly different way. To make a long story short, here is a summary of the bug effect on both platforms:

- on linux, the autocomplete list inherits the direction from the browser locale; so even when we have an ltr textbox in an rtl firefox, the direction for the autocomplete will be rtl.

- on windows, neither browser locale nor textbox direction seem to affect the autocomplete direction. Perhaps it inherits the direction from the OS locale.

    
    

    
  
Sorry, I want mention that Chrome/Chromium working well on this

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This should fix the different issues on all platforms.

        Attachment #525957 -
        Attachment is obsolete: true

        Attachment #527265 -
        Flags: review?

    
    

    
  
Comment on attachment 527265 [details] [diff] [review]
patch v2

Nit: can you please use spaces instead of tabs?

Where is the "8px" value coming from?

    
  

        Attachment #527265 -
        Flags: review? → review-

    
    

    
  
I will replace this patch by another that uses spaces, and will create appropriate css rules instead of using this.style.* (based on vingtetun's advice).

The 8px is a rough approximation. The ltr popup list has such padding but the rtl one lacks it, so I added it on the right side.

    
    

    
  
(In reply to comment #14)
> I will replace this patch by another that uses spaces, and will create
> appropriate css rules instead of using this.style.* (based on vingtetun's
> advice).

Sounds good, thanks!

> The 8px is a rough approximation. The ltr popup list has such padding but the
> rtl one lacks it, so I added it on the right side.

Good to hear it.  I suggest to modify the same place in the theme code which sets the padding for the LTR case.  For example, if it's using padding-left, you can switch it to use -moz-padding-start, and it should just work!

Also, can you please write a chrome test for this which makes sure that the direction of the popup is correctly modifies when the direction of the textbox changes?

Thanks!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
A small remark concerning the padding. There *was* a |-moz-padding-start| under |.autocomplete-treebody::-moz-tree-cell-text|, but it wasn't having the desired effect when the textbox direction is rtl. I moved it to |panel[type="autocomplete"]| which fixed the problem.

        Attachment #527265 -
        Attachment is obsolete: true

        Attachment #527519 -
        Flags: review?(neil)

    
    

    
  
The chrome test result are (will be) available at: http://tbpl.mozilla.org/?tree=MozillaTry&rev=5cf8d8cc49e2

    
    

    
  
Anas, FWIW, if you submit a patch to the try server, it's usually a good idea to ask for debug builds as well.  Debug builds could potentially detect issues that opt builds cannot.

    
    

    
  
Comment on attachment 527519 [details] [diff] [review]
patch v3

>+SimpleTest.waitForExplicitFinish();
>+setTimeout(runTest, 0);

This should probably be:

addLoadEvent(runTest);

to make sure that runTest is called after the load event has fired.

There is also one other concern about testing.  Is this possible to test for normal HTML input elements too?  This test ensures that this wouldn't break for XUL textbox'es, but I'm more worried about <html:input> textboxes as used on the web.  But I have no idea how to do this test.

    
  
Depends on: 652007

    
    

    
  
Comment on attachment 527519 [details] [diff] [review]
patch v3

>+panel[type="autocomplete"] {
>+  -moz-padding-start: 8px;
>+}
>+
> .autocomplete-history-popup {
>   max-height: 180px;
> }
>@@ -117,10 +121,6 @@
>   -moz-appearance: none !important;
> }
> 
>-.autocomplete-treebody::-moz-tree-cell-text {
>-  -moz-padding-start: 8px;
>-}
Moving the padding to the panel was always the wrong thing to do. While there was somewhere else that you could have moved the padding to that would have worked around the RTL bug in tree cell text, I don't like workarounds ;-)
So I filed bug 652007 to try to fix the RTL bug in tree cell text first.

        Attachment #527519 -
        Flags: review?(neil) → review-

    
    

    
  
(In reply to comment #19)
> There is also one other concern about testing.  Is this possible to test for
> normal HTML input elements too?  This test ensures that this wouldn't break for
> XUL textbox'es, but I'm more worried about <html:input> textboxes as used on
> the web.  But I have no idea how to do this test.

I am not an expert in that field, so perhaps I am talking nonsense here. But, I think that <html:input> textboxes in the web *become* xul textboxes when they are browsed in Firefox. Otherwise, how could my patch on a xul element affect the behavior of the external textbox provided in the attached testcase above?

    
    

    
  
(In reply to comment #22)
> (In reply to comment #19)
> > There is also one other concern about testing.  Is this possible to test for
> > normal HTML input elements too?  This test ensures that this wouldn't break for
> > XUL textbox'es, but I'm more worried about <html:input> textboxes as used on
> > the web.  But I have no idea how to do this test.
> 
> I am not an expert in that field, so perhaps I am talking nonsense here. But, I
> think that <html:input> textboxes in the web *become* xul textboxes when they
> are browsed in Firefox. Otherwise, how could my patch on a xul element affect
> the behavior of the external textbox provided in the attached testcase above?

What actually happens is that there's an form fill controller that attaches the popup to whichever HTML input you're autocompleting, as distinct from the regular controller in a XUL autocomplete which always attaches the popup to the same XUL textbox.

    
    

    
  
Comment on attachment 527754 [details] [diff] [review]
patch v4 (addressing ehsan and neil remarks)

>diff -r f0a03fc3247d toolkit/content/xul.css
>--- a/toolkit/content/xul.css	Fri Apr 22 10:38:42 2011 +0200
>+++ b/toolkit/content/xul.css	Fri Apr 22 16:00:04 2011 +0400
>@@ -394,6 +394,14 @@
>   -moz-box-orient: vertical;
> }
> 
>+panel[popupdir="ltr"] {
>+  direction: ltr;
>+}
>+
>+panel[popupdir="rtl"] {
>+  direction: rtl;
>+}
Sorry I didn't notice before, but I'm not keen on putting rules in xul.css; I looked for something else that we do rtl cloning on which turned out to be the HTML tooltip; this simply sets the style.direction property directly on the popup. It seems to me that this would be a reasonable thing to do here too.

    
    

    
  
(In reply to comment #24)
> Sorry I didn't notice before, but I'm not keen on putting rules in xul.css; I
> looked for something else that we do rtl cloning on which turned out to be the
> HTML tooltip; this simply sets the style.direction property directly on the
> popup. It seems to me that this would be a reasonable thing to do here too.

Just to make sure I understand this right, you prefer using this.style.direction instead of using css rules?

    
    

    
  
Comment on attachment 527754 [details] [diff] [review]
patch v4 (addressing ehsan and neil remarks)

>+            var popupDirection = window.getComputedStyle(aElement, null).direction;
Also previous comments have also drawn my attention to this line, which works in the XUL test case but in the form autocomplete test case aElement may be in a different document so you should compute style using its view.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #527754 -
        Attachment is obsolete: true

        Attachment #527754 -
        Flags: review?(neil)

        Attachment #527773 -
        Flags: review?(neil)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #527773 -
        Attachment is obsolete: true

        Attachment #527773 -
        Flags: review?(neil)

        Attachment #527777 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 527777 [details] [diff] [review]
patch v6

Looks like you forgot to update the test to read the popup style.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  
Yes. Sorry about that :)

        Attachment #527777 -
        Attachment is obsolete: true

        Attachment #527777 -
        Flags: review?(neil)

        Attachment #527845 -
        Flags: review?(neil)

    
  

        Attachment #527845 -
        Flags: review?(neil) → review+

    
  
Keywords: checkin-needed

    
    

    
  
Comment on attachment 527845 [details] [diff] [review]
patch v7

>--- a/toolkit/content/widgets/autocomplete.xml	Fri Apr 22 10:38:42 2011 +0200
>+++ b/toolkit/content/widgets/autocomplete.xml	Sat Apr 23 00:17:00 2011 +0400

>+            // Fix the direction and the padding of the autocomplete popup list based on the textbox direction, bug 649840		
>+            var popupDirection = aElement.ownerDocument.defaultView.getComputedStyle(aElement, "").direction;
>+            this.style.direction = popupDirection;

As opposed to what the comment is saying, you're actually not messing with the padding anymore.

The second getComputedStyle parameter is redundant, by the way.

    
  
Keywords: checkin-needed

    
    

    
  
(In reply to comment #31)
> (From update of attachment 527845 [details] [diff] [review])
> >+            // Fix the direction and the padding of the autocomplete popup list based on the textbox direction, bug 649840		
> As opposed to what the comment is saying, you're actually not messing with the
> padding anymore.
Yeah, I meant to mention that too, but I forgot :-(

> The second getComputedStyle parameter is redundant, by the way.
But sufficiently recently redundant that nobody's bothered to update any existing callers yet ;-)

    
    

    
  


  

        Attachment #527845 -
        Attachment is obsolete: true

        Attachment #528077 -
        Flags: review?(neil)

    
  

        Attachment #528077 -
        Flags: review?(neil) → review+

    
  
Keywords: checkin-needed

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/c17c9e2c8845
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6

    
    

    
  
Thanks a lot Anas for your excellent work here.  :-)

    
    

    
  

      
      
      
      

        Attached image
          
        Extra
        — 
      

    


  
Hi. First of all I would like to thank you for your greats jobs here :)
Sometimes (not always) in working with Firefox's searchbox—as you can see in my attachment—I see extra RTL style for search auto-complete popup. Can this be checked? (And this new bug is related to this bug?) Thank you :)

    
    

    
  
Bump!

    
    

    
  
(In reply to ebraminio from comment #36)
> Created attachment 549315 [details]
> Extra
> 
> Hi. First of all I would like to thank you for your greats jobs here :)
> Sometimes (not always) in working with Firefox's searchbox—as you can see in
> my attachment—I see extra RTL style for search auto-complete popup. Can this
> be checked? (And this new bug is related to this bug?) Thank you :)

I'm not sure what you mean, but the scrollbar is definitely misplaced in that screenshot.  Would you mind filing a new bug about that?  Thanks!

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #38)
I filled this: bug 686406
Thanks

    
    

    
  
(In reply to Ehsan Akhgari from comment #19)
> (From update of attachment 527519 [details] [diff] [review])
> There is also one other concern about testing.  Is this possible to test for
> normal HTML input elements too?  This test ensures that this wouldn't break
> for XUL textbox'es, but I'm more worried about <html:input> textboxes as
> used on the web.  But I have no idea how to do this test.
You could probably do it as part of the satchel tests.

    
    

    
  
Comment on attachment 528077 [details] [diff] [review]
modified patch
[Checked in: Comment 34]

So, it turns out that the test isn't really testing anything useful because the test autocomplete popup is a child of the autocomplete and therefore would automatically have inherited the direction anyway.

    
    

    
  


  
We discovered in bug 707039 that the test in attachment 528077 [details] [diff] [review] doesn't technically test anything useful, because the popup is a child of the autocomplete and so would have inherited the correct direction anyway.
Ideally I'd like to remove that test and just use this one...

        Attachment #591947 -
        Flags: review?(gavin.sharp)

        Attachment #591947 -
        Flags: review?(gavin.sharp) → review?(ehsan)

    
  

        Attachment #591947 -
        Flags: review?(ehsan) → review+

    
    

    
  


  

        Attachment #591947 -
        Attachment is obsolete: true

        Attachment #592246 -
        Flags: review?(ehsan)

    
  

        Attachment #592246 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 592246 [details] [diff] [review]
Tweaked patch
[Checked in: See comment 46+55]

https://hg.mozilla.org/mozilla-central/rev/8705f07a49ec (12a1)

        Attachment #592246 -
        Attachment description: Tweaked patch → Tweaked patch
[Checked in: Comment 44]

        Attachment #528077 -
        Attachment description: modified patch → modified patch
[Checked in: Comment 34]

    
    

    
  
> Tweaked patch
> [Checked in: Comment 44]
> 
> https://hg.mozilla.org/mozilla-central/rev/8705f07a49ec (12a1)

Backed out for m-c bustage (makefile entry not removed for the deleted file). Didn't fix in place since the removed test wasn't in the reviewed patch.

https://hg.mozilla.org/mozilla-central/rev/2bdf77e42239
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Comment on attachment 592246 [details] [diff] [review]
Tweaked patch
[Checked in: See comment 46+55]

(In reply to Ed Morley [:edmorley] from comment #45)
> Didn't fix in place since the removed test wasn't in the reviewed patch.
> 
> https://hg.mozilla.org/mozilla-central/rev/2bdf77e42239

Ah, well, doing things in parallel :-/

https://hg.mozilla.org/mozilla-central/rev/8b6e498d5933
Better test for bug 649840. r=ehsan. + bustage-fix by sgautherie.

Asking for feedback post-landing :-|

        Attachment #592246 -
        Attachment description: Tweaked patch
[Checked in: Comment 44] → Tweaked patch
[Checked in: Comment 46]

        Attachment #592246 -
        Flags: feedback?(ehsan)

        Attachment #592246 -
        Attachment description: Tweaked patch
[Checked in: Comment 46] → Tweaked patch
[Checked in: See comment 46]

    
    

    
  
(In reply to Serge Gautherie (:sgautherie) from comment #46)
> Comment on attachment 592246 [details] [diff] [review]
> Tweaked patch
> [Checked in: See comment 46]
> 
> (In reply to Ed Morley [:edmorley] from comment #45)
> > Didn't fix in place since the removed test wasn't in the reviewed patch.
> > 
> > https://hg.mozilla.org/mozilla-central/rev/2bdf77e42239
> 
> Ah, well, doing things in parallel :-/
> 
> https://hg.mozilla.org/mozilla-central/rev/8b6e498d5933
> Better test for bug 649840. r=ehsan. + bustage-fix by sgautherie.
> 
> Asking for feedback post-landing :-|

Ugh this missed the point "removed test wasn't in the reviewed patch" actually makes a difference.

    
    

    
  
Why did this landing remove a test without any reviews?  I'll back this out.

    
    

    
  
Comment on attachment 592246 [details] [diff] [review]
Tweaked patch
[Checked in: See comment 46+55]

My review on this patch still stands.

        Attachment #592246 -
        Flags: feedback?(ehsan)

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #48)
> Why did this landing remove a test without any reviews?  I'll back this out.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1685737184

    
    

    
  
(In reply to Ehsan Akhgari [:ehsan] from comment #48)
> Why did this landing remove a test without any reviews?  I'll back this out.

I'd like an answer to this as well. Simply removing a test without review is not acceptable. Serge, please explain what happened here.

    
    

    
  
Serge was just following Neil's plan from comment 42. That should have been included in the patch to review, though.

    
    

    
  
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #53)
> Serge was just following Neil's plan from comment 42. That should have been
> included in the patch to review, though.

Correct.  I also managed to miss comment 42 somehow.  So all in all, unfortunate turn of events.

I gave Neil verbal r+ on IRC for a patch which removes the test.  Effectively changeset 1a1685737184 should be backed out.

    
    

    
  
Comment on attachment 592246 [details] [diff] [review]
Tweaked patch
[Checked in: See comment 46+55]

(In reply to Ehsan Akhgari [:ehsan] from comment #54)
> Effectively changeset 1a1685737184 should be backed out.

https://hg.mozilla.org/mozilla-central/rev/e777c939a3f9

        Attachment #592246 -
        Attachment description: Tweaked patch
[Checked in: See comment 46] → Tweaked patch
[Checked in: See comment 46+55]
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in mozilla6, with test updated in mozilla13]

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #42)

> We discovered in bug 707039 that the test in attachment 528077 [details] [diff] [review]
> [diff] [review] doesn't technically test anything useful, because the popup
> is a child of the autocomplete and so would have inherited the correct
> direction anyway.

Ftr, that was in bug 707039 comment 43 (and previous).

> Ideally I'd like to remove that test and just use this one...

The new check added to test_bug_511615.html does fail if I locally back the autocomplete.xml fix out :-)

V.Fixed
Status: RESOLVED → VERIFIED

    
    

    
  
Comment on attachment 592246 [details] [diff] [review]
Tweaked patch
[Checked in: See comment 46+55]

PS: Though I must say I'm a little puzzled at how this new check works.

1) HTML sets dir=rtl. (But that is never checked.)
2) At start of test, JS sets dir=ltr. (But that is never checked either.)
3) At end of test, dir=rtl is checked.

Per my comment 56, it seems something is actually happening (as expected).
But it doesn't seem obvious (as in checked/documented) that 2- worked or when/why the direction changed back to rtl.

    
    

    
  


  
Addressing my comment 57.
I would like to document why the direction is changing (at that point) too:
could you explain it?

        Attachment #594480 -
        Flags: review?(neil)

    
    

    
  
(In reply to Serge Gautherie (:sgautherie) from comment #57)
> PS: Though I must say I'm a little puzzled at how this new check works.
> 
> 1) HTML sets dir=rtl.
In the content, yes.

> 2) At start of test, JS sets dir=ltr.
In the chrome, yes.

> 3) At end of test, dir=rtl is checked.
In the chrome, yes.

> Per my comment 56, it seems something is actually happening (as expected).
> But it doesn't seem obvious (as in checked/documented) that 2- worked or
> when/why the direction changed back to rtl.
One of the tests causes the popup to open, triggering the direction code.

    
    

    
  
Comment on attachment 594480 [details] [diff] [review]
(AAv1) test_bug_511615.html: Improve 'autocompletePopup.style.direction' checks

>+// Should be "" on first run, but can be "rtl" if test is (manually) reloaded.
Actually it could be anything depending on previously run tests.

I don't see a benefit to moving the other test, in case someone needs to adjust the tests at a later date.

        Attachment #594480 -
        Flags: review?(neil) → review-

    
    

    
  


  
AAv1, with comment 59 and comment 60 suggestion(s).

        Attachment #594480 -
        Attachment is obsolete: true

        Attachment #594499 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 594499 [details] [diff] [review]
(AAv2) test_bug_511615.html: Improve 'autocompletePopup.style.direction' checks

please file a new bug. this one is resolved.

        Attachment #594499 -
        Attachment is obsolete: true

        Attachment #594499 -
        Flags: review?(neil)

    
    

    
  
> (AAv2) test_bug_511615.html: Improve 'autocompletePopup.style.direction' checks
I didn't think the change was worthwhile anyway.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: