Invisible plugins added to the DOM after the "load" event will not prompt a doorhanger to appear

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
Firefox 14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 +)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The logic for determining if a doorhanger should appear is handled during the "load" event. Any invisible plugins added to the DOM after the load event will not trigger the doorhanger.

One way to solve this is to add two booleans to the ?Tab? to keep track of the state of the doorhanger: a boolean stating if the doorhanger has been shown for this page, and another boolean stating if the "load" event has been processed.

If we get a "PluginClickToPlay" event and the "load" event has been processed while the doorhanger hasn't been shown for this page, then we should show the doorhanger.

Updated

6 years ago
blocking-fennec1.0: --- → ?

Comment 1

6 years ago
I think this is pretty edge-case-y, so I don't think it needs to block. Jared, have you seen this problem in the wild at all, or is this just theoretical?
This is theoretical, just trying to cover all bases. I don't think this should block either.
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → +
Brad: This bug depends on bug 730318 which was marked as blocking-fennec1.0-. Are you sure you want this bug to block 1.0? I hope to land bug 730318 soon, but blocking flag difference between the two bugs is odd.
fixed the blocking flag on bug 730318

Comment 5

6 years ago
Brad, to clarify, this issue doesn't exist until bug 730318 lands, so if we don't want to make bug 730318 a blocker, this doesn't actually need to block.
bug 730318 should legitimately block on its own merits. Having the click-to-play be hokey has caused me to enable flash always on my GN. That really isn't an option on pre-Honeycomb devices.

Comment 7

6 years ago
Well, okay then, we'll fix it all!
Yoink!
Assignee: margaret.leibovic → jwein
Status: NEW → ASSIGNED
Blocks: 735833
Created attachment 606026 [details] [diff] [review]
Patch for bug (apply on top of patch for bug 730318)

This patch implements the necessary behavior to invisible plugins that are added to the DOM after the 'load' event.
Attachment #606026 - Flags: review?(margaret.leibovic)

Comment 10

6 years ago
Comment on attachment 606026 [details] [diff] [review]
Patch for bug (apply on top of patch for bug 730318)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // Track state of click-to-play plugin notifications.
>+    this.clickToPlayPluginDoorhangerShown = false;
>+    this.loadEventProcessed = false;

I think you should declare these in Tab(), like we used to do for |this._pluginCount| and |this._pluginOverlayShowing|, since that's where we initialize similar boolean flags.

