1352694 - Empty space appears inside tab for several seconds sometimes

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[684sigma]

User Agent   Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

I have a problem with Firefox Beta 52. It also happens in Nightly (2017-04-01). Doesn't happen in Firefox ESR 45.
Sometimes when browse, empty space appears inside tab for ~3 seconds, so the title becomes clipped. This is unfortunate.
It happens unpredictably, however, I noticed one specific scenario when it happens

1. Open https://www.w3schools.com/html/HTML5_video.asp 
2. Execute in console "document.title='';v=video1;v.play();setTimeout('v.pause()',500);setTimeout('v.play();v.pause()',2000)"
3. Wait 3.5s

Result: Empty space appears inside tab for ~1.5 seconds
Expected: No empty space

Comment 1

[684sigma]

In Nightly 2017-04-01 Bug 1345440 is fixed, but this bug isn't fixed.
I'll comment on discussion from Bug 1345440 here

1) In Bug 1345440 it was explained (by Jared and Daniel) that empty space appears, because opacity transition starts from "0" to "0". I think in this bug, the root cause is probably that UI is listening for 'transitionend' event to change attribute in selected tab.

2) See JavaScript code above, it uses 3ms timeout between "play()" and "pause()".
The bug (Comment 0) happens on my PC if timeout is between 0ms and 13ms. I often see sites that briefly play sound in background (I haven't reported it yet) and as a result, cause this bug.

3) (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> For what it's worth, I'm also OK with wontfix'ing this bug because the
> solution for it introduces a synchronous layout flush
There're at least 2 other obvious solutions: {1} to remove all transitions to make it work lie ESR 45, {2} control attributes with JavaScript (set timeouts and stuff, not listen to 'transitionend' (if that's the case)).

Comment 2

[:Gijs (he/him)]

(In reply to 684sigma from comment #1)
> In Nightly 2017-04-01 Bug 1345440 is fixed, but this bug isn't fixed.
> I'll comment on discussion from Bug 1345440 here
> 
> 1) In Bug 1345440 it was explained (by Jared and Daniel) that empty space
> appears, because opacity transition starts from "0" to "0". I think in this
> bug, the root cause is probably that UI is listening for 'transitionend'
> event to change attribute in selected tab.
> 
> 2) See JavaScript code above, it uses 3ms timeout between "play()" and
> "pause()".

I don't see 3ms anywhere, only 2s and 0.5s.

> The bug (Comment 0) happens on my PC if timeout is between 0ms and 13ms. I
> often see sites that briefly play sound in background (I haven't reported it
> yet) and as a result, cause this bug.

Can you give an example?

