Tab labels fade in UI direction regardless of title direction

VERIFIED FIXED in Firefox 54

Status

()

defect
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: salar2k, Assigned: Ehsan)

Tracking

(Depends on 1 bug, {regression, rtl})

53 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54+ fixed, firefox55+ verified)

Details

()

Attachments

(6 attachments)

Reporter

Description

2 years ago
Posted image ff fade title.jpg
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Title labels fade effect ends in wrong direction for Rtl text titles.
Reporter

Updated

2 years ago
Component: Untriaged → Tabbed Browser
Reporter

Comment 1

2 years ago
The sample text for testing. "این یک متن آزمایشی است"
Blocks: 658467
Status: UNCONFIRMED → NEW
Ever confirmed: true
Dão, can you investigate?

I guess this is too late for 53 now, which is a real shame. :-\
Flags: needinfo?(dao+bmo)
Keywords: regression, rtl
We currently pick the fading direction based on the UI locale rather than the individual labels.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #3)
> We currently pick the fading direction based on the UI locale rather than
> the individual labels.

So this means that for users that use RTL Firefoxen, LTR labels will fade in the wrong place, too? That sounds... not good. :-\
(In reply to :Gijs from comment #4)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > We currently pick the fading direction based on the UI locale rather than
> > the individual labels.
> 
> So this means that for users that use RTL Firefoxen, LTR labels will fade in
> the wrong place, too? That sounds... not good. :-\

Correct...
Summary: Tab labels fade in wrong end for Rtl content → Tab labels fade in UI direction regardless of title direction
Is this new in 53, or does it affect 52/esr52?  Is it a (recent) regression or just a bug?  Thanks!
Flags: needinfo?(salar2k)
(In reply to Randell Jesup [:jesup] from comment #6)
> Is this new in 53, or does it affect 52/esr52?  Is it a (recent) regression
> or just a bug?  Thanks!

It's a regression in 53, from bug 658467. Doesn't affect 52/esr52.
Flags: needinfo?(salar2k)
Duplicate of this bug: 1358424

Updated

2 years ago
Duplicate of this bug: 1358578
[Tracking Requested - why for this release]:

For users in RTL locales, this makes tab titles hard to read. Unfortunately, this went out in 53. :/
Let's track this for 54 since it's a recent regression. 
Dão, can you help find someone to work on this, since we have a chance to fix it for 54?
Flags: needinfo?(dao+bmo)
Masatoshi, do you know if/how chrome JS can easily detect the direction of a string?
Flags: needinfo?(dao+bmo) → needinfo?(VYV03354)
I don't know.

But what should happen if the string is mixed-direction?
Flags: needinfo?(VYV03354)
(In reply to Dão Gottwald [::dao] from comment #12)
> Masatoshi, do you know if/how chrome JS can easily detect the direction of a
> string?

smontagu maybe?

(In reply to Masatoshi Kimura [:emk] from comment #13)
> But what should happen if the string is mixed-direction?

For that case there's probably no perfect solution.
Flags: needinfo?(smontagu)
I guess we can try a simplified version of directionality algorithm defined in the HTML spec: https://html.spec.whatwg.org/multipage/dom.html#the-directionality

We can try something like:
1. check whether the document element has dir attribute, if it does, and it's one of ltr or rtl, we use it and apply it to the label, otherwise
2. check the content of title as if it is in a textarea with dir=auto, and apply the direction to the label as well.

That should work for majority of the cases.

If there is no such util function, I guess adding one isn't very hard.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> I guess we can try a simplified version of directionality algorithm defined
> in the HTML spec:
> https://html.spec.whatwg.org/multipage/dom.html#the-directionality
> 
> We can try something like:
> 1. check whether the document element has dir attribute, if it does, and
> it's one of ltr or rtl, we use it and apply it to the label, otherwise

We only have the string, no document.

> 2. check the content of title as if it is in a textarea with dir=auto, and
> apply the direction to the label as well.

This sounds like it might work, although I don't know what bidirectional character types AL, R and L are: "If the element's value contains a character of bidirectional character type AL or R, and there is no character of bidirectional character type L anywhere before it in the element's value, then the directionality of the element is 'rtl'. [BIDI]"
(In reply to Dão Gottwald [::dao] from comment #16)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> > We can try something like:
> > 1. check whether the document element has dir attribute, if it does, and
> > it's one of ltr or rtl, we use it and apply it to the label, otherwise
> 
> We only have the string, no document.

Why not? Well, I guess it doesn't matter too much, though. Only having the second step should address majority of cases.

> > 2. check the content of title as if it is in a textarea with dir=auto, and
> > apply the direction to the label as well.
> 
> This sounds like it might work, although I don't know what bidirectional
> character types AL, R and L are: "If the element's value contains a
> character of bidirectional character type AL or R, and there is no character
> of bidirectional character type L anywhere before it in the element's value,
> then the directionality of the element is 'rtl'. [BIDI]"

That is something defined in Unicode [1]. We should already have functions for that inside C++ because we need that anyway.

[1] http://www.unicode.org/reports/tr9/#Bidirectional_Character_Types
So actually the C++ function for doing the second step can be find from the directionality computation code [1], we can just expose that to script somehow.

I guess we may still want to take the page direction into account because the first strong character may be misleading. For example, a title |"<R>"'s meaning| in which <R> is a character typed "R" would be treated as RTL with the algorithm because the punctunation is neutral and the first strong character is <R>. But apparently the main sentence is still English, which should be LTR. In that case, if we check the page direction first, the author would still have a chance to fix it via setting the page direction properly.

[1] https://dxr.mozilla.org/mozilla-central/rev/0b255199db9d6a6f189b89b7906f99155bde3726/dom/base/DirectionalityUtils.cpp#296-378
This is a special case of bug 703242.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> I guess we can try a simplified version of directionality algorithm defined
> in the HTML spec:
> https://html.spec.whatwg.org/multipage/dom.html#the-directionality
> 
> We can try something like:
> 1. check whether the document element has dir attribute, if it does, and
> it's one of ltr or rtl, we use it and apply it to the label, otherwise
> 2. check the content of title as if it is in a textarea with dir=auto, and
> apply the direction to the label as well.

The HTML spec[1] already defines what we should do:

"User agents should use the document's title when referring to the document in their user interface. When the contents of a title element are used in this way, the directionality of that title element should be used to set the directionality of the document's title in the user interface."


(In reply to Dão Gottwald [::dao] from comment #16)

> We only have the string, no document.

So the code that passes the contents of the title element needs to compute the directionality of the element and pass that too.

[1] https://html.spec.whatwg.org/multipage/semantics.html#the-title-element
Depends on: 703242
Flags: needinfo?(smontagu)
Using the directionality of the element is what I initially thought, but I suspect whether that works in common cases. Also that doesn't match what Blink does actually.

The HTML spec really has a strict definition of how directionality should be computed, and if a document doesn't have any dir attribute, it would have ltr direction by default, since it is neither a textarea nor an input. Probably we should change the spec somehow.

If we change the spec to treat <title> like <textarea> etc., then I agree that we should probably just use the directionality of it.
Actually it seems Chrome simply uses the first strong character as the direction. If that is easy, we can probably just do it for now.
(In reply to Simon Montagu :smontagu from comment #19)
> So the code that passes the contents of the title element needs to compute
> the directionality of the element and pass that too.

How hard would it be to tack this info onto the DOMTitleChanged event?
Filed a WHATWG issue: whatwg/html#2648
FWIW, I think things should still be easy even if we need to check the directionality of title element. We just need to prepend a proper implicit marker (U+200E for LTR, U+200F for RTL) to the title string when we pass it to chrome.
Mark 54 won't fix as it's late for Beta54.
It seems we don't have agreement directionality of <title>, but I think the current situation is clearly unfortunate for Firefox, so I think we should just use first strong character for now, as it would align our behavior to other browsers (and probably our previous behavior), which is at least much better than now.

I think the way to fix it is to set "dir" attribute of tab in _setTabLabel of tabbrowser [1]. And to get the direction from the text, we need to expose a function from C++ to JavaScript for GetDirectionFromText in DirectionalityUtils.cpp [2].

This should be an easy fix, but it is not clear to me how should I add the glue interface from C++ to JavaScript... I guess we would need a new interface?


[1] https://dxr.mozilla.org/mozilla-central/rev/701e0ebc2b4b7ae57248e44fd06278e5309e1a05/browser/base/content/tabbrowser.xml#1517-1518
[2] https://dxr.mozilla.org/mozilla-central/rev/0b255199db9d6a6f189b89b7906f99155bde3726/dom/base/DirectionalityUtils.cpp#305-307
ni? for the question in comment 26.
Flags: needinfo?(dao+bmo)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26)
> It seems we don't have agreement directionality of <title>, but I think the
> current situation is clearly unfortunate for Firefox, so I think we should
> just use first strong character for now, as it would align our behavior to
> other browsers (and probably our previous behavior), which is at least much
> better than now.
> 
> I think the way to fix it is to set "dir" attribute of tab in _setTabLabel
> of tabbrowser [1]. And to get the direction from the text, we need to expose
> a function from C++ to JavaScript for GetDirectionFromText in
> DirectionalityUtils.cpp [2].
> 
> This should be an easy fix, but it is not clear to me how should I add the
> glue interface from C++ to JavaScript... I guess we would need a new
> interface?

Yeah, some new XPCOM interface I suppose, or maybe it would make sense to add this to nsIDOMWindowUtils (probably not since it doesn't have anything to do with the DOM window) or some other existing interface. I usually don't work on native code so I'm not the right person to give you exact instructions here...

As suggested earlier, adding the direction info to the DOMTitleChange event would work too for the purpose of this bug, but exposing GetDirectionFromText to chrome JS is probably a good idea in general.
Flags: needinfo?(dao+bmo)
Duplicate of this bug: 1370043
Assignee

Comment 30

2 years ago
I'll take this, and will write a patch tomorrow.

To answer Dao's question, it's both easy and very difficult: this is where we dispatch DOMTitleChange from: <https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/base/nsDocument.cpp#7044> and as you'll note we're calling GetTitle() there which gets to <https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/base/nsDocument.cpp#6943>.  |title| there points to our title element!  So we could just read the direction off of it quite easily.  The difficulty is where to put it.  The event that we dispatch is a simple event which doesn't have any extra properties for storing the direction information.  I suppose I could make it a custom event but besides the tiny risk of compat issues if some code depends on the exacts type of this object, it's kind of a pain to work with custom events in C++ because you'd need to go through JSAPI.  And beyond that I'd need to create a new WebIDL event type which at least would be machine-generated code not manually written code but still lots of unnecessary bloat IMO.  And as an RTL user, arguably using the first strong character is actually the better option here anyway since this is ultimately text that we are displaying to the user inside our own UI in our own way (with half of the text faded out) so the author would have no way to guess the implications of screwing up a runaway dir attribute on a <head> element somewhere.

I'm going to expose GetDirectionFromText to JS for the purposes of this bug.  Need to think of a good place to expose it on.  Suggestions welcome.
Assignee: nobody → ehsan
On alternative is to add a new chrome-only attribute to Document for getting direction of title. That would probably be the simplest approach for C++ side. And that would make us able to change its semantic easily in the future if we want to follow directionality defined in HTML spec.
Assignee

Comment 32

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #31)
> On alternative is to add a new chrome-only attribute to Document for getting
> direction of title. That would probably be the simplest approach for C++
> side. And that would make us able to change its semantic easily in the
> future if we want to follow directionality defined in HTML spec.

That's not going to be helpful here, since this information is needed in the parent process and right now this information is delivered to the parent through the DOMTitleChanged event <https://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/toolkit/content/widgets/remote-browser.xml#447>, so see my previous comment.
Assignee

Comment 33

2 years ago
Attachment #8876282 - Flags: review?(michael) → review+
Attachment #8876283 - Flags: review?(mconley) → review+

Comment 36

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6cc6c65a79
Part 1: Expose an nsIDOMWindowUtils method for retrieving the direction of a piece of text based on the first-strong character algorithm; r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/7533679209b1
Part 2: Switch the direction of the tab title fade-out effect based on the directionality of the title string; r=mconley
Assignee

Comment 37

2 years ago
[Tracking Requested - why for this release]:
I made a case to the release management team that this is a really bad regression that makes tab titles unusable for RTL language pages, and this affects many users.  We should try to fix this in a dot release for Firefox 54.
Tracked for 54. We will plan to include this fix in 54.0.1.

Hi Florin, Andrei, this is a P1 bug fix validation request. Could you team please validate this change on Nightly55 and Beta55 next week? This will give us the much needed confidence to push this change in 54.0.1 Thanks!
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #32)
> That's not going to be helpful here, since this information is needed in the
> parent process and right now this information is delivered to the parent
> through the DOMTitleChanged event
> <https://searchfox.org/mozilla-central/rev/
> a798ee4fc323f9387b7576dbed177859d29d09b7/toolkit/content/widgets/remote-
> browser.xml#447>, so see my previous comment.

Well, that message is sent fron content process at http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/toolkit/content/browser-child.js#423 which just reads information from document.

Anyway, your approach works as well. Nice work.
https://hg.mozilla.org/mozilla-central/rev/2e6cc6c65a79
https://hg.mozilla.org/mozilla-central/rev/7533679209b1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 41

2 years ago
Absolutely STUNNING!
Looking great on latest Nightly.
Also it seems that bug 1324890 is fixed now.

Based on the 2 important RTL bugs this fixes, I highly recommend to push it into 54 (or 54.0.1).

Updated

2 years ago
Depends on: 1371962
No longer depends on: 703242
Depends on: 1371995
I have reproduced this bug with Nightly 55.0a1 (2017-04-19) in Windows 10 (64-bit).

This bug's fix is verified with latest Nightly 55.0a1 (64-bit).
 
Build ID   :    20170611030208
User Agent :	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170607]
I reproduced this issue using Fx 55.0a1 (build id: 20170419030223 ) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 55.0a1, build ID: 20170611030208, on Windows 10 x64, Mac OS X 10.12.4 and Ubuntu 14.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Assignee

Comment 44

2 years ago
Comment on attachment 8876282 [details] [diff] [review]
Part 1: Expose an nsIDOMWindowUtils method for retrieving the direction of a piece of text based on the first-strong character algorithm

Approval Request Comment
[Feature/Bug causing the regression]: Bug 658467
[User impact if declined]: We show the end of the tab title for pages with a right to left title in LTR builds of the browser, and in RTL builds we show the end of the tab title for pages with left to right titles.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Already done.
[List of other uplifts needed for the feature/fix]: Bug 1371995.
[Is the change risky?]: It's not very risky, and I wrote the patch with an uplift to 54 in mind.  The C++ code here is just exposing part of the code that we have been using for years to implement the dir=auto HTML attribute to use it for the browser UI, and part 2 of the patch uses that code to compute the direction of the title very similarly to how dir=auto HTML direction works.  Note that it is very important that the regression fix in bug 1371995 is also uplifted with this at the same time!
[Why is the change risky/not risky?]:  See above.
[String changes made/needed]: None.
Attachment #8876282 - Flags: approval-mozilla-release?
Assignee

Comment 45

2 years ago
The patch here also needs the patch in bug 1371962!
Comment on attachment 8876282 [details] [diff] [review]
Part 1: Expose an nsIDOMWindowUtils method for retrieving the direction of a piece of text based on the first-strong character algorithm

Fix a tab regression. Release54+. Should be in 54.0.1.
Attachment #8876282 - Flags: approval-mozilla-release? → approval-mozilla-release+
This needs rebased patches for Release.
Flags: needinfo?(ehsan)
Assignee

Comment 50

2 years ago
Please land the patches in the following order:

  * This bug first
  * Followed by bug 1371995
  * Followed by bug 1371962

I have attached rebased and locally tested versions of all patches to the relevant bugs.
Flags: needinfo?(ehsan)

Updated

2 years ago
See Also: → 1382752
Depends on: 1432796
You need to log in before you can comment on or make changes to this bug.