Closed Bug 473440 Opened 11 years ago Closed 11 years ago

LTR edit box context menu appears LTR in RTL locales

Categories

(Toolkit :: XUL Widgets, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: ehsan, Assigned: ehsan)

Details

(4 keywords)

Attachments

(2 files, 2 obsolete files)

STR:

1. Load an RTL Firefox (he, fa or ar) or use Force RTL <https://addons.mozilla.org/en-US/firefox/addon/7438> to make the UI RTL.
2. Load the attached test case.
3. Right click on the text box.

Expected: The context menu should appear as RTL.

Result: The context menu appears LTR.

This is most prominently observed in the location bar for RTL locales (which set the location bar to be LTR).

The fix probably is to make sure the direction of the edit box's context menu comes from the direction of the parent of the textbox, rather than the textbox itself.
Attached patch Patch (v1) (obsolete) — Splinter Review
Simple fix.  Make textbox context menus obey the global direction, not the textbox direction.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #356931 - Flags: review?(dao)
Hm, can we just fix the location bar so that we make the input rather than the input-box ltr?
I guess the more relevant question is: Are there other cases where doing this in toolkit would be helpful?
(In reply to comment #3)
> I guess the more relevant question is: Are there other cases where doing this
> in toolkit would be helpful?

Yes.  Another quick example would be the Home page box in the main prefpane.  Taking a look around, commonDialog.xul has the same problem.  This really belongs in toolkit, since any toolkit-based app is potentially subject to this problem, and it is quite common in RTL UIs to have LTR text boxes (which contain mostly English data, or things like URLs, etc.)
Comment on attachment 356931 [details] [diff] [review]
Patch (v1)

please add a class name to the menupopup and use that in the css files.
(In reply to comment #5)
> (From update of attachment 356931 [details] [diff] [review])
> please add a class name to the menupopup and use that in the css files.

You mean something like class="&locale.dir;"?  Or adding a class programatically by reading the computed style?

Can I ask why you're suggesting this?
I mean a class name like "textbox-contextmenu".
Attached patch Patch (v2) (obsolete) — Splinter Review
Ah, right.  Here you are.  :-)
Attachment #356931 - Attachment is obsolete: true
Attachment #356953 - Flags: review?(dao)
Attachment #356931 - Flags: review?(dao)
Comment on attachment 356953 [details] [diff] [review]
Patch (v2)

>+      <xul:menupopup anonid="input-box-contextmenu" class="textbox-contextmenu" chromedir="&locale.dir;"

This line gets quite long, should wrap it.

> .textbox-input-box menupopup {
>   cursor: default;

Please also make this use the new class.

>+menupopup.textbox-contextmenu[chromedir="rtl"] {

No need for the "menupopup" part.
(In reply to comment #9)
> >+menupopup.textbox-contextmenu[chromedir="rtl"] {
> 
> No need for the "menupopup" part.

Oh, and this belongs in xul.css.
Attached patch Patch (v2.1)Splinter Review
Addressed comments.
Attachment #356953 - Attachment is obsolete: true
Attachment #356954 - Flags: review?(dao)
Attachment #356953 - Flags: review?(dao)
Attachment #356954 - Flags: review?(enndeakin)
Attachment #356954 - Flags: review?(dao)
Attachment #356954 - Flags: review+
Attachment #356954 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/9750c237c3ff
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 356954 [details] [diff] [review]
Patch (v2.1)

Simple and low-risk patch.
Attachment #356954 - Flags: approval1.9.1?
Flags: in-litmus?
--- a/toolkit/content/widgets/textbox.xml
+++ b/toolkit/content/widgets/textbox.xml
@@ -487,16 +487,18 @@
+                     class="textbox-contextmenu"
+                     chromedir="&locale.dir;"

Shouldn't you add something like the following to textbox.xml?

<!DOCTYPE bindings [
  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
  %globalDTD;
]>

Instead of assuming that all consumers would include global.dtd?
Aaagh, Thanks. I think I need my glasses changed.
Comment on attachment 356954 [details] [diff] [review]
Patch (v2.1)

a191=beltzner
Attachment #356954 - Flags: approval1.9.1? → approval1.9.1+
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.