631992 - [rtl] link tooltip appears as rtl

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tomer Cohen :tomer]

Attachment: screenshot (Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre)

As a follow-up to bug 402240, we now have the link target at the bottom of the window, similar to Google Chrome. While we still have some workarounds in place in order to keep it aligned well in necko.properties, which make strings such as "{RLE}Waiting for {LRE}www.iana.org{PDF}…{PDF}‎" for cases were the English equivalent is "Waiting for www.iana.org…", we can't control the directionality of URIs which appear while hovering links. 

Steps to reproduce: 
Using recent RTL nightly build of Firefox, type the following text in the location bar, and hover the link in the content area —
data:text/html,<a href="http://example.net">hover me</a>

Current Result:
The final Slash character should be printed on the right side of the URI, and not at the left. This occur because we use direction:rtl on that tooltip.

See screenshot to see it in action.

I'd be also very happy if we can at the same time remove some of the complexity of strings in necko.properties, as we add much too many control characters in order to make the string align well in RTL builds. 

http://mxr.mozilla.org/l10n-central/source/ar/netwerk/necko.properties?raw=1
http://mxr.mozilla.org/l10n-central/source/fa/netwerk/necko.properties?raw=1
http://mxr.mozilla.org/l10n-central/source/he/netwerk/necko.properties?raw=1

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[(no longer active)]

Once this is fixed, the #statusbar-display direction override should also be removed from the intl.css file for the ar, he and fa locales (in addition to what comment 0 says).

Comment 3

[Dão Gottwald [:dao]]

Did the status bar have this issue? I mostly restored what we did there. Maybe I missed some hidden status bar magic.

The patch in bug 631298 adds a type attribute, which should allow setting direction:ltr for URLs.

Comment 4

[Dão Gottwald [:dao]]

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

See comment 3

Comment 5

[(no longer active)]

(In reply to comment #3)
> Did the status bar have this issue? I mostly restored what we did there. Maybe
> I missed some hidden status bar magic.

It did in 3.6, but it was fixed when the preview was moved to the location bar.  But maybe that was accidental.  You certainly didn't miss any existing logic.  :-)

> The patch in bug 631298 adds a type attribute, which should allow setting
> direction:ltr for URLs.

OK, will write a new patch on top of that.

Comment 6

[(no longer active)]

Attachment: Patch (v2)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 510462 [details] [diff] [review]
Patch (v2)

Can you move this up directly after statuspanel[type=status]?

Comment 8

[(no longer active)]

(In reply to comment #7)
> Comment on attachment 510462 [details] [diff] [review]
> Patch (v2)
> 
> Can you move this up directly after statuspanel[type=status]?

Sure, will do when landing.

Comment 9

[(no longer active)]

Comment on attachment 510462 [details] [diff] [review]
Patch (v2)

Gavin, can you approve this please so that I can land it right after bug 631298?

Comment 10

[Mike Shaver (:shaver emeritus)]

17:23 < ehsan_mibbit> johnath: 631298 should be a blocker too I think since
                      631992 depends on it
17:24 <@gavin> 631992 should not be a blocker
17:24 <@gavin> not a regression from 3.6
17:25 < ehsan_mibbit> hmm
17:25 < ehsan_mibbit> johnath: actually I think gavin is right

Comment 11

[Johnathan Nightingale [:johnath]]

Ehsan and Gavin assert that this is not a regression from 3.6, and therefore not a blocker, much less a hardblocker.

Comment 12

[Simon Montagu :smontagu]

Attachment: Screenshot 3.6.13

This totally is a regression from 3.6 -- see attached screenshot. Since the patch has approval2.0 anyway, I won't bother rerequesting blocking, but please prioritize appropriately.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Created attachment 510956 [details]
> Screenshot 3.6.13
> 
> This totally is a regression from 3.6 -- see attached screenshot. Since the
> patch has approval2.0 anyway, I won't bother rerequesting blocking, but please
> prioritize appropriately.

Well, our RTL locales work around this by a hack in intl.css.  Without that hack, this is just as broken on branch as it is on trunk...

I've removed it from fa (by mistake) in http://hg.mozilla.org/l10n-central/fa/rev/8a40f844da8e.  If we don't get this fixed in time for Firefox 4, we should restore the #status-display thing there.

Dao, what do you say we take my first patch here and modify the patch in bug 631298 to revert the change that my patch makes to updateStatusText?  There is no real reason why this should depend on bug 631298, except for implementation details...

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c1fce77c84d8

Comment 15

[(no longer active)]

I removed the hack which is now unneeded from l10n-central/ar:

http://hg.mozilla.org/l10n-central/ar/rev/0aa81704277c

Anas, please include this revision in the sign-off for b12.  he and fa were fine, and did not need modification.

Comment 16

[Anas Husseini [:linostar]]

Attachment: Screenshot for Arabic build of fx4 b12pre

Comment 17

[Anas Husseini [:linostar]]

(In reply to comment #15)
> I removed the hack which is now unneeded from l10n-central/ar:
> 
> http://hg.mozilla.org/l10n-central/ar/rev/0aa81704277c
> 
> Anas, please include this revision in the sign-off for b12.  he and fa were
> fine, and did not need modification.

The bug seems fixed before applying the change in intl.css (I am referring here to my local build of firefox). So is this natural? Furthermore, I noticed that the url box (although ltr) is right-justified, is it the same way with fa and he, or is it a bug?

Comment 18

[Simon Montagu :smontagu]

(In reply to comment #17)
> Furthermore, I noticed that
> the url box (although ltr) is right-justified, is it the same way with fa and
> he, or is it a bug?

That was a change made in bug 610682

Comment 19

[(no longer active)]

(In reply to comment #17)
> (In reply to comment #15)
> > I removed the hack which is now unneeded from l10n-central/ar:
> > 
> > http://hg.mozilla.org/l10n-central/ar/rev/0aa81704277c
> > 
> > Anas, please include this revision in the sign-off for b12.  he and fa were
> > fine, and did not need modification.
> 
> The bug seems fixed before applying the change in intl.css (I am referring here
> to my local build of firefox). So is this natural?

yes, this is the whole point behind this bug.  We should kill all intl.css hacks, one by one!  :-)

Comment 20

[Teodosia Pop]

Verified using Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12.