1357656 - Tab labels fade in UI direction regardless of title direction

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Salar]

Attachment: 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.

Comment 1

[Salar]

The sample text for testing. "این یک متن آزمایشی است"

Comment 2

[:Gijs (he/him)]

Dão, can you investigate?

I guess this is too late for 53 now, which is a real shame. :-\

Comment 3

[Dão Gottwald [:dao]]

We currently pick the fading direction based on the UI locale rather than the individual labels.

Comment 4

[:Gijs (he/him)]

(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. :-\

Comment 5

[Dão Gottwald [:dao]]

(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...

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Is this new in 53, or does it affect 52/esr52?  Is it a (recent) regression or just a bug?  Thanks!

Comment 7

[:Gijs (he/him)]

(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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

[Tracking Requested - why for this release]:

For users in RTL locales, this makes tab titles hard to read. Unfortunately, this went out in 53. :/

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 12

[Dão Gottwald [:dao]]

Masatoshi, do you know if/how chrome JS can easily detect the direction of a string?

Comment 13

[Masatoshi Kimura [:emk]]

I don't know.

But what should happen if the string is mixed-direction?

Comment 14

[Dão Gottwald [:dao]]

(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.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 16

[Dão Gottwald [:dao]]

(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]"

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 19

[Simon Montagu :smontagu]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 22

[Dão Gottwald [:dao]]

(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?

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Filed a WHATWG issue: whatwg/html#2648

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 25

[Gerry Chang [:gchang]]

Mark 54 won't fix as it's late for Beta54.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

ni? for the question in comment 26.

Comment 28

[Dão Gottwald [:dao]]

(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.

Comment 30

[(no longer active)]

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.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 32

[(no longer active)]

(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.

Comment 33

[(no longer active)]

Attachment: Screenshot after my patches

Comment 34

[(no longer active)]

Attachment: Part 1: Expose an nsIDOMWindowUtils method for retrieving the direction of a piece of text based on the first-strong character algorithm

Comment 35

[(no longer active)]

Attachment: Part 2: Switch the direction of the tab title fade-out effect based on the directionality of the title string

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[(no longer active)]

[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.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2e6cc6c65a79
https://hg.mozilla.org/mozilla-central/rev/7533679209b1

Comment 41

[Itiel]

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).

Comment 42

[Almas Hossain[:mAlmas]]

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]

Comment 43

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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!

Comment 44

[(no longer active)]

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.

Comment 45

[(no longer active)]

The patch here also needs the patch in bug 1371962!

Comment 46

[Gerry Chang [:gchang]]

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.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

This needs rebased patches for Release.

Comment 48

[(no longer active)]

Attachment: Part 1: Expose an nsIDOMWindowUtils method for retrieving the direction of a piece of text based on the first-strong character algorithm

(Rebased for release)

Comment 49

[(no longer active)]

Attachment: Part 2: Switch the direction of the tab title fade-out effect based on the directionality of the title string

Comment 50

[(no longer active)]

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.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Thank you kind sir :)

https://hg.mozilla.org/releases/mozilla-release/rev/4a71f9c6b710
https://hg.mozilla.org/releases/mozilla-release/rev/a5f0660e6251