825035 - Blocklisted click-to-play notification only fades in/out once per window per session

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

Bug 819992 added a notification to the location bar when a blocklisted plugin is found on a page. The notification is supposed to fade in and out to help draw attention to it. However, this only occurs once for each window.

Manual testing with DOM Inspector by adding/removing the hidden="true" attribute on the notification will get the animation to work correctly, but for some reason this does not happen when the code is run.

Comment 1

[Alex Keybl [:akeybl]]

Jared - we didn't see this regression from bug 819992 until now. Have you made any progress? This means that the plan in https://bugzilla.mozilla.org/show_bug.cgi?id=819992#c3 is not yet fully implemented.

We're now coming on beta 5, but we would definitely make an exception if resolved before tomorrow.

Comment 2

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

I've been working on something that might fix this, but it might/might-not work. It will involve us moving from CSS animations to CSS transitions since restarting CSS animations does not appear to be a supported ability.

Comment 3

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

Attachment: Patch (WIP)

CSS animations are not easily restarted, which causes this bug because the animation doesn't restart when the element gets the "hidden" attribute added/removed.

The animation here is slightly tweaked but I believe it to be superior to the previous animation anyways.

I only wrote the patch for Windows because I'm not sure if it's something that we could take. Changing [hidden] to a newly created attribute such as "collapsed" was needed because the transition won't interpolate if the element was previously display:none, but this obviously makes this patch hit more parts of the codebase than is normally desirable.

Comment 4

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

Another negative about this patch is that it will apply the transition to all notification-icons. I couldn't use the ::before pseudo-element on the actual icon because generated content is not defined for replaced elements.

A workaround would be to set a attribute on the notification-popup-box with a white-space separated list of notification icons being shown. With that, I'd be able to do #notification-popup-box[notification~="blocked-plugins"]::before to only apply this style to the specific notification.

Comment 5

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

Attachment: Patch v.2 (WIP)

Removed some flotsam from the previous patch.

Comment 6

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

Attachment: Patch v.2 (WIP) [correct one this time]

Uploaded the wrong patch last time.

Comment 7

[Alex Keybl [:akeybl]]

Is there a lower risk (possibly more hacky) solution that we can use specifically for FF19 and CTP? If we can't get a fix in today (looking unlikely) we've got one more opportunity to fix for FF19 on Monday, but that's pretty late to be making changes of this nature.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #3)
> I only wrote the patch for Windows because I'm not sure if it's something
> that we could take. Changing [hidden] to a newly created attribute such as
> "collapsed" was needed because the transition won't interpolate if the
> element was previously display:none, but this obviously makes this patch hit
> more parts of the codebase than is normally desirable.

You could override display and use visibility:collapse without changing the attributes.

I'm curious whether Gecko is smart enough to halt the animation when collapsed or whether it would constantly consume resources.

Comment 9

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

If I override display and use visibility:collapse, about 90% of the patch will remain the same. I still have to switch from using an animation to a transition because it is non-trivial to restart the animation.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #9)
> If I override display and use visibility:collapse, about 90% of the patch
> will remain the same.

We must be looking at different patches then.

> I still have to switch from using an animation to a
> transition because it is non-trivial to restart the animation.

I wasn't saying you could drop that part of the patch.

Comment 11

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

Attachment: Patch v.2

Thanks, I see what you're saying now.

Comment 12

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

Attachment: Patch v.21

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 710375 [details] [diff] [review]
Patch v.21

This doesn't handle multiple notification icons being present simultaneously. E.g. if there's a geolocation notification and then we block a plugin, the click-to-play icon wouldn't animate.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch?

Does this work? My build is messed up, need to build from scratch which will take some time.

Comment 15

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

Comment on attachment 710402 [details] [diff] [review]
patch?

Review of attachment 710402 [details] [diff] [review]:
-----------------------------------------------------------------

This works with the following typo fixed, and it's much simpler than my patch. Thanks for the help!

::: browser/themes/winstripe/browser.css
@@ +2352,5 @@
>  #blocked-plugins-notification-icon {
>    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);
>  }
>  
> +#blocked-plugins-notification-icon[showing],

s/showing/hidden/

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 710402 [details] [diff] [review]
patch?

While unorthodox, I'm pre-approving for landing on Aurora/Beta so that this can make it into FF19b5.

Comment 17

[Dão Gottwald [:dao]]

> ::: browser/themes/winstripe/browser.css
> @@ +2352,5 @@
> >  #blocked-plugins-notification-icon {
> >    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);
> >  }
> >  
> > +#blocked-plugins-notification-icon[showing],
> 
> s/showing/hidden/

Um, we never set 'hidden' on these icons, do we?

Comment 18

[Dão Gottwald [:dao]]

That said, it seems like the "#blocked-plugins-notification-icon[showing]," line might not be needed at all.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #17)
> > ::: browser/themes/winstripe/browser.css
> > @@ +2352,5 @@
> > >  #blocked-plugins-notification-icon {
> > >    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);
> > >  }
> > >  
> > > +#blocked-plugins-notification-icon[showing],
> > 
> > s/showing/hidden/
> 
> Um, we never set 'hidden' on these icons, do we?

I see my mistake now, though: I should have written :not([showing])

Anyway, it would be great if you could test whether I can just drop that line.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 710402 [details] [diff] [review]
patch?

