Closed Bug 825035 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: General, defect)

20 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox18 --- wontfix
firefox19 + verified
firefox20 + verified
firefox21 + verified
firefox-esr17 19+ verified

People

(Reporter: jaws, Assigned: dao)

References

Details

(Whiteboard: [CtPUR:+])

Attachments

(2 files, 4 obsolete files)

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.
Version: Trunk → 20 Branch
Keywords: verifyme
QA Contact: paul.silaghi
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.
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (WIP) (obsolete) — Splinter Review
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.
Attachment #709882 - Flags: feedback?(dao)
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.
Attached patch Patch v.2 (WIP) (obsolete) — Splinter Review
Removed some flotsam from the previous patch.
Attachment #709882 - Attachment is obsolete: true
Attachment #709882 - Flags: feedback?(dao)
Attachment #710037 - Flags: feedback?(dao)
Whiteboard: [CtPUR:+]
Uploaded the wrong patch last time.
Attachment #710037 - Attachment is obsolete: true
Attachment #710037 - Flags: feedback?(dao)
Attachment #710346 - Flags: feedback?(dao)
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.
(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.
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.
(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.
Attached patch Patch v.2 (obsolete) — Splinter Review
Thanks, I see what you're saying now.
Attachment #710346 - Attachment is obsolete: true
Attachment #710346 - Flags: feedback?(dao)
Attachment #710374 - Flags: review?(dao)
Attachment #710374 - Attachment is obsolete: true
Attachment #710374 - Flags: review?(dao)
Attached patch Patch v.21Splinter Review
Attachment #710375 - Flags: review?(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.
Attachment #710375 - Flags: review?(dao) → review-
Attached patch patch?Splinter Review
Does this work? My build is messed up, need to build from scratch which will take some time.
Attachment #710402 - Flags: review?(jaws)
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/
Attachment #710402 - Flags: review?(jaws) → review+
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.
Attachment #710402 - Flags: approval-mozilla-beta+
Attachment #710402 - Flags: approval-mozilla-aurora+
> ::: 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?
That said, it seems like the "#blocked-plugins-notification-icon[showing]," line might not be needed at all.
(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 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?
Attachment #710402 - Flags: approval-mozilla-beta+
Attachment #710402 - Flags: approval-mozilla-aurora+
(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
Assignee: jaws → dao
https://hg.mozilla.org/mozilla-central/rev/93bdd70980ec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
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?
(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.
Sorry for the confusion, when I said "won't fade" I meant "won't fade in/out", "won't animate".
(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 .
(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.
(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
(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 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.
Attachment #710402 - Flags: approval-mozilla-esr17?
Comment on attachment 710402 [details] [diff] [review]
patch?

Approving on ESR, please make sure to land by 2/11 .Thank you !
Attachment #710402 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
(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.
(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
Verified fixed FF 20b1.
Verified fixed FF 21b3.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.