> 3) (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > For what it's worth, I'm also OK with wontfix'ing this bug because the
> > solution for it introduces a synchronous layout flush
> There're at least 2 other obvious solutions: {1} to remove all transitions
> to make it work lie ESR 45,

Which would make the icon flicker on the sites you mention under (2), which would then also be annoying...

> {2} control attributes with JavaScript (set
> timeouts and stuff, not listen to 'transitionend' (if that's the case)).

I don't see any transitionend listeners, but there is a timer using JS already...

Jared, if I'm reading the code right (haven't tested this), if we hit DOMAudioPlaybackStarted within the scheduled removal time after a pause, we will cancel the timer but the soundplaying attribute will still be on the tab and so we won't flush style. Seems like we should always flush if we remove soundplaying-scheduledremoval, so we could set a bool in that if statement and then flush after the conditional block that calls setAttribute("soundplaying", "true"), regardless of whether we just added it or it was already there. Does that sound sensible or am I missing something?

Comment 3

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

(In reply to :Gijs from comment #2)
> Jared, if I'm reading the code right (haven't tested this), if we hit
> DOMAudioPlaybackStarted within the scheduled removal time after a pause, we
> will cancel the timer but the soundplaying attribute will still be on the
> tab and so we won't flush style. Seems like we should always flush if we
> remove soundplaying-scheduledremoval, so we could set a bool in that if
> statement and then flush after the conditional block that calls
> setAttribute("soundplaying", "true"), regardless of whether we just added it
> or it was already there. Does that sound sensible or am I missing something?

I don't think it will be necessary to flush style in that case.

Working through your example:
1. The tab doesn't have the soundplaying attribute
2. The tab gets the soundplaying attribute and style is flushed.
3. The audio is immediately paused and gets the "soundplaying-scheduledremoval" which starts the transition with a transition-delay.
4. The audio is started again, and the "soundplaying-scheduledremoval" is removed but the soundplaying attribute remains and thus style isn't flushed.

The style flush in (2) is sufficient to get the icon displayed immediately. We need the flush to happen before the transition since we would be going from 0->0 without it (the flush forces us to go from 0->.8->0).

Removing the soundplaying-scheduledremoval attribute will change the opacity to go from anywhere between and including .8-0 -> .8. If the opacity was at .8 when the scheduledremoval attribute is removed it will stay at .8 and it can wait for the style system to update it on the interval without needing to force anything. If it was at 0, we again can wait for the style system to update on the next interval and bring it back to .8 naturally.

Comment 4

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

With more thought, your read of the code was correct Gijs. Patch incoming.

Comment 5

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

Attachment: Bug 1352694 - Flush style on the tab when audio playback is started to restart the transitions.

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

Comment 6

[:Gijs (he/him)]

Comment on attachment 8854296 [details]
Bug 1352694 - Flush style on the tab when audio playback is started to restart the transitions.

https://reviewboard.mozilla.org/r/126222/#review128844

On the assumption you've checked with the STR in comment #0 that you can reproduce this and that this fixes it, r=me! :-)

Comment 7

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

(In reply to :Gijs from comment #6)
> Comment on attachment 8854296 [details]
> Bug 1352694 - Flush style on the tab when audio playback is started to
> restart the transitions.
> 
> https://reviewboard.mozilla.org/r/126222/#review128844
> 
> On the assumption you've checked with the STR in comment #0 that you can
> reproduce this and that this fixes it, r=me! :-)

Yes, reproduced and fixed before requesting review :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8713c2bbcd82
Flush style on the tab when audio playback is started to restart the transitions. r=Gijs

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8713c2bbcd82

Comment 10

[Ryan VanderMeulen [:RyanVM]]

684sigma, can you please verify that this is working better in tomorrow's nightly?

Comment 11

[684sigma]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> 684sigma, can you please verify that this is working better in tomorrow's
> nightly?

Well, this bug and bug 1345440 are fixed in Nightly 2017-04-06 and Nightly 2017-04-09, but Bug 1345479 is not fixed. But Jared is working to remove the fix from the browser in Bug 1354612, you better ask him.



(In reply to :Gijs from comment #2)
> I don't see 3ms anywhere, only 2s and 0.5s.

Here is the code:
document.title=''; v=video1; v.play(); setTimeout('v.pause()',500); setTimeout('v.play(); setTimeout(`v.pause();`, 3)', 2500)

I had several versions of each bug report, so this example got lost. The case with 3ms between play() and pause() doesn't matter if your solution relies on flush, but if the new solution will rely on something like 'setTimeout(...,0)', then 3ms case may cause problems.

> Can you give an example?

Before answering, I should note that after checking once again, those pages don't cause this bug. It happens randomly, and I can't tell exactly when or where I found it, sorry.
The pages I mentioned are described in Bug 1354992. As I've said, "I haven't reported it yet", so the answer (filing a new bug report) would take several days anyway.

> Which would make the icon flicker on the sites you mention under (2), which would then also be annoying...

Luckily, I haven't seen much of that. And considering that in my example site does this once, one flicker for 10ms is less annoying than empty space staying for 3 seconds.
You're saying that as if I wouldn't like removing the audio indicator completely.
My only point is that adding in the browser more things that don't work as intended is very distracting and is causing a lot of annoyance for user. Probably from developer's perspective it's better to have a feature in some (very broken) form than don't have that feature, but from user's perspective it's better to have a consistent WORKING set of features.
There're a lot of bugs in the feature that I, a random man, have spotted on day 1:
Bug 1345538, Bug 1345479, Bug 1344509, Bug 1351020, Bug 1346872, Bug 1345541, Bug 1346880, Bug 1354992, Bug 1352694, Bug 1345440.
If you confirm that you released the feature on ESR 45 with a known bug, then I again will respond that I would like that feature to stay in development until all obvious bugs are resolved.

> I don't see any transitionend listeners, but there is a timer using JS already...

To be clear: I haven't looked at your code at all, but I know that it can be done with timeouts without bugs, see also Bug 1345440 comment 24.
Also, there's another way to fix the bug: to remove transition added in Bug 1311799. It won't make the icon flicker.
Anyway, my point still stands: there's more than 1 way to fix the bug.



Mozregression-gui generated this regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b99f3a05db461e9a4384d95a95c7ecbfde863bc4&tochange=36105103e8699a45893b16745e8b8f85daebf82a
->
1311799 – Fade the tab audio icon when the audio has stopped but before it has been removed
https://bugzilla.mozilla.org/show_bug.cgi?id=1311799

Comment 12

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

Wontfix for 54 as we are heading into beta. The fix can ride with 55, as with bug 1345440.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'm inclined to wontfix this for esr52. What are your thoughts?

Comment 14

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

Yeah, this should just ride the trains. It's very low severity and low occurrence as well. I would rather wait.

Comment 15

[Sajedul Islam]

I have reproduced this bug with Nightly 55.0a1 (2017-04-01) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build ID 	20170426030329
User Agent      Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]