1399111 - new tab burst animation in background tabs is too eye catch

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Alice0775 White]

+++ This bug was initially created as a clone of Bug #1398442 +++

[Tracking Requested - why for this release]:

Suddenly, flickering of the background tab is in my eyes. While reading or entering something on the current tab



Reproducible: 100%

Steps To Reproduce:
1. Open 4-5 tabs in background (e.g. google.com, yahoo.com etc etc...)
2. Open web page in foreground tab[X] (i.e., data:text/html,<center><textarea cols=100></textarea></center> )

3. Start typing something in textarea in tab[X] while other tabs are still loading.


Actual Results:
Suddenly that flicker of background tab comes into my eyes

Expected Results:
The UI should not disturb user concentration. (except security warning).

I think that the flashing effect is bad.
I think a gradually changing UI is desirable(For example, the progress indicator gradually grows, and then it fade out at 100% and disappear)

Comment 1

[Alice0775 White]

Especially annoying, if the back ground tab has reload function with setInterval.
E.g. http://www.osaka-pref-rivercam.info/toubu/04.html (reloaded in every 1 minutes)

Comment 2

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

Amy, what are your thoughts? At the same time that it may be distracting, for other uses it may be helpful to know that the page content has just been updated.

Comment 3

[Amy Lee [:amylee] UX]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Amy, what are your thoughts? At the same time that it may be distracting,
> for other uses it may be helpful to know that the page content has just been
> updated.
I don't mind it, as you said it lets the user know that content has finished loading, but if it's a situation where the site reloads automatically as in the example URL (http://www.osaka-pref-rivercam.info/toubu/04.html) I can see why it can be distracting. I'm not sure how common these sites are that would have an automatic reload and if we should consider these edge cases? Asking Eric for input since he worked on the tab animations.

Comment 4

[Alice0775 White]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Amy, what are your thoughts? At the same time that it may be distracting,
> for other uses it may be helpful to know that the page content has just been
> updated.

In the background tab, 
It is meaningless to animate the burst every time updating.
I think that browser should display the unread state(like pinned tab) instead of the tab burst.

Comment 5

[Eric Pang [:epang] UX]

As designed the intention is to let the user know when their content has loaded and is ready to be viewed.  Saying that, I wouldn't be against making the background tab burst more subtle.  Jared, would it be possible to have a different opacity for selected and background tabs? Thinking we can lower the opacity of the burst for background tabs so they are not as bright.  Let me know what you think and if it's possible.

Comment 6

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

Short answer, yes it's possible.

Long answer, I don't know if we should differentiate. Purely because people may switch tabs while a page is loading and then wait for it to finish loading before switching back. 

I think the better fix here would be maybe to use a lower opacity for tabs that were not focused when the load began. With the exception of links that are opened in a new tab.

Comment 7

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

Attachment: Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

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

Comment 8

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

Amy, can you test out this build and let me know what you think? I have dropped the starting opacity of the burst in unselected tabs to .2 (from .4). We could also just not run the animation in this case.

https://queue.taskcluster.net/v1/task/JQhCVvyMRn2ERPpy8MAqLw/runs/0/artifacts/public/build/target.dmg

Comment 9

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

Per comment 8

Comment 10

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

This URL will reproduce this bug easily:
data:text/html,<script>setTimeout(function() { location.reload(true); }, 1000)</script>

Comment 11

[Eric Pang [:epang] UX]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Amy, can you test out this build and let me know what you think? I have
> dropped the starting opacity of the burst in unselected tabs to .2 (from
> .4). We could also just not run the animation in this case.
> 
> https://queue.taskcluster.net/v1/task/JQhCVvyMRn2ERPpy8MAqLw/runs/0/
> artifacts/public/build/target.dmg

The burst looks good at 20%
For the ball it looks like it changes from grey to white.  Can we keep it at the first state (Grey) to match the text and ‘x’?
Anything that loads in the background doesn’t need the 2 stages.  The grey keeps things more muted. Thanks!

Comment 12

[:Gijs (he/him)]

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

https://reviewboard.mozilla.org/r/184164/#review189700

r=me with the change below.

If we also change the fill colour of the loading indicator for background tabs, can you do that in a separate followup cset? Thanks!

::: browser/themes/shared/tabs.inc.css:110
(Diff revision 1)
>    animation: tab-burst-animation 375ms cubic-bezier(0,0,.58,1);
>    -moz-context-properties: fill;
>    fill: var(--tab-loading-fill);
>  }
>  
> +:root[sessionrestored] .tab-loading-burst[bursting][selectedSinceLoad=false]::before {

It's a little counterintuitive to use an attribute with a negative value to detect this case, when we often remove attributes rather than setting them to 'false' (and check only for the presence of the attribute in CSS)

Maybe you could use the attribute name "unusedSinceLoad" and set it to true in the cases where we currently set `selectedSinceLoad` to false?

Comment 13

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

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

https://reviewboard.mozilla.org/r/184164/#review189700

> It's a little counterintuitive to use an attribute with a negative value to detect this case, when we often remove attributes rather than setting them to 'false' (and check only for the presence of the attribute in CSS)
> 
> Maybe you could use the attribute name "unusedSinceLoad" and set it to true in the cases where we currently set `selectedSinceLoad` to false?

Yeah, I'll do that. I fought back and forth between these names because I think "unusedSinceLoad" is better for the points you raised, and I used "selectedSinceLoad" because it is generally better practice to have variables that use positive verbiage (!ununsedSinceLoad may sound awkward when reading it).

Comment 14

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

Attachment: Bug 1399111 - Keep the loading indicator gray for tabs that have been unselected since load.

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

Comment 15

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

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

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

Comment 16

[:Gijs (he/him)]

Comment on attachment 8913354 [details]
Bug 1399111 - Keep the loading indicator gray for tabs that have been unselected since load.

https://reviewboard.mozilla.org/r/184708/#review189954

::: browser/themes/shared/tabs.inc.css:184
(Diff revision 1)
>  
>  @keyframes tab-throbber-animation-rtl {
>    100% { transform: translateX(100%); }
>  }
>  
> -:root[sessionrestored] .tab-throbber[progress]::before {
> +:root[sessionrestored] .tab-throbber[progress][unusedSinceLoad]::before {

I must be missing something, because this doesn't look right to me. Here's how I'm reading this, please let me know where I'm wrong:

1) we want to use the grey colour throughout for tabs that are loading in the background and have not been used/selected since the load started.
2) the grey colour comes from fill: currentColor in the: `:root[sessionrestored] .tab-throbber[busy]::before` rule a few lines up
3) it is thus applied to `[busy]` tabs
4) for `[progress]` tabs we use white/blue depending on the background colour
5) the white/blue comes from `fill: var(--tab-loading-fill)` in this rule, which is overriding the earlier one.
6) we now want to use grey for those tabs, too, iff they are unused.

