Closed Bug 1371962 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

53 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- verified
firefox55 --- verified
firefox56 --- verified

People

(Reporter: itiel_yn8, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 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.
ni @ehsan: Do you think this issue caused by your patch?
Flags: needinfo?(ehsan)
Summary: Misplaced tab separators near hovered/focused tabs whose titles are faded out to the opposite direction of the browser → Misplaced tab separators near hovered/focused tabs whose titles are faded out to the opposite of the browser's UI direction
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!
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
This avoids using a reserved XUL attribute name.
Attachment #8876821 - Flags: review?(mconley)
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.
Attachment #8876821 - Flags: approval-mozilla-release?
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.
Attachment #8876821 - Flags: approval-mozilla-beta?
Comment on attachment 8876821 [details] [diff] [review]
Rename the dir attribute added in bug 1357656 to labeldirection

Recent regression, Beta55+
Attachment #8876821 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Attachment #8876821 - Flags: review?(mconley) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f9c6ce520c
Rename the dir attribute added in bug 1357656 to labeldirection; r=mconley
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/e2f9c6ce520c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Version: unspecified → 53 Branch
Working great on latest Nightly (56.0a1 (2017-06-14)).
Status: RESOLVED → VERIFIED
We may want this on 54 release too.
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 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.
Attachment #8876821 - Flags: approval-mozilla-release? → approval-mozilla-release+
Please read bug 1357656 comment 50 before uplifting to the release repository.  Thanks in advance!
Setting for verification in 54.0.1 as well.
Flags: qe-verify+
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.
You need to log in before you can comment on or make changes to this bug.