719875 - "Tap to activate plugin" placeholder can't be tapped after going back/forward in history

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Martijn Wargers (dead)]

Tested on the LG Optimus Black, Android 2.2.2
You need to have the Setting Plugins to "Tap to Play".

Steps to reproduce:
- Go to http://people.mozilla.org/~mwargers/tests/plugins/flash/
- Tap on the "flashembed.html" link
- Tap on the Android back button
- Press on the Android context button
- Tap on "Forward"
- Tap on the "Tap to activate plugin" placeholder.

Expected result:
- Flash embed gets activated, blue circles can be seen

Actual result:
- "Tap to activate plugin" placeholder stays there.

Comment 1

[:Margaret Leibovic]

What's happening here is that we're clearing _pluginsToPlay on pagehide when we navigate back, but then when we navigate forward, we don't get PluginClickToPlay events for the plugin objects because we're loading the page from the cache.

I'm not sure what the best way to fix this is. We somehow need to restore _pluginsToPlay when we navigate through session history with nsIWebNavigation's goForward()/goBack(). Maybe we can store this with session history entries? Finkle, do you have any ideas?

Comment 2

[:Margaret Leibovic]

Alternatively, we could give up on storing plugins in _pluginsToPlay, and instead look through the DOM for plugin objects in playAllPlugins(). I'm not sure how much more inefficient/error-prone this might be, but I could explore this idea.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> What's happening here is that we're clearing _pluginsToPlay on pagehide when
> we navigate back, but then when we navigate forward, we don't get
> PluginClickToPlay events for the plugin objects because we're loading the
> page from the cache.

Seems like you would need a map of documents/pugins. We do something similar in ViewportHandler with a weak map of documents which allow the documents to live even when cached. The array of plugins could be contained with the associated document. Maybe. I don't know how life cycle would affect that idea. You can use "pageshow" / "pagehide" event.persisted to know if the document came from cache.

> I'm not sure what the best way to fix this is. We somehow need to restore
> _pluginsToPlay when we navigate through session history with
> nsIWebNavigation's goForward()/goBack(). Maybe we can store this with
> session history entries? Finkle, do you have any ideas?

Storing with session history seems overkill to me.

(In reply to Margaret Leibovic [:margaret] from comment #2)
> Alternatively, we could give up on storing plugins in _pluginsToPlay, and
> instead look through the DOM for plugin objects in playAllPlugins(). I'm not
> sure how much more inefficient/error-prone this might be, but I could
> explore this idea.

This idea would seem to be do-able. You just need fast access to the plugins and be iframe-aware.

Comment 4

[:Margaret Leibovic]

Attachment: WIP

This is my attempt at my idea from comment 2, but I still need to figure out how to deal with the doorhanger notifications for hidden plugins. If we want to get rid of state, I should be getting rid of _pluginOverlayShowing as well, but then I'm going to need to do something else to decide whether or not we should show a doorhanger.

In practice, I would think most sites wouldn't have deeply nested sub-frames, but I guess this could theoretically be slow. I also think I should look into removing the playAllPlugins click listener if possible, since we're now doing more work after the click.

Comment 5

[:Margaret Leibovic]

Thinking about this more, I feel like the best solution would be to re-fire PluginClickToPlay events when we navigate back/forward to a page with click-to-play plugins that haven't been clicked-to-play. We could still look into cleaning up the way we collect plugin objects to play, but if we're not getting these events again, I think we'd have to search for plugins without clickable overlays when we load the page in order to decide if we should show the hidden plugin doorhanger, and that sucks.

Josh, I know bug 90268 is landing soon and that will probably affect these code paths. Do you know if there's a good way to re-fire these PluginClickToPlay events when loading a page from the cache?

Also, this is a kinda messy situation for native fennec, since we'd probably need a totally different patch for aurora/beta if we wanted to uplift a change like this. Maybe for aurora/beta I can change the way we find plugins to play so that tapping to play will work, but I don't think there's a good way to decide to show the hidden plugin doorhanger without re-firing PluginClickToPlay events. Since that's already broken right now and seems like even more of an edge case, perhaps we can just live with it for aurora/beta.

Comment 6

[:Margaret Leibovic]

Attachment: patch

This patch fixes the issue for plugins with visible overlays, but we still won't show the doorhanger for hidden plugins on back/forward navigation. This is definitely an improvement over the current state of things, and it probably addresses the P2-ness of this bug, but we'll want to look into a platform fix to totally fix the issue.

To avoid calling playAllPlugins more than we want, I realized we can just set the click listener on the overlay instead of the plugin, since the overlays all conveniently disappear once playAllPlugins has been called. Calling playAllPlugins on every click was a problem we had before, but we just fixed it by checking the length of _pluginsToPlay, which is a solution we can't use anymore (and it's sub-optimal anyway). I still decided to set a "played" attribute on the plugins, in case more plugin objects get added to the page later and the user tries to play them (we wouldn't want to accidentally re-load plugins that are already playing).

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 593197 [details] [diff] [review]
patch

Sounds like the platform code to re-fire the event is the ideal solution.

Comment 8

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 593197 [details] [diff] [review]
> patch
> 
> Sounds like the platform code to re-fire the event is the ideal solution.

I filed bug 723617 about this.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3dfff9cb09a

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f3dfff9cb09a

Comment 11

[:Margaret Leibovic]

Comment on attachment 593197 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Tap to play plugins don't work on pages loaded from the cache 
Testing completed (on m-c, etc.): Landed on m-c, needs to be verified
Risk to taking this patch (and alternatives if risky): This changes the way we find plugins to play when the user taps on a plugin, so there's some risk, but I think we need to take this and just be on the lookout for any regressions

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 593197 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f3329e303fe

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d7899addb8fc

Comment 15

[Andreea Pod]

Verified fixed on builds:
Firefox 11 (tinderbox build): 	1328588649/	06-Feb-2012 21:51 		
Firefox 12: Firefox 12.0a2 (2012-02-07)
Firefox 13: Firefox 13.0a1 (2012-02-07)

Device: LG Optimus 2X (Android 2.2.2)