Closed Bug 1313751 Opened 3 years ago Closed 3 years ago

Fix unicode bidi for URL of tooltip in RTL locales

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: magicp.jp, Assigned: Gijs)

References

Details

Attachments

(4 files)

Steps to Reproduce:
1. Start any versions Firefox in RTL locales (e.g. Arabic)
2. Open "https://www.mozilla.org/en-US/contribute/"
3. Bookmark current page
4. Mouse hover on urlbar and this bookmark > Check URL of tooltip

Actual Results:
"/https://www.mozilla.org/en-US/contribute"
 ^
Expected Results:
"https://www.mozilla.org/en-US/contribute/"
                                         ^
Do we do this wrong for other tooltips or tooltip-like items, such as the link hovering tooltip, or tooltips on the history or "restore closed tabs" menus / panels ?
Flags: needinfo?(magicp.jp)
Do you think need to modify each tooltips?
Flags: needinfo?(magicp.jp)
(In reply to magicp from comment #2)
> Do you think need to modify each tooltips?

Well, probably - if we do this wrong in other tooltips, which was my question. It would be helpful if you could answer that. :-)
Flags: needinfo?(magicp.jp)
We should fix them if Firefox supports with RTL locales. I think tooltips should be displayed original text simply.
Flags: needinfo?(magicp.jp)
(In reply to magicp from comment #5)
> We should fix them if Firefox supports with RTL locales. I think tooltips
> should be displayed original text simply.

But you've not answered the question... Of course we should fix this, what I'm trying to find out is whether we need to basically fix 1 tooltip that is broken compared to 5 others that are working correctly, or whether ALL of them are broken, and what we really want is to make XUL automatically set direction:auto on tooltips or something like that. For this, it would be *really helpful* if you could clarify if it's *only* the bookmarks tooltips that are broken, or also the other ones.
Flags: needinfo?(magicp.jp)
I don't know how to find all. But if all tooltips are controlled by global/popup.css, you can change direction. can't you?
Flags: needinfo?(magicp.jp)
(In reply to magicp from comment #7)
> I don't know how to find all.

Sure, but the history and synced tabs ones would be a good start.

> But if all tooltips are controlled by
> global/popup.css, you can change direction. can't you?

"Maybe."

It might not actually be appropriate to change the direction of *all* of these items.

Neil, do you have ideas about what would be the easiest solution here?
Flags: needinfo?(enndeakin)
(In reply to :Gijs Kruitbosch from comment #8)

> It might not actually be appropriate to change the direction of *all* of
> these items.

Do you know any wrong cases?
(In reply to magicp from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> 
> > It might not actually be appropriate to change the direction of *all* of
> > these items.
> 
> Do you know any wrong cases?

The "failure" here is because the direction is rtl and so the text direction of non-directional elements like '/' is wrong and they end up on the wrong side.

I don't know if it's possible there are consumers that display RTL-mixed-with-non-directional-or-LTR text that depend on the direction in the tooltip being RTL that would no longer display their (within-RTL) non-directional characters (or LTR elements) correctly if we changed the direction from RTL to auto.

In any case, changing the direction: CSS for *all* tooltips is a risky change. It's easier and less risky to change things 1 tooltip at a time, but there might be a lot of tooltips to change. I don't know.
Attached image tooltips test cases.png
I confirmed all of tooltips in xul files. Then grouping them and selected some tooltips for test cases. It works well and I hope it will help you. Please find attached file. Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
Is this the case that the tooltips that display uris need to use direction: ltr ? (Or use the class uri-element)
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #12)
> Is this the case that the tooltips that display uris need to use direction:
> ltr ? (Or use the class uri-element)

Maybe. Not all of them only display a URI, though. It's possible the ones that display both a description and a URI would need to be refactored to use a tooltip element with an explicit <label> that we can give that class, or something...
I believe the attached patch fixes everything in attachment 8805921 [details] except for the URL bar tooltip, which is more work because it uses tooltiptext, and we'd need to update it to use a different tooltip element in order to be able to style it selectively. I don't think we can blanket-style all |tooltip| elements in Firefox with unicode-bidi: -moz-plaintext and hope for the best.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8808713 [details]
Bug 1313751 - fix direction of URLs and/or page titles in a number of UI elements,

https://reviewboard.mozilla.org/r/91460/#review91294

::: browser/base/content/browser.css:626
(Diff revision 2)
>  }
>  
> +.bookmark-item > label {
> +  /* ensure we use the direction of the bookmarks label instead of the
> +   * browser locale */
> +  unicode-bidi: -moz-plaintext;

The -moz prefix shouldn't be needed.
Comment on attachment 8808713 [details]
Bug 1313751 - fix direction of URLs and/or page titles in a number of UI elements,

https://reviewboard.mozilla.org/r/91460/#review91750
Attachment #8808713 - Flags: review?(enndeakin) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/948ab12433f4
fix direction of URLs and/or page titles in a number of UI elements, r=enndeakin
Blocks: 1317264
Hrmpf, after reviewing the screenshot one more time, I think I'm also not fixing the bhTooltip's main label/title/thing with this patch. Rebuilding to confirm, if I'm right I'll file another followup.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1317268
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/948ab12433f4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I have reproduced the bug in Nightly 52.0a1 (2016-10-28) with the instruction from comment 0 and Linux 64 Bit

Bug is fixed now on Latest Beta 53.0b3 (Build ID:20170316115347)
User Agent:Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

[testday-20170317]
Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.