logically, I think that means this rule should get `:not([unusedSinceLoad])`, so that we only override the fill for used/selected (not unused) tabs. Right? But this is doing the opposite, so I'm confused.

Comment 17

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

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

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

Comment 18

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

Comment on attachment 8913354 [details]
Bug 1399111 - Keep the loading indicator gray for tabs that have been unselected since load.

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

Comment 19

[:Gijs (he/him)]

Comment on attachment 8913354 [details]
Bug 1399111 - Keep the loading indicator gray for tabs that have been unselected since load.

https://reviewboard.mozilla.org/r/184708/#review189984

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcced3138ab7
Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3e7cd55f6cb9
Keep the loading indicator gray for tabs that have been unselected since load. r=Gijs

Comment 21

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

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1392157
[User impact if declined]: tabs loading in the background may be too distracting
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no, just landed on autoland
[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?]: just changes the CSS to use a lighter tab burst in background tabs
[String changes made/needed]: none

Comment 22

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

Comment on attachment 8913354 [details]
Bug 1399111 - Keep the loading indicator gray for tabs that have been unselected since load.

Approval Request Comment -> see comment #21

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/fcced3138ab7
https://hg.mozilla.org/mozilla-central/rev/3e7cd55f6cb9

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912832 [details]
Bug 1399111 - Tabs that start loading in the background and haven't been selected before completing loading will show a lighter tab burst.

Photon polish, should be in 57b5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66cc570df716
https://hg.mozilla.org/releases/mozilla-beta/rev/c53c961ce407

Comment 26

[Amit singh]

[bugday-20180110]
I tried to reproduce this it is not reproducible on Firefox 58.0b14 (64-bit)(Build Id: 20180103230655).
Tested in Linux debain 4.10.0-38-generic #42~16.04.1-Ubuntu x86_64

Comment 27

[:Gijs (he/him)]

Taking this off the verification radar.