Jared Wein [:jaws] (please needinfo? me) Assignee
	Description • 8 years ago

From https://bugzilla.mozilla.org/show_bug.cgi?id=1232357#c39,
> Do we have any plan to add extra effects (eg. fade out) for sound indicator? 
> (Chrome uses fading out) Since it looks little strange because disappearing very 
> suddenly.

	:Gijs (he/him)
	Comment 2 • 8 years ago
mozreview-review

Comment on attachment 8803105 [details]
Bug 1311799 - Fade the tab audio icon when the audio has stopped but before it has been removed.

https://reviewboard.mozilla.org/r/87316/#review86762

I don't think this is what the comment in bug 1232357 asked about. I think it was effectively suggesting we transition the opacity from .8 to 0 when we remove it, rather than just setting it to 0 directly. Setting it to .4 in the meantime would mean we would go back to flickering in cases where the audio volume occasionally disappears.

	:Gijs (he/him)
	Comment 3 • 8 years ago

(note that I'm out until Wed, so if you have an update before then you may want to find another reviewer - sorry!)

	:Gijs (he/him)
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8803105 [details]
Bug 1311799 - Fade the tab audio icon when the audio has stopped but before it has been removed.

https://reviewboard.mozilla.org/r/87316/#review89066

::: browser/base/content/tabbrowser.xml:5082
(Diff revision 2)
>  
>            if (tab.hasAttribute("soundplaying")) {
> +            tab.setAttribute("soundplaying-scheduledremoval", "true");
> +            this._tabAttrModified(tab, ["soundplaying-scheduledremoval"]);
> +
>              let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS");

Shouldn't the delay in the transition be based on this pref? Right now the pref can change and the transition delay and length doesn't.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 6 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8803105 [details]
> Bug 1311799 - Fade the tab audio icon when the audio has stopped but before
> it has been removed.
> 
> https://reviewboard.mozilla.org/r/87316/#review89066
> 
> ::: browser/base/content/tabbrowser.xml:5082
> (Diff revision 2)
> >  
> >            if (tab.hasAttribute("soundplaying")) {
> > +            tab.setAttribute("soundplaying-scheduledremoval", "true");
> > +            this._tabAttrModified(tab, ["soundplaying-scheduledremoval"]);
> > +
> >              let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS");
> 
> Shouldn't the delay in the transition be based on this pref? Right now the
> pref can change and the transition delay and length doesn't.

Yes, I looked at that and I'm not sure what we want to do here. I'd rather not do `tab.style.transitionDelay = (removalDelay - 300) + "ms";` as that style would get applied to other transitions and wouldn't get removed without adding even more code.

Ideally we could use `attr()` for non-generated content properties and then set this via an attribute and use `calc()` in the transition property.

	:Gijs (he/him)
	Comment 7 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Comment on attachment 8803105 [details]
> > Bug 1311799 - Fade the tab audio icon when the audio has stopped but before
> > it has been removed.
> > 
> > https://reviewboard.mozilla.org/r/87316/#review89066
> > 
> > ::: browser/base/content/tabbrowser.xml:5082
> > (Diff revision 2)
> > >  
> > >            if (tab.hasAttribute("soundplaying")) {
> > > +            tab.setAttribute("soundplaying-scheduledremoval", "true");
> > > +            this._tabAttrModified(tab, ["soundplaying-scheduledremoval"]);
> > > +
> > >              let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS");
> > 
> > Shouldn't the delay in the transition be based on this pref? Right now the
> > pref can change and the transition delay and length doesn't.
> 
> Yes, I looked at that and I'm not sure what we want to do here. I'd rather
> not do `tab.style.transitionDelay = (removalDelay - 300) + "ms";` as that
> style would get applied to other transitions and wouldn't get removed
> without adding even more code.
> 
> Ideally we could use `attr()` for non-generated content properties and then
> set this via an attribute and use `calc()` in the transition property.

You could set a custom CSS property in the JS and use the value from the CSS (both with and without calc, haven't really thought about what would make more sense).

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 9 • 8 years ago

This still duplicates the transition duration between the CSS and the binding. I couldn't get calc() to work with the custom property and getting the computed style to do the math within the binding got pretty messy quickly since we might be showing the .tab-icon-overlay or .tab-icon-sound.

	:Gijs (he/him)
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8803105 [details]
Bug 1311799 - Fade the tab audio icon when the audio has stopped but before it has been removed.

https://reviewboard.mozilla.org/r/87316/#review89204

	Pulsebot
	Comment 11 • 8 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36105103e869
Fade the tab audio icon when the audio has stopped but before it has been removed. r=Gijs

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/36105103e869