If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

It is too easy to enable flash for sites

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: kbrosnan, Assigned: Margaret)

Tracking

({uiwanted, ux-error-prevention})

Trunk
Firefox 15
ARM
Android
uiwanted, ux-error-prevention
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, blocking-fennec1.0 betaN+, fennec+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
From the release sign off enabling Flash is too easy. The door hanger tends to get the user to enable flash for the whole site.
Click-to-play in desktop Firefox just puts an icon in the urlbar.  The doorhanger doesn't appear unless you click the icon.  We could do the same in Fennec, especially if we implemented a general method for collapsing and re-showing doorhangers (bug 700437).
Depends on: 700437
Keywords: uiwanted, ux-error-prevention
(Reporter)

Updated

5 years ago
blocking-fennec1.0: --- → ?
tracking-fennec: ? → +
blocking-fennec1.0: ? → betaN+
Margaret - What do you think about hiding the doorhanger if any flash plugin is ivisible on the page. So we only show the doorhanger if the plugins on the page are not visible or too small to show the placeholder.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 3

5 years ago
This is a tricky issue. You'll want to see bug 734323.

Previously, we would wait until the load event to decide whether or not to show the doorhanger, but the problem was that we wouldn't end up showing the doorhanger if an invisible plugin was added to the DOM after the load event had already fired. The solution to this was to decide whether or not to show the doorhanger for each PluginClickToPlay event.

We could add some logic to see whether or not we've already processed a visible plugin, and not show the doorhanger in that case, but if an invisible plugin is added before a visible one, we'll still end up showing the doorhanger. I think this is probably as good of a solution as we'll be able to get.

Adding Jared in case he has ideas.
Warning: Hack Ahead

What if we delayed sending the doorhanger request in the PluginClickToPlay event. If we saw an invisible plugin and delayed 500ms, and then a visible plugin was found - we could cancel the timeout on the doorhanger request. We would only do the delay while the page was loading, by using "oad" or "pageshow" or something.

I did warn you!
Yeah, we could do a hack like that. On another note, we could remove the checked-by-default site-wide enabling of Flash.
(Reporter)

Comment 6

5 years ago
UX had issue with that as the user would get this notification every time they went to a site (with flash).
Then it looks like we will just have to bump up the priority for bug 700437.
(Assignee)

Comment 8

5 years ago
If we're going to need bug 700437 to fix this completely, I don't think this should be a beta blocker. I think we could try to land mfinkle's hack idea for beta, but bug 700437 feels to risky at this point, especially since the UX for it isn't spec-ed out yet.
(Assignee)

Comment 9

5 years ago
Created attachment 626094 [details] [diff] [review]
only show the doorhanger if visible plugins don't get added in the near future

The logic is a little tricksy here, so I tried to make sure the comments explained things well enough.

To test this, I made a test page that includes an invisible plugin, then adds a visible plugin after the page has finished loading: http://people.mozilla.com/~mleibovic/test/invisible_and_visible_embeds.html.

I also checked some of Martijn's many test cases to make sure this didn't regress other behavior: http://people.mozilla.org/~mwargers/tests/plugins/flash/.
Attachment #626094 - Flags: review?(mark.finkle)
Attachment #626094 - Flags: feedback?(jaws)
Comment on attachment 626094 [details] [diff] [review]
only show the doorhanger if visible plugins don't get added in the near future

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

::: mobile/android/chrome/content/browser.js
@@ +2120,5 @@
> +          if (!this.pluginDoorhangerTimeout) {
> +            this.pluginDoorhangerTimeout = setTimeout(function() {
> +              if (!this.clickToPlayPluginDoorhangerShown && !this.pluginOverlayVisible)
> +                PluginHelper.showDoorHanger(this);
> +            }.bind(this), 500);

We're going to need to clearTimeout on this on pagehide.
Attachment #626094 - Flags: feedback?(jaws) → feedback+
Comment on attachment 626094 [details] [diff] [review]
only show the doorhanger if visible plugins don't get added in the near future

Agreed about the clearTimeout comment. Also, could we somehow combine clickToPlayPluginDoorhangerShown and pluginOverlayVisible into a single flag? I cringe at adding one-off boolean flags.
(Assignee)

Comment 12

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 626094 [details] [diff] [review]
> only show the doorhanger if visible plugins don't get added in the near
> future
> 
> Agreed about the clearTimeout comment. Also, could we somehow combine
> clickToPlayPluginDoorhangerShown and pluginOverlayVisible into a single
> flag? I cringe at adding one-off boolean flags.

We use clickToPlayPluginDoorhangerShown to decide if we need to make a NativeWindow.doorhanger.hide() call. If we combine the flags, we'd end up making that call a lot more, which means a message to Java, and I'm not sure if it's worth it.

Actually, I just discovered that we can run into a NPE in Tab.removeDoorHanger() if we try to remove a non-existent doorhanger, and that's pretty bad, especially since that's theoretically possible in our current code. I'll file a bug to fix that ASAP.
(Assignee)

Comment 13

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #12)

> Actually, I just discovered that we can run into a NPE in
> Tab.removeDoorHanger() if we try to remove a non-existent doorhanger, and
> that's pretty bad, especially since that's theoretically possible in our
> current code. I'll file a bug to fix that ASAP.

Nevermind, I'm dumb. HashMap's remove method won't throw, so this isn't an issue.
(Assignee)

Comment 14

5 years ago
Created attachment 626281 [details] [diff] [review]
updated patch

This patch combines the flags into a single shouldShowPluginDoorhanger flag. However, this has the negative quality that we'll always be making a call to hide the doorhanger when the user clicks on an overlay to activate a plugin (keeping a check for !shouldShowPluginDoorhanger doesn't really make sense, since that should always be true if the user is clicking on a visible plugin).

Up to you, I can switch back to the two flags, or keep this as is
Attachment #626094 - Attachment is obsolete: true
Attachment #626094 - Flags: review?(mark.finkle)
Attachment #626281 - Flags: review?(mark.finkle)
Attachment #626281 - Flags: feedback+
Comment on attachment 626281 [details] [diff] [review]
updated patch

I'm OK with sending the message to hide the doorhanger unconditionally. It shouldn't be a performance impact. The Tab class is becoming large (lots of data memebrs) and I worry about maintainability. I can file a new bug about re-organizing it a bit.
Attachment #626281 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e017a0a0f6b
Target Milestone: --- → Firefox 15

Updated

5 years ago
Blocks: 757218

Updated

5 years ago
No longer blocks: 757218
https://hg.mozilla.org/mozilla-central/rev/8e017a0a0f6b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 626281 [details] [diff] [review]
updated patch

[Triage Comment]
Attachment #626281 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6a19f69f6f95
status-firefox14: --- → fixed
status-firefox15: --- → fixed
This works exactly like it is described in comment #15 on all branches.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-31)
Firefox 14.0a2 (2012-05-31)
Firefox 14.0b4 (2012-05-31)

Device: Samsung Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.