a+ carries over for the final patch, but it doesn't sound like this is ready quite yet?

Comment 21

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

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > > ::: browser/themes/winstripe/browser.css
> > > @@ +2352,5 @@
> > > >  #blocked-plugins-notification-icon {
> > > >    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);
> > > >  }
> > > >  
> > > > +#blocked-plugins-notification-icon[showing],
> > > 
> > > s/showing/hidden/
> > 
> > Um, we never set 'hidden' on these icons, do we?
> 
> I see my mistake now, though: I should have written :not([showing])
> 
> Anyway, it would be great if you could test whether I can just drop that
> line.

Yeah, it works fine without the line.

https://hg.mozilla.org/integration/mozilla-inbound/rev/93bdd70980ec
https://hg.mozilla.org/releases/mozilla-aurora/rev/4af2713c8f5e
https://hg.mozilla.org/releases/mozilla-beta/rev/62edd3b84353

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/93bdd70980ec

Comment 23

[Paul Silaghi, QA [:pauly]]

Looks ok. The animation works fine in new tabs/windows. Verified fixed FF 19b5 on Win 7, Ubuntu 12.04, Mac OS X 10.7.5

Comment 24

[Paul Silaghi, QA [:pauly]]

Couple of minor remarks (are not regressions):
1. If reloading the page before activating the plugin (or if reloading a page with multiple plugins with only one plugin activated eg. http://www.miniclip.com/games/commando-3/en/), the ctp icon won't fade.
2. Switching CTP blocked tab with a normal tab (www.google.com) makes the CTP icon to fade everytime.
Do you consider any of these to be real issues?

Comment 25

[Dão Gottwald [:dao]]

(In reply to Paul Silaghi [QA] from comment #24)
> 1. If reloading the page before activating the plugin (or if reloading a
> page with multiple plugins with only one plugin activated eg.
> http://www.miniclip.com/games/commando-3/en/), the ctp icon won't fade.

That's probably just bug 825804.

Comment 26

[Paul Silaghi, QA [:pauly]]

Sorry for the confusion, when I said "won't fade" I meant "won't fade in/out", "won't animate".

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Jared Wein [:jaws] from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > (In reply to Dão Gottwald [:dao] from comment #17)
> > > > ::: browser/themes/winstripe/browser.css
> > > > @@ +2352,5 @@
> > > > >  #blocked-plugins-notification-icon {
> > > > >    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);
> > > > >  }
> > > > >  
> > > > > +#blocked-plugins-notification-icon[showing],
> > > > 
> > > > s/showing/hidden/
> > > 
> > > Um, we never set 'hidden' on these icons, do we?
> > 
> > I see my mistake now, though: I should have written :not([showing])
> > 
> > Anyway, it would be great if you could test whether I can just drop that
> > line.
> 
> Yeah, it works fine without the line.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/93bdd70980ec
> https://hg.mozilla.org/releases/mozilla-aurora/rev/4af2713c8f5e
> https://hg.mozilla.org/releases/mozilla-beta/rev/62edd3b84353

Can we please get an esr17 patch ready and nominated for approval asap ? Needs to land cleanly before 2/11 .

Comment 28

[Alex Keybl [:akeybl]]

(In reply to Paul Silaghi [QA] from comment #24)
> 2. Switching CTP blocked tab with a normal tab (www.google.com) makes the
> CTP icon to fade everytime.

Can you be more specific? I don't understand the behavior you're describing here.

Comment 29

[Paul Silaghi, QA [:pauly]]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Can you be more specific? I don't understand the behavior you're describing
> here.
1. Have outdated flash installed
2. Open miniclip.com -> CTP icon fades in/out
3. Open google.com in another tab
4. Click on miniclip tab again -> CTP icon fades in/out again

Comment 30

[Alex Keybl [:akeybl]]

(In reply to Paul Silaghi [QA] from comment #29)
> (In reply to Alex Keybl [:akeybl] from comment #28)
> > Can you be more specific? I don't understand the behavior you're describing
> > here.
> 1. Have outdated flash installed
> 2. Open miniclip.com -> CTP icon fades in/out
> 3. Open google.com in another tab
> 4. Click on miniclip tab again -> CTP icon fades in/out again

I think that's good behavior, even if unintentional. Thanks for clarifying.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 710402 [details] [diff] [review]
patch?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: I didn't think this is needed for ESR :)
User impact if declined: see comment 0
Fix Landed on Version: 21, uplifted to 20 and 19
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 710402 [details] [diff] [review]
patch?

Approving on ESR, please make sure to land by 2/11 .Thank you !

Comment 33

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/db74aa5639c1

Comment 34

[Paul Silaghi, QA [:pauly]]

(In reply to Alex Keybl [:akeybl] from comment #30)
> I think that's good behavior, even if unintentional. Thanks for clarifying.
You're right, it's just that I was thinking having a flash game (or a music player, radio) surrounded by some flash ads - I want only the game active and the ads blocked. Switching tabs might be annoying at some point due to the repeatedly red animation.

Comment 35

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi [QA] from comment #23)
> Looks ok. The animation works fine in new tabs/windows. Verified fixed FF
> 19b5 on Win 7, Ubuntu 12.04, Mac OS X 10.7.5
Verified fixed 2013-02-12-03-45-03-mozilla-esr17

Comment 36

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 20b1.

Comment 37

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 21b3.