741139 - Plugin placeholders wrapped in links can't be activated

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

You need to have the "Tap to Play" setting enabled for plugins for this bug.

See url testcase, steps to reproduce:
- Tap on the 2nd or 3rd plugin placeholder.

Notice how you the page is going to http://google.com, because there is a link around the plugin placeholder.

I don't know what should be happening in this case, to be honest.
But it is impossible to activate the Flash plugin in this case.
I've seen this kind of markup on several site, related to Flash ads.

Comment 1

[:Margaret Leibovic]

Hm, we're already capturing the click event during the capturing phase, and we're already calling preventDefault() here:

https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1951

I think instead of preventDefault() we may want stopPropagation(), since I don't think we want other elements to be finding out about this click. We could try that and see what happens.

Comment 2

[:Margaret Leibovic]

Attachment: patch

It appears we need _both_ preventDefault() and stopPropagation().

I'm not sure exactly how preventDefault() works, but my guess is that it's not working the way we want here because the event handler is on a child of the link, not on the link itself. I'm also not sure why stopPropagation() by itself doesn't work (I tried, it doesn't), since I would think the link wouldn't find out about the event if we're calling stopPropagation(). All in all, this is confusing, but this patch seems to do the job.

Comment 3

[:Margaret Leibovic]

Attachment: patch

(Un-bitrotted to apply on top of the patch from bug 741128)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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

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

Margaret, can you add a mochitest for this?

Comment 5

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 612631 [details] [diff] [review]
> patch
> 
> Review of attachment 612631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Margaret, can you add a mochitest for this?

Sigh, we don't have mochitests for this mobile click-to-play UI. I saw you made lots of tests in bug 711552, which we should try to port over. However, I'm not sure if we've developed a way to add UI tests that interact with content, since our UI tests are all Java-based, so this may require some effort to figure out.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Oh ok, that's too bad.

Comment 7

[Martijn Wargers (dead)]

The testcase appears to work now in current trunk build (2012-04-14) on the Samsung Galaxy Nexus.

Comment 8

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

Margaret - Has this been fixed by a different patch?

Comment 9

[:Margaret Leibovic]

I also can't reproduce this issue anymore, although I'm not sure what else would have fixed it.

Comment 10

[Martijn Wargers (dead)]

I can reproduce it again with at least a trunk build from 30-4 (but also today's build) on the Galaxy Nexus. I haven't seen it on the Galaxy SII.

Comment 11

[Martijn Wargers (dead)]

Never mind, I had Plugins disabled, so I guess that would be expected behavior.
There is not much visible difference between disabled plugin placeholder and click to play placeholder, btw.

Comment 12

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #11)
> Never mind, I had Plugins disabled, so I guess that would be expected
> behavior.
> There is not much visible difference between disabled plugin placeholder and
> click to play placeholder, btw.

Stephen Horlander was working on a new icon for the click to play overlay. Jared may know if that ended up in a bug somewhere.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Yes, there are plans to change the click-to-play icon to have a play triangle in it. See https://bugzilla.mozilla.org/show_bug.cgi?id=746859 for more details.