1371962 - Misplaced tab separators near hovered/focused tabs whose titles are faded out to the opposite of the browser's UI direction

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Itiel]

Attachment: Misplaced separators

This is a regression, according to Mozregression it's from the second patch of bug 1357656:

2017-06-10T22:07:11: DEBUG : Starting merge handling...
2017-06-10T22:07:11: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=7533679209b1d740374dcb3a8a36d1e23de36dff&full=1
2017-06-10T22:07:12: DEBUG : Found commit message:
Bug 1357656 - Part 2: Switch the direction of the tab title fade-out effect based on the directionality of the title string; r=mconley

STR, using Windows 10:
1. Install latest (LTR) Nightly
2. Open many tabs, I'd say 20 (so each tab text would get the fade out effect for sure) and in each one go to the following link for example:
https://addons.mozilla.org/he/firefox/
(if the site redirects you to a version of your language, change it back to Hebrew (עברית))
You can go to any other website, just make sure the first strong character of its title is RTL and that the title is long enough for Firefox to fade it out.
3. Hover or focus on one of these tabs.

AR:
a. The left side of the hovered/focused tab has a separator, and
b. this very separator is probably taken from the right side of the tab next to it, who is missing a separator.
See attached screenshot.

ER:
a. There shouldn't be a separator there
b. There should be.

The same bug applies also to other themes (tested on the default themes that come with Nightly).

The same steps can be followed on a RTL version of Nightly, you just need to use tabs whose text is long enough and LTR.

Comment 1

[Tomer Cohen :tomer]

ni @ehsan: Do you think this issue caused by your patch?

Comment 2

[(no longer active)]

Yes, this is another regression caused by my patch.

It turns out that the dir attribute is a reserved XUL attribute: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/dir

Apparently setting it to a value except than those two makes it behave the same way as reverse, which results in the artifact described in comment 0.  Using another name for this attribute makes the bug go away!

Comment 3

[(no longer active)]

Attachment: Rename the dir attribute added in bug 1357656 to labeldirection

This avoids using a reserved XUL attribute name.

Comment 4

[(no longer active)]

Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

This is another regression fix that needs to be taken for bug 1357656.  This is super low risk, it just renames an attribute.

Comment 5

[(no longer active)]

Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1357656
[User impact if declined]: Comment 0
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's an attribute rename
[String changes made/needed]: None.

Comment 6

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

Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

Recent regression, Beta55+

Comment 7

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

Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

Review of attachment 8876821 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f9c6ce520c
Rename the dir attribute added in bug 1357656 to labeldirection; r=mconley

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95f8f0437e2a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2f9c6ce520c

Comment 11

[Itiel]

Working great on latest Nightly (56.0a1 (2017-06-14)).

Comment 12

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

We may want this on 54 release too.

Comment 13

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

I reproduced this issue using Fx 55.0a1ar(2017-06-10) on Windows 10 x64.
I can confirm this issue is fixed, I verified Fx 55.0b2 using the RTL addon, Fx 55.0b2ar and Fx 56.0a1ar (since the RTL addon is not working properly on the nightly channel), build ID: 20170618030207, on Windows 10 x64.

Cheers!

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

Fix a tab regression. Release54+. Should be in 54.0.1.

Comment 15

[(no longer active)]

Attachment: Rename the dir attribute added in bug 1357656 to labeldirection

(Rebased for release)

Comment 16

[(no longer active)]

Please read bug 1357656 comment 50 before uplifting to the release repository.  Thanks in advance!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/90f18f9c15f7

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting for verification in 54.0.1 as well.

Comment 19

[Gabi Cheta [:Gabi] Desktop QA]

I reproduced this issue using Fx 55.0a1ar on Windows 10 x64.
Issue verified as fixed using Fx 54.0.1 using the RTL addon on Windows 10 x64.