Closed Bug 630564 Opened 9 years ago Closed 9 years ago

Ctrl+shift+x should switch directions in location bar in RTL mode

Categories

(Firefox :: Address Bar, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: moz.teodosia, Assigned: ehsan)

References

()

Details

(Keywords: rtl, Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 file, 1 obsolete file)

Build ID: Mozilla/5.0 (Windows NT 6.1; rv:2.0b11pre) Gecko/20110131 Firefox/4.0b11pre 

Steps To Reproduce:
1. Install a RTL language pack.
2. Focus on the location bar and type an URL.
3. Press Ctrl+shift+x.

Actual Results:
Only the cursor moves, but the text doesn't.

Expected Results:
Ctrl+shift+x should switch directions. Text should be aligned to the right, and direction should be changed to RTL/LTR.
Without the language pack (in LTR mode), the ctrl+shift+x combination works.
Possible regression from bug 629172? or it was not fully fixed?

Teodosia, does it work in the 2011-01-26 nightly?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011-01-26-06-mozilla-central/
blocking2.0: --- → ?
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
Using Mozilla/5.0 (Windows NT 6.1; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre the ctrl+shift+x combination doesn't work in textareas nor into the location bar.

Since bug 629172 was fixed, ctrl+shift+x works in textareas, but not into the location bar.
Assignee: nobody → ehsan
blocking2.0: ? → final+
Hmm, I assume this worked in FF3.6? If not, we should unblock.
Whiteboard: [softblocker]
This is a regression from 3.6, and it's happened because of bug 610682.

The reason is that after that bug, there is a text-align: right style set on the location bar's input element: <http://hg.mozilla.org/mozilla-central/rev/e98b94aa64fa#l1.21>.

The same behavior can be seen in the testcase in the URL field.

I'm not sure what we can do to fix this, though.  We want the default direction of the location bar to be LTR, but we want it right-aligned.  If there is a text-align style set on the input/textarea element, switching the direction using Ctrl+shift+X doesn't override that.  Of course, we could change this, but is this what we want to do?
Assignee: ehsan → nobody
Blocks: 610682
Component: Editor → Location Bar
Keywords: rtl
Product: Core → Firefox
QA Contact: editor → location.bar
Assignee: nobody → ehsan
David: can we style the anonymous div underneath text controls in the chrome browser.css stylesheet, the same way that we do in forms.css?
No, you can't.  You can only style native anonymous content in a UA stylesheet.
(In reply to comment #7)
> No, you can't.  You can only style native anonymous content in a UA stylesheet.

Would a hack which adds something like:

input[hackishAttr_IgnoreTextAlignment] > div[dir=rtl].anonymous-div {
  text-align: right;
}

input[hackishAttr_IgnoreTextAlignment] > div[dir=ltr].anonymous-div {
  text-align: left;
}

to forms.css and then adding the hackishAttr_IgnoreTextAlignment to the input box in the urlbar be acceptable?

The only other two ways that I can think of for addressing this is either adding:

input.uri-element-right-align:-moz-locale-dir(rtl) > div[dir=ltr].anonymous-div {
  /* ... */
}

to forms.css (this solution is kind of like the first one), or loading a UA stylesheet for the URL bar (which I'd really like to avoid, if possible).

The second-solution's advantage is that it won't affect web content, since :-moz-locale-dir only works on XUL documents, and remote XUL is dead.  Its draw-back is that it will regress if the classname is changed, for example (though, I will be adding the appropriate comments to signal people who look into the urlbar code in the future).  This is so far my favorite solution.

Any preferences?  :-)
Of course I guess we could write tests for this by reusing the urlbar binding...
You could use an @-moz-document rule to limit your new rules to browser.xul or chrome://*, right?  And then use whatever version of your rule you want.  I'd probably be ok with such a thing in forms.css.  More ok than the first roposal in comment 8, probably.
(In reply to comment #10)
> You could use an @-moz-document rule to limit your new rules to browser.xul or
> chrome://*, right?  And then use whatever version of your rule you want.  I'd
> probably be ok with such a thing in forms.css.  More ok than the first roposal
> in comment 8, probably.

Good suggestion (@-moz-document).  I'll combine that with comment 8's second suggestion then.
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #509351 - Flags: review?(roc)
Whiteboard: [softblocker] → [softblocker][has patch][needs review roc]
Comment on attachment 509351 [details] [diff] [review]
Patch (v1)

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -202,16 +202,20 @@ splitmenu {
>   -moz-binding: url(chrome://browser/content/urlbarBindings.xml#urlbar);
> }
> 
> .uri-element-right-align:-moz-locale-dir(rtl),
> html|input.uri-element-right-align:-moz-locale-dir(rtl),
> .ac-url-text:-moz-locale-dir(rtl),
> .ac-title:-moz-locale-dir(rtl) > description {
>   direction: ltr !important;
>+  /* Note that there is a hack in forms.css to make dynamic direction change
>+     correctly change the input field's text-align in RTL mode.  If you make
>+     changes here, you should change forms.css too.  See bug 630564 for more
>+     information. */
>   text-align: right !important;
> }

It looks like html|input.uri-element-right-align:-moz-locale-dir(rtl) isn't needed anymore here.
Dão, I think you're reading this wrong. The rule in forms.css does the opposite - it sets *left* alignment when the direction is ltr, if it was *forced by the user* (the dir attribute isn't set unless cmd_SwitchTextDirection is invoked).
(In reply to comment #14)
> (the dir attribute isn't set unless cmd_SwitchTextDirection is invoked).

That's the point I wasn't aware of. Looks like this could be addressed by removing [dir=rtl] from the code added to forms.css, though.
Maybe I'm not following.  The code in browser.css sets right alignment for an LTR input.  That's the opposite of what's done, with this patch, in forms.css (forced LTR -> left alignment, forced RTL -> right alignment).
Whiteboard: [softblocker][has patch][needs review roc] → [softblocker][needs landing]
Attached patch For check-inSplinter Review
Dao is right.  We don't need to reinforce the text-align: right case in this patch, so removing the [dir=rtl] rule works perfectly fine.
Attachment #509351 - Attachment is obsolete: true
I actually meant to only remove [dir=rtl] from the selector, rather than removing the whole rule. And then remove the line mentioned in comment 13, so that this stuff isn't spread across multiple mostly unrelated files.
(In reply to comment #18)
> I actually meant to only remove [dir=rtl] from the selector, rather than
> removing the whole rule. And then remove the line mentioned in comment 13, so
> that this stuff isn't spread across multiple mostly unrelated files.

Sure, makes sense.  Will land with that change.
http://hg.mozilla.org/mozilla-central/rev/108aec53a280
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [softblocker][needs landing] → [softblocker]
Target Milestone: --- → Firefox 4.0b12
(In reply to comment #20)
> http://hg.mozilla.org/mozilla-central/rev/108aec53a280

 .uri-element-right-align:-moz-locale-dir(rtl),
 html|input.uri-element-right-align:-moz-locale-dir(rtl),
 .ac-url-text:-moz-locale-dir(rtl),
 .ac-title:-moz-locale-dir(rtl) > description {
   direction: ltr !important;
-  text-align: right !important;
 }

You seem to have removed the wrong line. html|input.uri-element-right-align:-moz-locale-dir(rtl), is redundant, text-align: right !important; likely still needed.
(In reply to comment #21)
> (In reply to comment #20)
> > http://hg.mozilla.org/mozilla-central/rev/108aec53a280
> 
>  .uri-element-right-align:-moz-locale-dir(rtl),
>  html|input.uri-element-right-align:-moz-locale-dir(rtl),
>  .ac-url-text:-moz-locale-dir(rtl),
>  .ac-title:-moz-locale-dir(rtl) > description {
>    direction: ltr !important;
> -  text-align: right !important;
>  }
> 
> You seem to have removed the wrong line.
> html|input.uri-element-right-align:-moz-locale-dir(rtl), is redundant,

Except that forms.css doesn't set direction: ltr... So this either needs to be added, or said line from above needs to be kept after all. Anyway, text-align: right is needed for .uri-element-right-align, .ac-url-text and .ac-title, isn't it?
Just to make it perfectly clear what I think you should end up with:

.uri-element-right-align:-moz-locale-dir(rtl),
.ac-url-text:-moz-locale-dir(rtl),
.ac-title:-moz-locale-dir(rtl) > description {
  direction: ltr !important;
  text-align: right !important;
}

@-moz-document url-prefix(chrome://) {
  input.uri-element-right-align:-moz-locale-dir(rtl) > .anonymous-div {
    direction: ltr !important;
    text-align: right !important;
  }

  input.uri-element-right-align:-moz-locale-dir(rtl) > .anonymous-div[dir=ltr] {
    text-align: left !important;
  }
}
Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get right-aligned text because of their parents' alignment, right?

This is the idea here:

data:text/html,<div style="text-align:left"><p style="direction:rtl">foo</p></div>

The paragraph would be left-aligned.

Now, what you propose in comment 23 would also work, but I'm thinking that it's best for us not to set direction over there, as it would be interacting with the @dir attribute which is invisible to the people reading that stylesheet...

(This is all too hacky, and I wish there was a better way to do this, but if you still want me to do what you suggested in comment 23, I will reopen the bug and happily provide the patch.  :-) )
(In reply to comment #24)
> Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> right-aligned text because of their parents' alignment, right?

I guess so, although .uri-element-right-align seems to be a currently unused general-purpose class, so I don't know much about its potential parents.

> Now, what you propose in comment 23 would also work, but I'm thinking that it's
> best for us not to set direction over there, as it would be interacting with
> the @dir attribute which is invisible to the people reading that stylesheet...

Why would there be any interaction if html|input.uri-element-right-align:-moz-locale-dir(rtl) isn't part of the selector anymore?
(In reply to comment #25)
> (In reply to comment #24)
> > Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> > right-aligned text because of their parents' alignment, right?
> 
> I guess so, although .uri-element-right-align seems to be a currently unused
> general-purpose class, so I don't know much about its potential parents.

Is it?

ehsanakhgari:~/moz/mozilla-central [08:10:25]$ grep -rn uri-element-right-align browser/
browser/base/content/browser.css:205:.uri-element-right-align:-moz-locale-dir(rtl),
browser/base/content/browser.css:206:html|input.uri-element-right-align:-moz-locale-dir(rtl),
browser/base/content/urlbarBindings.xml:66:                      class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"

> > Now, what you propose in comment 23 would also work, but I'm thinking that it's
> > best for us not to set direction over there, as it would be interacting with
> > the @dir attribute which is invisible to the people reading that stylesheet...
> 
> Why would there be any interaction if
> html|input.uri-element-right-align:-moz-locale-dir(rtl) isn't part of the
> selector anymore?

I mean, the interaction between the dir attribute on the input element and on the anonymous div...
(In reply to comment #26)
> > > Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> > > right-aligned text because of their parents' alignment, right?
> > 
> > I guess so, although .uri-element-right-align seems to be a currently unused
> > general-purpose class, so I don't know much about its potential parents.
> 
> Is it?

> browser/base/content/urlbarBindings.xml:66:                     
> class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"

That's a html node. .uri-element-right-align alone doesn't match it, as the default namespace in browser.css is xul.

> I mean, the interaction between the dir attribute on the input element and on
> the anonymous div...

The input node has a dir attribute?
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Verified fixed based on the changeset from the posted patch 
http://hg.mozilla.org/mozilla-central/rev/108aec53a280

Tested on:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204
Firefox/4.0b12pre
w/ Persian Language Pack 3.0.2

http://hg.mozilla.org/mozilla-central/rev/847a825087f2
20110204030345
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
(In reply to comment #28)
> Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204
> Firefox/4.0b12pre

Well it works, but in RTL-mode first time after startup ctrl+shift+x switches only after second key press then it works normally until next restart.
(In reply to comment #30)
> (In reply to comment #28)
> > Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204
> > Firefox/4.0b12pre
> 
> Well it works, but in RTL-mode first time after startup ctrl+shift+x switches
> only after second key press then it works normally until next restart.

I'm not sure what we could do about it...  It actually works, but because the initial setup is weird (LTR and right-aligned), you don't see it in most cases.  You should see it on a URL like "http://www.google.com/"; the first time you press Ctrl+Shift+X, you should see it change to "/http://www.google.com", right?
(In reply to comment #27)
> (In reply to comment #26)
> > > > Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> > > > right-aligned text because of their parents' alignment, right?
> > > 
> > > I guess so, although .uri-element-right-align seems to be a currently unused
> > > general-purpose class, so I don't know much about its potential parents.
> > 
> > Is it?
> 
> > browser/base/content/urlbarBindings.xml:66:                     
> > class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"
> 
> That's a html node. .uri-element-right-align alone doesn't match it, as the
> default namespace in browser.css is xul.

Oh, I didn't know that.  I thought we match class names regardless of the namespace...

> > I mean, the interaction between the dir attribute on the input element and on
> > the anonymous div...
> 
> The input node has a dir attribute?

No, I'm talking about the anonymous div under it...
(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #28)
> > > Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204
> > > Firefox/4.0b12pre
> > 
> > Well it works, but in RTL-mode first time after startup ctrl+shift+x switches
> > only after second key press then it works normally until next restart.
> 
> I'm not sure what we could do about it...  It actually works, but because the
> initial setup is weird (LTR and right-aligned), you don't see it in most cases.
>  You should see it on a URL like "http://www.google.com/"; the first time you
> press Ctrl+Shift+X, you should see it change to "/http://www.google.com",
> right?

Yeah, exactly.
(In reply to comment #33)
> > I'm not sure what we could do about it...  It actually works, but because the
> > initial setup is weird (LTR and right-aligned), you don't see it in most cases.
> >  You should see it on a URL like "http://www.google.com/"; the first time you
> > press Ctrl+Shift+X, you should see it change to "/http://www.google.com",
> > right?
> 
> Yeah, exactly.

OK.  Like I said, I don't think that there is any way to make things better, but if you have suggestions, please file a bug and CC me on it too.  Thanks!  :-)
(In reply to comment #32)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > > > Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> > > > > right-aligned text because of their parents' alignment, right?
> > > > 
> > > > I guess so, although .uri-element-right-align seems to be a currently unused
> > > > general-purpose class, so I don't know much about its potential parents.
> > > 
> > > Is it?
> > 
> > > browser/base/content/urlbarBindings.xml:66:                     
> > > class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"
> > 
> > That's a html node. .uri-element-right-align alone doesn't match it, as the
> > default namespace in browser.css is xul.
> 
> Oh, I didn't know that.  I thought we match class names regardless of the
> namespace...

So this needs to be fixed, right?

> > > I mean, the interaction between the dir attribute on the input element and on
> > > the anonymous div...
> > 
> > The input node has a dir attribute?
> 
> No, I'm talking about the anonymous div under it...

I still don't see the problem.
(In reply to comment #35)
> (In reply to comment #32)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > > > Hmm.  .uri-element-right-align, .ac-url-text and .ac-title still get
> > > > > > right-aligned text because of their parents' alignment, right?
> > > > > 
> > > > > I guess so, although .uri-element-right-align seems to be a currently unused
> > > > > general-purpose class, so I don't know much about its potential parents.
> > > > 
> > > > Is it?
> > > 
> > > > browser/base/content/urlbarBindings.xml:66:                     
> > > > class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"
> > > 
> > > That's a html node. .uri-element-right-align alone doesn't match it, as the
> > > default namespace in browser.css is xul.
> > 
> > Oh, I didn't know that.  I thought we match class names regardless of the
> > namespace...
> 
> So this needs to be fixed, right?

Perhaps.  Nothing is *broken*, as far as I can see, so you should probably file a new bug and CC me, and I'll attach a patch removing the .uri-element-right-align line from the CSS file.

> > > > I mean, the interaction between the dir attribute on the input element and on
> > > > the anonymous div...
> > > 
> > > The input node has a dir attribute?
> > 
> > No, I'm talking about the anonymous div under it...
> 
> I still don't see the problem.

Every input element has an anonymous div inside it which has the displayed value of the field as a child text node.  Attributes set on the anonymous div are hidden from normal content, and are only accessible from C++.  The editor sets a dir attribute on the anonymous div, so that the content node's dir attribute doesn't change (as that change would be visible to web content).  This is a really bad hack.  In general we should avoid applying any non-default stuff to the anonymous div as much as possible.  An extra direction property there takes us one step further from an ideal future.  I hope this explains things.  If it doesn't, please ping me on irc so we can have a chat.  :-)
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.