Closed Bug 1392979 Opened 7 years ago Closed 7 years ago

Update Flash permission doorhanger to match the rest of our permission UI

Categories

(Core Graveyard :: Plug-ins, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bram, Assigned: alexical)

References

Details

Attachments

(5 files, 1 obsolete file)

This issue is to implement an update to our Flash permission doorhanger, so it uses the same style that the rest of our permission UI uses, and is ready for v57.

String alternatives:

1. Will you allow this site to use Adobe Flash? (Flash may slow browser performance.)

2. Do you want to allow Flash to run on this site? Only allow Flash on sites you trust.

3. Will you allow this site to use Adobe Flash? (Flash can affect browser performance and security)

Followed by a checkbox called “Remember this decision”.

Followed by two buttons: [Don’t Allow] and [Allow] – default to Allow.
Hi Bram, I did some investigation and turns out there was bug 1290912 for this, which apparently got stuck due to some remaining open questions. I'll try to summarize some of the discussion to try to find some points that can be decided quickly and what can be left for the future:

- there's some talks about the behavior of the red icon (which is used for outdated/vulnerable plugin versions), which I believe we can ignore for now. (it has probs but works reasonably enough, and doesn't need to block a visual update of the doorhanger)

- one of the points that were raised in that bug is that we hope that users would click the "Allow and Remember" most, and the proposed new version has a checkbox for that, but there's an open question on whether that checkbox should be checked by default or not.

- I want to mention that we don't need to be tied to the checkbox design. There are alternate versions of this dialog that can be used. See for example the one used for push notifications, which has two buttons and a dropdown.  (e.g. https://gauntface.github.io/simple-push-demo/ )

- Benjamin mentioned that the current UI was fine-tuned with user research data, and was worried about changing it. So another approach is to just modernize the look, while keeping the same two buttons (Allow and Allow and Remember)

- And there's a question of incorporating this into the Site Permissions (Identity Block) panel too. (Showing "Activate Flash" as one of the permissions granted to the website if you click the identity block icon). Ideally we want that as well, but hopefully this can be left for a different bug.


One thing that I will mention is that although it looks like this can be a simple visual update, the implementation won't be as trivial. Every new doorhanger already gets the new, modern look, but this one remained with the outdated look exactly because it's a custom panel which implemented a lot of extra behavior necessary for plugins.
From some reading of the code, I believe a lot of the complexity came from when we supported more than just Flash, so that conceivably is no longer a worry anymore, but properly removing that complexity will require some extra care (and updating a lot of tests too).
See Also: → 1290912
Hi Felipe, thanks for summarising the issue so deftly!

> there's some talks about the behavior of the red icon […] which I believe we can ignore for now
> […]
> there's a question of incorporating this into the Site Permissions (Identity Block) panel too

Yes. Let’s resolve this in another bug.

Two questions here:

1. Has the “Your plugin is outdated” message already updated to the new UI style?

2. If we don’t incorporate Flash management into the Site Permissions panel, then it must be managed from the doorhanger itself. This seems to be part of the the “custom panel which implemented a lot of extra behavior” that you wrote about. Is it possible to port all of these panel states into Photon?


> […] there's an open question on whether that checkbox should be checked by default or not.

As we believe that Flash is insecure and may affect performance, we should follow it through by making ‘Always Allow’ go through an extra checkbox, which is not checked by default.

One design note here: we have a design pattern for a separate warning text below the checkbox. What do you think?

