1392622 - [RTL] Favicons are overlaying title on latest nighty

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Arash Mousavi [:Arash-M]]

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

[:Gijs (he/him)]

Jared, any chance this is from bug 1352119?

Comment 2

[Dão Gottwald [:dao]]

I think this is very likely from bug 1352119.

Comment 3

[Arash Mousavi [:Arash-M]]

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.

Comment 4

[Tomer Cohen :tomer]

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

Properties such as padding-inline-start that are dependant on the direction property need to be flipped when labeldirection doesn't match the UI direction. I had to use margin-inline-end here instead of padding because the padding was not shifting the right-side of the box in this case. I also had to change the direction of the animation transform due to the change in directionality of the UI.

Review commit: https://reviewboard.mozilla.org/r/171446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171446/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

[Dão Gottwald [:dao]]

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?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171446/diff/1-2/

Comment 10

[Dão Gottwald [:dao]]

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?

Comment 12

[Dão Gottwald [:dao]]

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171446/diff/2-3/

Comment 14

[Dão Gottwald [:dao]]

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?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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 16

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 18

[Dão Gottwald [:dao]]

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

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

[Dão Gottwald [:dao]]

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

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171446/diff/3-4/

Comment 22

[Pulsebot]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/97b83ee324ca

Comment 24

[Stefan [:StefanG_QA]]

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