427739 - in RTL builds, popups for site identity and star hang the wrong way

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[mcdavis941 (sporadically reading bugmail)]

In RTL (hebrew) builds, the site identity and star popups hand toward the outsides of the window, when they should hang toward to middle of the location bar as they do in LTR builds.

Screenshot compares the layout in LTR vs RTL builds.

Using:

Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.9pre) Gecko/2008040706 Minefield/3.0pre

Comment 1

[mcdavis941 (sporadically reading bugmail)]

Attachment: screenshot comparing LTR and RTL builds

Comment 2

[Simon Montagu :smontagu]

I thought I had seen this before, but I can't now find another report.

Comment 3

[(no longer active)]

This is an RTl bug, not a Hebrew-specific bug.

Comment 4

[(no longer active)]

I don't know of any dupe, and couldn't find one after an extensive search.  I'll be posting a patch here.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Simple patch to reverse the direction in RTL mode.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 319791 [details] [diff] [review]
Patch (v1)

Please add a comment as to why this is right, e.g. that "they should hang toward to middle of the location bar".

>+        if (window.getComputedStyle(gBrowser, null).direction == "rtl")

Is gBrowser randomly chosen? Can you use document.documentElement instead, or read the chromedir attribute from somewhere?

Comment 7

[(no longer active)]

(In reply to comment #6)
> (From update of attachment 319791 [details] [diff] [review])
> Please add a comment as to why this is right, e.g. that "they should hang
> toward to middle of the location bar".

Because the location bar stays LTR for the most part.  The only thing which gets affected by the RTL setting is drop-down arrow for the location bar, which is an attempt to match the style for drop-down lists in RTL mode.

If they hang toward outside of the location bar, it wouldn't feel consistent with the LTR-ness of the location bar.

> >+        if (window.getComputedStyle(gBrowser, null).direction == "rtl")
> 
> Is gBrowser randomly chosen? Can you use document.documentElement instead, or
> read the chromedir attribute from somewhere?

Hmmm, I could do that, but reading the computed style should be more robust, I guess, because that's what actually gets used during run-time.  If the browser element is rasing any concerns, I could change that.  Would you suggest document.documentElement is better?  BTW, I used <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/tabbrowser.&rev=1.271&mark=1167-1169> as a pattern.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 319791 [details] [diff] [review] [details])
> > Please add a comment as to why this is right, e.g. that "they should hang
> > toward to middle of the location bar".
> 
> Because the location bar stays LTR for the most part.  The only thing which
> gets affected by the RTL setting is drop-down arrow for the location bar, which
> is an attempt to match the style for drop-down lists in RTL mode.
> 
> If they hang toward outside of the location bar, it wouldn't feel consistent
> with the LTR-ness of the location bar.

So if the location bar was rtl, this wouldn't be an issue?
What I meant, though, is: Add a comment to the patch.

> Hmmm, I could do that, but reading the computed style should be more robust, I
> guess, because that's what actually gets used during run-time.  If the browser
> element is rasing any concerns, I could change that.  Would you suggest
> document.documentElement is better?

Yes, document.documentElement would make more sense since the location bar isn't a children of gBrowser.
chromdir would be faster, and I don't think there's a scenario where it would be incorrect.

Comment 9

[(no longer active)]

Attachment: Patch (v1.1)

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 319791 [details] [diff] [review] [details] [details])
> > > Please add a comment as to why this is right, e.g. that "they should hang
> > > toward to middle of the location bar".
> > 
> > Because the location bar stays LTR for the most part.  The only thing which
> > gets affected by the RTL setting is drop-down arrow for the location bar, which
> > is an attempt to match the style for drop-down lists in RTL mode.
> > 
> > If they hang toward outside of the location bar, it wouldn't feel consistent
> > with the LTR-ness of the location bar.
> 
> So if the location bar was rtl, this wouldn't be an issue?

Yes, it wouldn't, because then for example the site identity button would appear at the right side of the location bar, and the popup would hang toward the left, and therefore, the middle of the location bar, as expected.  The same logic applies to the bookmark properties dialog.

> What I meant, though, is: Add a comment to the patch.

Oh, sure.  Sorry for the misunderstanding.

> > Hmmm, I could do that, but reading the computed style should be more robust, I
> > guess, because that's what actually gets used during run-time.  If the browser
> > element is rasing any concerns, I could change that.  Would you suggest
> > document.documentElement is better?
> 
> Yes, document.documentElement would make more sense since the location bar
> isn't a children of gBrowser.
> chromdir would be faster, and I don't think there's a scenario where it would
> be incorrect.

OK, I used the chromedir attribute of urlbar in the new version of the patch.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 320362 [details] [diff] [review]
Patch (v1.1)

>+        if (document.getElementById("urlbar").getAttribute("chromedir") == "rtl")

>+    if (document.getElementById("urlbar").getAttribute("chromedir") == "rtl")

use gURLBar

Comment 11

[(no longer active)]