> Will you allow this site to use Adobe Flash?
> 
> [ ] Remember this decision
> 
> Flash can affect browser performance and security <-- text appears in red (e.g. https://cl.ly/1w1d3H333g3l)
> 
> [Don’t Allow]   [Allow Flash]


> There are alternate versions of this dialog that can be used. See for example the one used for push notifications, which has two buttons and a dropdown.

This is an interesting solution!

First of all, we don’t want to use the dropdown button for the default actions: ‘Allow Flash’ and ‘Always Allow Flash’. It’s clearer to declare the extra choice in a separate checkbox.

This leaves us with using the dropdown button for the secondary actions: ‘Don’t Allow’ and ‘Never Allow’. But if we have these two options, then we have a total of 4 options spread over a primary button, a secondary dropdown button and a checkbox.

This seems a bit confusing to me. If given the choice, I’d rather keep the option to Always Allow – knowing that a significant percentage of our Flash users want to do it – over keeping the option to Never Allow (which is only useful in some situation).


> Benjamin mentioned that the current UI was fine-tuned with user research data, and was worried about changing it. So another approach is to just modernize the look, while keeping the same two buttons (Allow and Allow and Remember)

I think there has been user research done, as well, to test the effectiveness of the new doorhanger UI. I’d prefer to follow what UX and our design system has suggested.

However, it’s very important to acknowledge the fact that maybe this is the easier path to land the new UI:

Current UI --> New UI with same string + same buttons --> New UI with new string + button following most other permissions

So we don’t necessarily have to ship with the new string + button for Photon, as long as the look is modernised first.

My conservative concern: if the panel has been so heavily customised (as I’ve written above e.g. managing Flash from the doorhanger itself, and there may be other special cases), then it might not even be possible to change the string + button in time for Photon without breaking too many things.

Then the least we can do is just update the look without changing the string + button, then update the string for v58?


Does this help clarify things a bit? Thanks a lot for working on this!
Flags: needinfo?(felipc)
Attached image flashnotification.PNG
> Current UI --> New UI with same string + same buttons

How's this? (On Windows, the button order is reversed for the modern doorhanger UI - do we want to follow that, or keep the button order as it was? On OSX the button order will remain unchanged regardless.)
Flags: needinfo?(bram)
Quick note: my personal feeling is that it might be a _little_ tight getting the new strings and checkbox in, but that it is still feasible, so long as we defer the Site Permissions panel work. As Felipe noted, clearing out the code that handles multiple plugins will remove a lot of complexity, though we'll have to sift through the tests to do this. I can make a rough sketch of this though and see how much work we're really talking about.


> Flash can affect browser performance and security <-- text appears in red (e.g. https://cl.ly/1w1d3H333g3l)

This seems a bit user-scaring to me. My gut says most users would like the browser to just make a decision for them about security (and to a lesser extent performance.) Saying that it can affect security just creates an uncomfortable uncertainty in my mind - I don't know the extent to which it compromises my security and I don't want to do the research to find out - I just want to browse and feel safe. While it's nice to inform our users, I don't know that they will be any more informed from this warning. They might just switch to Chrome and see what it does automatically. If we really want to inform our users, do we maybe want to just have a link that says "Read more about Flash security and performance" and links to an article explaining in more detail?
(In reply to Doug Thayer [:dthayer] from comment #3)
> How's this? (On Windows, the button order is reversed for the modern
> doorhanger UI - do we want to follow that, or keep the button order as it
> was? On OSX the button order will remain unchanged regardless.)

This looks great – exactly as it should be!

Let’s follow each OS’ default button order. On Windows, the main (usually positive) action is always on the left. There’s no need to implement our own logic.


(In reply to Doug Thayer [:dthayer] from comment #4)
> This seems a bit user-scaring to me. My gut says most users would like the
> browser to just make a decision for them about security (and to a lesser
> extent performance.) Saying that it can affect security just creates an
> uncomfortable uncertainty in my mind - I don't know the extent to which it
> compromises my security and I don't want to do the research to find out - I
> just want to browse and feel safe.

You’re right. I think putting the warning in red is way too much! It was worth exploring, but let’s just go back to the earlier proposals posted on comment #0.

Back in the day, I didn’t want to put any warning on the message, with the understanding that 1) the user wants to get things done, not deal with problems, and 2) if we make it harder for them from doing what they want to do, they’ll find an alternative (which may well involve migration to another browser).

But since Flash was becoming more and more of a risk every day, we had to think of a way of communicating its impact somehow.

So when we say:

> 1. Will you allow this site to use Adobe Flash? (Flash may slow browser performance.)
> OR
> 3. Will you allow this site to use Adobe Flash? (Flash can affect browser performance and security)

The user might think, “OK. I’ll just move to another browse where Flash won’t affect performance or security”.


So perhaps, what we should say instead is something like the second suggested string:

> 2. Do you want to allow Flash to run on this site? Only allow Flash on sites you trust.

And then uncheck the “Remember” checkbox by default.


Having an article would be really nice. I haven’t thought of that before. I’m worried about our shipping time frame, but writing a SUMO article may be a totally a separate effort from modernising the UI.

We already have an article for blocking intrusive Flash, after all – https://support.mozilla.org/en-US/kb/flash-blocklists

But I’d be very happy with saying simply “Only allow Flash on sites you trust”.
Flags: needinfo?(bram)
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment on attachment 8902779 [details]
Bug 1392979 - Modernize buttons for Flash CTP

https://reviewboard.mozilla.org/r/174442/#review179678

Awesome!
Attachment #8902779 - Flags: review?(felipc) → review+
Attached image flash_outdated.PNG
Bram, I believe we also need strings for the case where Flash is outdated and should be updated. See the attachment for what the current doorhanger looks like.
Flags: needinfo?(bram)
One more question: I'm realizing I'm not actually clear on what "Don't Allow" with "Remember" checked should mean. With camera permissions, since sites can request camera permission at any time, "Never allow" can mean something like, "Don't ask me again", but the Flash doorhanger always requires user interaction to show, so I don't know that "Never allow" really makes any sense. Would that mean that we don't even show the plugin icon in the infobar, and treat Flash as if it's completely disabled on that page (and remove all of the Activate Adobe Flash overlays?) At that point it becomes necessary to figure out exactly how a user might undo such an action, though.
Sorry for the barrage of questions, but I'm realizing it also feels weird to keep the same doorhanger in the case where the user has already allowed Flash and might want to block it. At the very least, it seems like the "Allow" button should be changed to "Continue Allowing" in that case. Otherwise, the user might be confused as there's no indication that Flash is currently allowed, so they might think that their initial interaction which allowed it didn't even do anything. Thoughts?
Felipe: I figured I'd put the review up while these questions were still unanswered since they shouldn't change much of the overall picture. I just wanted to get your thoughts on the overall approach here.

Here's the try push (the browser_all_files_referenced failures have been fixed):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d150b220819ddefb75cdae5e8c7156b1d7bc8df4
Bram: one more question - do you want the "Learn More" link removed? I notice it wasn't mentioned in your comments, but I wasn't clear on whether that meant you wanted it gone or not.
Hi Doug, it’s awesome to see your progress!


Answering some of your questions below:

(In reply to Doug Thayer [:dthayer] from comment #9)
> […] we also need strings for the case where Flash is outdated
> and should be updated.

Earlier, I was thinking that all outdated Flash should be prevented from running until it’s been updated to the latest version. I haven’t thought fully of its repercussions, so let’s keep our current behaviour and simply update the strings:

> Do you want to allow an outdated version of Flash to run on this site?
> 
> An outdated version can affect browser performance and security.
> 
> [Update Adobe Flash…] <-- this is a link
> 
> 
> [ ] Remember this decision
> 
> [Don’t Allow] [Allow]



(from comment #10)
> One more question: I'm realizing I'm not actually clear on what "Don't
> Allow" with "Remember" checked should mean […]
> Would that mean that we don't even show the plugin icon in
> the infobar, and treat Flash as if it's completely disabled on that page
> (and remove all of the Activate Adobe Flash overlays?) At that point it
> becomes necessary to figure out exactly how a user might undo such an
> action, though.

When the user clicks “Don’t Allow” with “Remember” checked, we should remove all the Flash overlays. No more grey boxes.

But we will still show the crossed out plugin icon. Currently, this icon has its own doorhanger where you can unblock it. But in the future, it should open the Control Center.

The Center will show something like:

> Adobe Flash        Blocked [x]

When you click the x, I was thinking that the webpage should reload. This time, the grey boxes appear again. The user has just gone from Blocked --> Click-to-play. The user needs to explicitly interact with a Flash object and click “Allow” in order for the state to go from Click-to-play --> Allowed.

But a big caveat here: will we have time to also ship Flash management in the Control Center?



(In reply to Doug Thayer [:dthayer] from comment #11)
> […] I'm realizing it also feels weird to
> keep the same doorhanger in the case where the user has already allowed
> Flash and might want to block it. At the very least, it seems like the
> "Allow" button should be changed to "Continue Allowing" in that case.

I was hoping that putting Flash permission management in the Control Center will solve this problem. If we know that we can’t do it, then let me know, and I’ll think of a temporary workaround.



(In reply to Doug Thayer [:dthayer] from comment #15)
> Bram: one more question - do you want the "Learn More" link removed? I
> notice it wasn't mentioned in your comments, but I wasn't clear on whether
> that meant you wanted it gone or not.

I would prefer to have it removed, since the behaviour would have landed for a few versions when v57 lands. It will help simplify our doorhanger.



What do you think of these ideas?
Flags: needinfo?(bram) → needinfo?(dothayer)
Comment on attachment 8903652 [details]
Bug 1392979 - Migrate Plugin Doorhanger to PopupNotifications

https://reviewboard.mozilla.org/r/175426/#review181322

I'm a bit worried that some of the test coverage lost was implictly covering more things than multiple plug-ins support.. But I don't know if there's much to do about that

::: browser/base/content/browser-plugins.js:285
(Diff revision 1)
>          browser.messageManager.sendAsyncMessage("BrowserPlugins:NotificationShown");
>        }
>        return;
>      }
>  
> +    if (plugins.length == 1) {

When I was working on the blue badge on the icon, I think I saw a case where this was called with no plugins in the list.. Maybe it was when the notification was re-showing? Or when a plugin is removed from a page? I don't remember the details or even if I'm misremembering things.. But I just wanted to mention it to keep an eye on it..
Attachment #8903652 - Flags: review?(felipc) → review+
Flags: needinfo?(felipc)
(In reply to Bram Pitoyo [:bram] from comment #17)
> When the user clicks “Don’t Allow” with “Remember” checked, we should remove
> all the Flash overlays. No more grey boxes.
> 
> But we will still show the crossed out plugin icon. Currently, this icon has
> its own doorhanger where you can unblock it. But in the future, it should
> open the Control Center.

Which doorhanger does it currently have? I can't find a doorhanger that seems like it fits this case. The copy for all of the plugin doorhangers we show can be found here:
http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#285

Since there's no current way to get the overlays to hide without disabling a plugin completely (through about:addons), I think we'll need new copy for this.

> But a big caveat here: will we have time to also ship Flash management in
> the Control Center?

Possibly, but it's a little risky. The current code for that is very streamlined for things that work in a fairly different way than plugins/Flash.
Flags: needinfo?(dothayer)
(In reply to Doug Thayer [:dthayer] from comment #19)
> Which doorhanger does it currently have? I can't find a doorhanger that
> seems like it fits this case. The copy for all of the plugin doorhangers we
> show can be found here:
> http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.properties#285
> 
> Since there's no current way to get the overlays to hide without disabling a
> plugin completely (through about:addons), I think we'll need new copy for
> this.

Odd. I remember seeing a doorhanger that would allow you to block Flash after enabling it, but it’s been gone. Maybe even gone for a couple of months now.

Now it says “Adobe Flash is enabled on <sitename>” and it has no control (line 312)

Ideally, this control should ship inside the Control Center, but this is a special case, and we might not have time before v57.

So, what if the the doorhanger for all pluginEnabled states (line 311–313) keeps the same message, but has two extra button below it?

> [Keep Allowing]   [Don’t Allow] <-- This is the primary/blue button

Would this help solve the problem?
Flags: needinfo?(dothayer)
(In reply to Bram Pitoyo [:bram] – On PTO until 18 September 2017 from comment #20)
> So, what if the the doorhanger for all pluginEnabled states (line 311–313)
> keeps the same message, but has two extra button below it?
> 
> > [Keep Allowing]   [Don’t Allow] <-- This is the primary/blue button
> 
> Would this help solve the problem?

Works for me. I have another quick question, while I have you - the existing mechanism that hides the plugin overlays seems a bit weird to me, in that it doesn't actually hide the overlays, it just stops showing the white text (this is separate from what happens when you disable the plugin entirely). The result is that you end up with empty gray blocks, which are still clickable to show the doorhanger, but don't have the Activate Adobe Flash message. There's some heuristics governing when it hides things like this, but I wanted to know if we want to keep that behavior, since it's a little weird. You can see an example here:

http://get.adobe.com/flashplayer/about/

When Flash is click to play on this page, you'll see one large overlay, and one smaller gray box on the right. Should this just be empty white (not visible) instead? That would make it simpler to get the empty boxes for the Block + Remember case we talked about above.
Flags: needinfo?(dothayer) → needinfo?(bram)
(In reply to Bram Pitoyo [:bram] – On PTO until 18 September 2017 from comment #20)
> So, what if the the doorhanger for all pluginEnabled states (line 311–313)
> keeps the same message, but has two extra button below it?
> 
> > [Keep Allowing]   [Don’t Allow] <-- This is the primary/blue button

Drive-by: Please don't do that. In every other doorhanger, the primary button is a positive/Allow action. We've trained our users that blue means Allow and gray means Don't Allow. It's consistent right now, please don't make a precedence for inconsistency. It's not worth it.
Attachment #8903652 - Attachment is obsolete: true
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0ccba7a4ba1
Modernize buttons for Flash CTP r=Felipe
Blocks: 1398972
I created 1398972 to track the more substantive work on modernizing this doorhanger. For 57 I think the safest and best option is just to modernize the styles, so I am landing the patch which only does that.
https://hg.mozilla.org/mozilla-central/rev/f0ccba7a4ba1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1399172
Flags: needinfo?(bram)
Hi Doug, sorry for the late reply, as I just returned from PTO.

(In reply to Doug Thayer [:dthayer] from comment #21)
> […] the existing mechanism that hides the plugin overlays seems a bit weird
> […] The result is that you end up with empty gray blocks, which are still
> clickable to show the doorhanger, but don't have the Activate Adobe Flash
> message […]
> 
> When Flash is click to play on this page, you'll see one large overlay, and
> one smaller gray box on the right. Should this just be empty white (not
> visible) instead? That would make it simpler to get the empty boxes for the
> Block + Remember case we talked about above.

Instead of showing a blank grey block when there’s not enough space to show icon + text, I’d prefer showing only the icon, like so: https://cl.ly/2T3p010U3H2q

But I’m not very picky if we can’t implement it, because there’s already a Control Center icon that the user can interact with even if there’s no grey block. If empty white box is a simpler solution, let’s go with it.


(In reply to Johann Hofmann [:johannh] from comment #22)
> Drive-by: Please don't do that. In every other doorhanger, the primary
> button is a positive/Allow action. We've trained our users that blue means
> Allow and gray means Don't Allow. It's consistent right now, please don't
> make a precedence for inconsistency. It's not worth it.

Agreed completely, and apologies for not checking about this beforehand. If positive actions are, without exception, always located in the primary button position, then this dialogue box shouldn’t break that.
Depends on: 1403733
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.