>         // If the overlay is too small, hide the overlay and act like this
>         // is a hidden plugin object
>-        if (PluginHelper.isTooSmall(plugin, overlay)) {
>-          overlay.style.visibility = "hidden";
>+        if (!overlay || PluginHelper.isTooSmall(plugin, overlay)) {
>+          if (overlay)
>+            overlay.style.visibility = "hidden";
>+          if (this.loadEventProcessed && !this.clickToPlayPluginDoorhangerShown)
>+            PluginHelper.showDoorHanger(this);

Thinking really edge-case-y here, if another invisible plugin is added even later, we won't show another doorhanger. That being said, I don't think we want to be showing multiple doorhangers, since that's annoying, so maybe this is the best we can do. What do you think?

>@@ -1914,16 +1920,20 @@ Tab.prototype = {
>     let uri = browser.currentURI.spec;
>     let documentURI = "";
>     let contentType = "";
>     if (browser.contentDocument) {
>       documentURI = browser.contentDocument.documentURIObject.spec;
>       contentType = browser.contentDocument.contentType;
>     }
> 
>+    // Reset state of click-to-play plugin notifications.
>+    this.clickToPlayPluginDoorhangerShown = false;
>+    this.loadEventProcessed = false;

Doing this on location change, are we going to run into any of those issues we were having before with the bfcache? I haven't thought this through, and I was just wondering if you have :)

r=me, with comments addressed/pondered.
Attachment #606026 - Flags: review?(margaret.leibovic) → review+
(In reply to Margaret Leibovic [:margaret] from comment #10)
> Comment on attachment 606026 [details] [diff] [review]
> Patch for bug (apply on top of patch for bug 730318)
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+    // Track state of click-to-play plugin notifications.
> >+    this.clickToPlayPluginDoorhangerShown = false;
> >+    this.loadEventProcessed = false;
> 
> I think you should declare these in Tab(), like we used to do for
> |this._pluginCount| and |this._pluginOverlayShowing|, since that's where we
> initialize similar boolean flags.

Oh, that's a better place than where I was adding these lines. I'll move them.

> >         // If the overlay is too small, hide the overlay and act like this
> >         // is a hidden plugin object
> >-        if (PluginHelper.isTooSmall(plugin, overlay)) {
> >-          overlay.style.visibility = "hidden";
> >+        if (!overlay || PluginHelper.isTooSmall(plugin, overlay)) {
> >+          if (overlay)
> >+            overlay.style.visibility = "hidden";
> >+          if (this.loadEventProcessed && !this.clickToPlayPluginDoorhangerShown)
> >+            PluginHelper.showDoorHanger(this);
> 
> Thinking really edge-case-y here, if another invisible plugin is added even
> later, we won't show another doorhanger. That being said, I don't think we
> want to be showing multiple doorhangers, since that's annoying, so maybe
> this is the best we can do. What do you think?

The doorhanger is already visible. If the user has denied activation, we won't show another doorhanger which is OK. This did make me realize that I'm not handling the case where the user allows activation. In that scenario, we should auto-activate upon receiving this event. I'll fix the patch to handle that case.

> >@@ -1914,16 +1920,20 @@ Tab.prototype = {
> >     let uri = browser.currentURI.spec;
> >     let documentURI = "";
> >     let contentType = "";
> >     if (browser.contentDocument) {
> >       documentURI = browser.contentDocument.documentURIObject.spec;
> >       contentType = browser.contentDocument.contentType;
> >     }
> > 
> >+    // Reset state of click-to-play plugin notifications.
> >+    this.clickToPlayPluginDoorhangerShown = false;
> >+    this.loadEventProcessed = false;
> 
> Doing this on location change, are we going to run into any of those issues
> we were having before with the bfcache? I haven't thought this through, and
> I was just wondering if you have :)

We shouldn't run in to issues with the bfcache as I understand it. More testing on this would be good though.
Created attachment 607387 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

This is mainly the previous r+'d patch, but with a change to auto-activate newly added plugins if plugins have already been activated for the page while the page is loaded.
Attachment #606026 - Attachment is obsolete: true
Attachment #607387 - Flags: review?(margaret.leibovic)

Comment 14

6 years ago
Comment on attachment 607387 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>       case "PluginClickToPlay": {
>         let plugin = aEvent.target;
>         let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>-        if (!overlay)
>+
>+        if (this.clickToPlayPluginsActivated) {
>+          PluginHelper.playPlugin(plugin);
>           return;
>+        }

Nit: Put this above the overlay variable declaration, since that isn't used until below.

>         // Add click to play listener to the overlay
>         overlay.addEventListener("click", function(e) {
>           if (e) {
>             if (!e.isTrusted)
>               return;
>             e.preventDefault();
>           }
>+          let tab = BrowserApp.getTabForWindow(e.target.ownerDocument.defaultView.top);
>+          tab.clickToPlayPluginsActivated = true;
>           PluginHelper.playAllPlugins(e.target.ownerDocument.defaultView);

Factor out e.target.ownerDocument.defaultView? Also, why are you setting clickToPlayPluginsActivated on defaultView.top, but only passing defaultView to playAllPlugins?
Created attachment 607820 [details] [diff] [review]
Patch for bug v3 (apply on top of patch for bug 730318)

Nice catch with the wrong parameter being passed to playAllPlugins. Thank you for noticing that :)
Attachment #607387 - Attachment is obsolete: true
Attachment #607820 - Flags: review?(margaret.leibovic)
Attachment #607387 - Flags: review?(margaret.leibovic)

Comment 16

6 years ago
Comment on attachment 607820 [details] [diff] [review]
Patch for bug v3 (apply on top of patch for bug 730318)

(In reply to Jared Wein [:jaws] from comment #15)

> Nice catch with the wrong parameter being passed to playAllPlugins. Thank
> you for noticing that :)

Haha, that's what review is for! :)
Attachment #607820 - Flags: review?(margaret.leibovic) → review+
And a much greener try run after I fixed a minor issue with the patch that this patch depends on:
https://tbpl.mozilla.org/?tree=Try&rev=1f7fe227e41d
https://hg.mozilla.org/mozilla-central/rev/c120dd831b3f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out on inbound since the patch for bug 730318 caused bustage on b2g builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85fe5da4eca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch was backed out due to test failures. I'm investigating now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/26808dda208c
https://hg.mozilla.org/mozilla-central/rev/265ae1eb0189
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.