862201 - Mixed conted doorhanger disappears when user selects "Keep Blocking"

Status: RESOLVED WORKSFORME

(Core Graveyard :: Security: UI, defect)

Description

[Justin Dolske [:Dolske]]

1) Visit https://vine.co/v/bFd1IDuwiKu
2) Get mixed content blocker icon in url bar (bug 861753)
3) Click icon, get doorhanger, click Keep Blocking
4) doorhanger and icon (!) go away.

This is a bit surprising, since normally it sticks around because "Keep Blocking" doesn't actually do anything. I wanted to unblock the content, and had to reload the page to get the icon back.

Comment 1

[Tanvi Vyas[:tanvi]]

This is where the popupnotification is set up: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6444.  The "Keep Blocking" button is created in the "action" variable.  Justin, do you know how to keep the shield icon when the user clicks "Keep Blocking"?

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> This is where the popupnotification is set up:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#6444.  The "Keep Blocking" button is created in the "action" variable. 
> Justin, do you know how to keep the shield icon when the user clicks "Keep
> Blocking"?

Comment 3

[Tanvi Vyas[:tanvi]]

Code has moved a bit:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6613

Comment 4

[Justin Dolske [:Dolske]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)

> Justin, do you know how to keep the shield icon when the user clicks "Keep
> Blocking"?

Not offhand. You'll presumably need to give PopupNotifications some way of knowing a provided action is actually a NOP (ala Not Now).

Comment 6

[Tanvi Vyas[:tanvi]]

Spoke to Matt and got some pointers:
We should add eventCallback function to the options of the mixed content blocker notification and listen for the "shown" event.  Then you can replace the buttoncommand attribute of the notification.

The default behaviour is PopupNotifications._onButtonCommand(event); so you can set it to an empty string to test.  You could probably replace it with PopupNotifications._dismiss(); to dismiss only

eventCallback options parameter: https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?mark=218,227#205

Note that we can replace _onButtonCommand only after the shown event has triggered; doing it before would just result in it being rewritten because the DOM is reconstructed everytime the doorhanger is opened:
https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?rev=8f24192cb2b2&mark=510#483

Comment 7

[Matthew N. [:MattN]]

My other idea is to have PopupNotifications check the return type of the mainAction callback and change behaviour based on the value. (e.g. return false to not remove).

If we want to switch the callback to be async at some point it may make this idea more complicated.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

"main" actions that do nothing are kind of a weird edge case, and we do want to move to async callbacks ideally (if it's not already too late), so I don't really like the return value idea.

It looks like http://hg.mozilla.org/mozilla-central/diff/22737775ef32/toolkit/modules/PopupNotifications.jsm#l1.12 added exactly what you need, though - a way to specify that the main action should act like "dismiss" rather than "remove".

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

AFACT, the "keep blocking" button was removed.