[RTL] Favicons are overlaying title on latest nighty

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Arash-M, Assigned: jaws)

Tracking

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

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted image favicon.png
What's wrong:

With today's nightly, tabs are making space on the wrong direction while the favicons overlay the title (check the attachment).

How should it be?

On an RTL locale, the tab should have a padding on right (instead of left) so the favicons do not overlay the title.

Comment 1

2 years ago
Jared, any chance this is from bug 1352119?
Flags: needinfo?(jaws)
Whiteboard: [photon-animation][triage][photon-visual][triage]
I think this is very likely from bug 1352119.
No longer blocks: photon-tabs
Whiteboard: [photon-animation][triage][photon-visual][triage] → [photon-animation][triage]
(Reporter)

Comment 3

2 years ago
I have to mention that because of the same issue the new loading animation is also not visible. It seems that the title text blocks it.
Flags: qe-verify+
Priority: -- → P3
QA Contact: stefan.georgiev
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Last good revision: a64f03637f34455a95c5d441ce60a4503b7511a5
First bad revision: de253c53864457e3879dd405212e368ec77ddfb2
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a64f03637f34455a95c5d441ce60a4503b7511a5&tochange=de253c53864457e3879dd405212e368ec77ddfb2

de253c538644	Jim Porter — Bug 1352119 - Implement new page loading indicator animation, part 1: update the throbber. Additional performance improvements implemented by Jared Wein. r=dao

Updated

2 years ago
Keywords: regression
Flags: needinfo?(jaws)

Updated

2 years ago
Duplicate of this bug: 1392736
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
To test this I used http://www.bbc.com/arabic/world-41010500 as well as changed the `intl.uidirection` pref in about:config to 1.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

https://reviewboard.mozilla.org/r/171446/#review176666

::: browser/themes/shared/tabs.inc.css:145
(Diff revision 1)
>  .tab-sharing-icon-overlay {
>    animation: 3s linear tab-sharing-icon-pulse infinite;
>  }
>  
>  .tab-label-container {
>    padding-inline-start: 22px;

Why not use margin here?
Iteration: --- → 57.2 - Aug 29
Priority: P3 → P1
Comment hidden (mozreview-request)
I'm gonna take a step back: Can you remind me what the padding/margin stuff is good for in the first place? Was this for the sliding animation that was dropped? Can we just get rid these changes from bug 1352119?
Flags: needinfo?(jaws)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

https://reviewboard.mozilla.org/r/171446/#review177422
Attachment #8900093 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Blocks: 1392792

Comment 14

2 years ago
mozreview-review
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

https://reviewboard.mozilla.org/r/171446/#review177496

::: browser/themes/shared/tabs.inc.css:102
(Diff revision 3)
>    fill: currentColor;
>    opacity: 0.7;
>  }
>  
> +:root[sessionrestored] .tab-throbber[busy]:-moz-locale-dir(rtl)::before {
> +  animation-name: tab-throbber-animation-rtl;

Not sure what difference this makes. Does animation-direction: reverse; achieve the same?
Attachment #8900093 - Flags: review?(dao+bmo)
Flags: needinfo?(jaws)
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

https://reviewboard.mozilla.org/r/171446/#review177496

> Not sure what difference this makes. Does animation-direction: reverse; achieve the same?

The -rtl animation moves from 0px to +960px, whereas the default (ltr) animation moves from 0px to -960px. The asset is drawn in the opposite direction. Normally in the transform of the animation I would keep the translate the same but add scaleX(-1) but that doesn't seem to have the same effect in generated-content as it was having elsewhere.
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

I did also try using animationd-direction: reverse; just to make sure I didn't miss anything and that doesn't work. All that animation-direction should do is run from 100% to 0%, not change the direction of the background-image.
Attachment #8900093 - Flags: review?(dao+bmo)

Updated

2 years ago
Duplicate of this bug: 1393516
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> Comment on attachment 8900093 [details]
> Bug 1392622 - Remove the padding changes that bug 1352119 introduced since
> they aren't needed now that we're not sliding the tab title.
> 
> https://reviewboard.mozilla.org/r/171446/#review177496
> 
> > Not sure what difference this makes. Does animation-direction: reverse; achieve the same?
> 
> The -rtl animation moves from 0px to +960px, whereas the default (ltr)
> animation moves from 0px to -960px.

How does this matter in practice?
The background image is displayed facing the opposite way in RTL (from 0x,0y to 960x,0y in LTR, and 0x,0y to -960x,0y in RTL), whereas translateX stays consistent in this case (at least for ::before). So because the image is displayed the opposite way we need to do the translate accordingly.

Comment 20

2 years ago
mozreview-review
Comment on attachment 8900093 [details]
Bug 1392622 - Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL.

https://reviewboard.mozilla.org/r/171446/#review178116
Attachment #8900093 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)

Comment 22

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97b83ee324ca
Remove the padding changes that bug 1352119 introduced since they aren't needed now that we're not sliding the tab title. Also, flip the translation of the animation when running in RTL. r=dao

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97b83ee324ca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Depends on: 1394241
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170830100230

This bug fix is Verified with latest Nightly 57.0a1
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: 1393505
You need to log in before you can comment on or make changes to this bug.