Closed Bug 631992 Opened 9 years ago Closed 9 years ago

[rtl] link tooltip appears as rtl

Categories

(Firefox :: General, defect)

x86
Linux
defect
Not set

Tracking

()

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

People

(Reporter: tomer, Assigned: ehsan)

References

()

Details

(Keywords: rtl)

Attachments

(4 files, 1 obsolete file)

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
Blocks: 541656
blocking2.0: --- → ?
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #510460 - Flags: review?(dao)
Attachment #510460 - Flags: approval2.0?
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).
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.
Assignee: ehsan → nobody
Status: ASSIGNED → NEW
Depends on: 631298
Comment on attachment 510460 [details] [diff] [review]
Patch (v1)

See comment 3
Attachment #510460 - Flags: review?(dao)
Attachment #510460 - Flags: approval2.0?
(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.
Assignee: nobody → ehsan
Attached patch Patch (v2)Splinter Review
Attachment #510460 - Attachment is obsolete: true
Attachment #510462 - Flags: review?(dao)
Comment on attachment 510462 [details] [diff] [review]
Patch (v2)

Can you move this up directly after statuspanel[type=status]?
Attachment #510462 - Flags: review?(dao) → review+
(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.
Whiteboard: [address comment 7 before landing!]
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?
Attachment #510462 - Flags: approval2.0?
Status: NEW → ASSIGNED
blocking2.0: ? → final+
Whiteboard: [address comment 7 before landing!] → [address comment 7 before landing!][hardblocker]
Attachment #510462 - Flags: approval2.0? → approval2.0+
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
blocking2.0: final+ → -
Whiteboard: [address comment 7 before landing!][hardblocker] → [address comment 7 before landing!]
Ehsan and Gavin assert that this is not a regression from 3.6, and therefore not a blocker, much less a hardblocker.
Attached image 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.
(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...
http://hg.mozilla.org/mozilla-central/rev/c1fce77c84d8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [address comment 7 before landing!]
Target Milestone: --- → Firefox 4.0b12
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.
(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?
(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
(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!  :-)
Verified using Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.