Attachment: Patch (v1.2)

(In reply to comment #10)
> (From update of attachment 320362 [details] [diff] [review])
> >+        if (document.getElementById("urlbar").getAttribute("chromedir") == "rtl")
> 
> >+    if (document.getElementById("urlbar").getAttribute("chromedir") == "rtl")
> 
> use gURLBar

Done.  Carrying over the r+ flag.

Comment 12

[(no longer active)]

This is ready for mozilla-central.

Comment 13

[(no longer active)]

Without this being fixed, RTL locales suffer from the weird directions.  I think we should take this on 3.0.1, especially since it has a low-risk patch with r+.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/0de3a2a370a7

Comment 15

[Samuel Sidler (old account; do not CC)]

Comment on attachment 320382 [details] [diff] [review]
Patch (v1.2)

Can we get a test for this patch before approving for 1.9?

Comment 16

[(no longer active)]

(In reply to comment #15)
> (From update of attachment 320382 [details] [diff] [review])
> Can we get a test for this patch before approving for 1.9?

You mean an automated test?  I'm not sure how I can test this as a chrometest...  I assume that you're not looking for a litmus test of course...

Comment 17

[(no longer active)]

Looking for help from sayrer.  I need to know how I can go about creating an automated test for this patch.  Thanks!

Comment 18

[Dão Gottwald [:dao]]

In a browser chrome test you could set the urlbar's chromedir attribute to "rtl" (given an ltr build), initiate a click on the identity button and check that the identity popup hangs the _wrong_ way (by comparing boxObject.screenX values). However, this would be pretty fragile and I wouldn't consider such a test overly useful. E.g. it would fail for no good reason if someone reimplements this using the computed style rather than the chromedir attribute.

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 320382 [details] [diff] [review]
Patch (v1.2)

Moving approval request out to 1.9.0.3.

Comment 20

[(no longer active)]

(In reply to comment #18)
> In a browser chrome test you could set the urlbar's chromedir attribute to
> "rtl" (given an ltr build), initiate a click on the identity button and check
> that the identity popup hangs the _wrong_ way (by comparing boxObject.screenX
> values). However, this would be pretty fragile and I wouldn't consider such a
> test overly useful. E.g. it would fail for no good reason if someone
> reimplements this using the computed style rather than the chromedir attribute.

I agree that it would be too fragile.  I can go ahead and write this test, but I guess a Litmus test would serve better here, especially with the Force RTL extension <https://addons.mozilla.org/addon/7438>.

What do you think, Samuel?

Comment 21

[Samuel Sidler (old account; do not CC)]

Let's go ahead and just get a Litmus test for this in lieu of an automated test.

Comment 22

[(no longer active)]

Here's a shot at a Litmus test:

Testgroup: 3.0 Full Functional Tests (56)
Subgroup: UI Elements (890)
Testcase: Location bar popup direction in RTL mode.
Product: Firefox
Branch: 3.0
Author email: [e-mail]
Steps to perform:
1. If you are using an RTL version of the browser (Hebrew, Arabic or Persian), skip to step 4.
2. If you don't already have the Force RTL extension, install it from <https://addons.mozilla.org/addon/7438> (requires login to AMO) and restart your browser.
3. From the Tools menu, select "Force RTL Direction".
4. Click on the site identity button.
5. Click on the star button if the current page is not already bookmarked.
6. Click on the star button.
Expected Results:
After step 4, the identity dialog which pops out should hang to the right side of the identity button you clicked, like the first picture of https://bugzilla.mozilla.org/attachment.cgi?id=314317.  If the identity dialogs looks like the third picture, then the test has failed.
After step 6, the bookmark properties dialog which pops out should hang to the left side of the identity button you clicked, like the second picture of https://bugzilla.mozilla.org/attachment.cgi?id=314317.  If the bookmark properties dialogs looks like the fourth picture, then the test has failed.
Regression Bug ID#: 427739

I'm not sure who to CC in order to get this in Litmus, so I'm trying Marcia...

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

Ehsan: testcase added as https://litmus.mozilla.org/show_test.cgi?id=7036.

Could you bump the version compatibility of https://addons.mozilla.org/en-US/firefox/addon/7438 to work with the nightlies?  ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/ doesn't seem to have any RTL-enabled builds...thanks!

Comment 24

[(no longer active)]

(In reply to comment #23)
> Ehsan: testcase added as https://litmus.mozilla.org/show_test.cgi?id=7036.

Thanks for adding the testcase.

> Could you bump the version compatibility of
> https://addons.mozilla.org/en-US/firefox/addon/7438 to work with the nightlies?
>  ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/ doesn't
> seem to have any RTL-enabled builds...thanks!

Sure, done.

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080915032512 Minefield/3.1b1pre and the Force RTL add-on.

Comment 26

[Samuel Sidler (old account; do not CC)]

Comment on attachment 320382 [details] [diff] [review]
Patch (v1.2)

We're opting not to take this in a maintenance release.