752516 - Unable to activate click-to-play overlay if there is an overlay above the plugin element

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Tomer Cohen :tomer]

Attachment: testcase – Try to activate the Flash object while plugins.click_to_play is set to true

I find it too common for websites to use Flash for the graphics on a page, while using plain HTML for the navigation menus. Because they prefer to place the navigation menus above the flash object, it makes the user unable to communicate with the flash object, as the HTML container catch the mouse clicks. 

While web authors know this limitations and make sure that there is no need to click on the object, I know two issues reproducible in the wild where the user will be required to click on the flash object:

• "Plugin is not installed. Click to install plugin." link
• "Plugins are disabled. Click to activate plugin" link

Steps to reproduce:
a. Install Firefox on a clean machine that doesn't have flash installed.
b. visit http://www.iec.co.il
c. Click on the upper flash object, in order to install flash.
d. Not clickable. (The other flash object still accessible).

Or —
a. Disable plugins (about:config → plugins.click_to_play)
b. visit http://www.iec.co.il
c. Click on the upper flash object, in order to install flash.
d. Not clickable. (The other flash object still accessible).



A simpler testcase is attached.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I don't think this is something we can fix (particularly if/when we attach the click listeners to the plugin elements themselves, as in bug 741130/bug 765506). There are many ways a site can make a plugin invisible, un-clickable, etc. and prevent the intuitive "click here" interaction from working. That's why we have the url bar icon/notification as a backup.

Comment 2

[Tomer Cohen :tomer]

We can actually show the notification message above any other overlay layer using z-index (for the notification itself, not the plugin), but I am not sure if it won't break something else this way.

Comment 3

[Georg Fritzsche [:gfritzsche]]

I don't think putting the overlay layer on top is the right thing: it would lead to e.g. the nav elements being hidden in the example page.

In general we have the problem here that we don't know whether the plugin element will respect z-index without activating it (windowed vs. window-less, which is negotiated at plugin instantiation time).
I think we should be able to extend the "hidden plugin" detection from bug 810082 to cover the case where the C2P overlay is covered in other layers (partially, completely?).
Additionally we could adjust the overlay UI for that case to avoid confusion from trying to click on it.

Comment 4

[Benjamin Smedberg]

We could certainly special-case Flash by inspecting the wmode parameters. If the page hasn't specified wmode="transparent" or "opaque" then it expects the Flash to sit above the page and we can probably safely do the same thing with the CtP overlay.

I'm not certain that this special-case is worth the effort though (note that it wouldn't apply on mac anyway, since plugins are always windowless/part of the z-order on mac). But for right now I'm going to mark this P3 and we can come back to it later.

Comment 11

[John Schoenick [:johns]]

This seems to cause a fair amount of bustage, should this be CtPDefault:P2?

Comment 12

[Benjamin Smedberg]

Yeah. Let's just use zindex on the binding to move it topmost. The user can use the X button can solve the problems from comment 3.

Comment 13

[Georg Fritzsche [:gfritzsche]]

The linked example [1] in the dupe bug 8 has a variation of this scenario:
Two <table>s, one being z-indexed over the other. The upper table contains a transparent image element, the lower one a Flash element.

In this case just z-indexing the binding wouldn't fix it - or am i missing something?

[1]: http://event.on24.com/utils/test/testYourSystem.html?checkBrowser=true&checkOS=true&checkBandwidth=true&checkCookie=true&checkflash=true&flashtype=flv&mediatype=audio&text_language_id=en&isSilverlightEnabled=true&mac=true&flashconsole=true

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> In this case just z-indexing the binding wouldn't fix it - or am i missing
> something?

We need to adress this separately, breaking this out into re-opened bug 853973.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Yeah. Let's just use zindex on the binding to move it topmost. The user can
> use the X button can solve the problems from comment 3.

I did a quick check and that doesn't solve the problem in this test-case: it actually sets the z-index on a wrapper element, not the <object> itself.
Wrappers for Flash elements seem to be rather common, at least judging from previous CtP bugs etc. (e.g. for all those video-players that dynamically insert the Flash player).

The next-best thing i can think of is:
* walk up the ancestors (maybe max N) until we find one with non-auto z-index
* if we found one, store it's z-index, z-index it to the top
On removing the binding we would restore the original z-index.

Would that be acceptable? Or were you thinking of a better way Benjamin?

Comment 16

[Benjamin Smedberg]

I'm not sure why a wrapper element would change this behavior at all.

We shouldn't change the z-index of the <object> element itself. We should make sure the inner box (id="mainBox") has relative positioning and change the z-index of *that*. And use the maximum possible z-index so that it's always higher than anything on the page.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: Layer the CtP binding on top

Layering the binding on top. 
r?bsmedberg for the toolkit part.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 741268 [details] [diff] [review]
Layer the CtP binding on top

r?jaws for the browser/ test.

Comment 19

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=1b4c6e8a3da5

Comment 20

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

Comment on attachment 741268 [details] [diff] [review]
Layer the CtP binding on top

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

Did you run this test without the pluginProblemContent.css change? I just want to make sure that the click event isn't being simulated against the actual element versus the coordinates of the center of the element. It should be sending a click event to the coordinates of what would be the center of the element, but not actually setting the plugin element as the target of the event (I hope that makes sense :) ).

::: browser/base/content/test/browser_bug752516.js
@@ +29,5 @@
> +  // The plugin events are async dispatched and can come after the load event
> +  // This just allows the events to fire before we proceed
> +  executeSoon(actualTest);
> +}
> +  

nit: whitespace

@@ +38,5 @@
> +  ok(!objLoadingContent.activated, "Plugin should not be activated");
> +
> +  EventUtils.synthesizeMouseAtCenter(plugin, {}, gTestBrowser.contentWindow);
> +  let condition = function() objLoadingContent.activated;
> +  waitForCondition(condition, pluginActivated, "Waited too long for plugin to activate");

I think this can just be:
waitForCondition(condition, finish, "Waited too long for plugin to activate.");

The pluginActivated function below seems redundant to me.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Jared Wein [:jaws] from comment #20)
> Did you run this test without the pluginProblemContent.css change? I just
> want to make sure that the click event isn't being simulated against the
> actual element versus the coordinates of the center of the element. It
> should be sending a click event to the coordinates of what would be the
> center of the element, but not actually setting the plugin element as the
> target of the event (I hope that makes sense :) ).

Good point, i actually checked that :)

Addressed the nits.

http://hg.mozilla.org/integration/mozilla-inbound/rev/606109f58516

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/606109f58516

Comment 23

[Paul Silaghi, QA [:pauly]]

23.0a1 (2013-04-29) Win 7
This is not entirely fixed. The testcase works fine, but:
1. The link in comment 0 doesn't work
2. The duplicates: 804580 809785 855323 are not fixed
3. If the plugin is vulnerable, no CTP overlay is displayed for the testcase, bug 809785

Comment 24

[Paul Silaghi, QA [:pauly]]

Actually 3. is not a regression of this bug

Comment 25

[Georg Fritzsche [:gfritzsche]]

So this fixes only the attached test-case :(
With that i think we're back to comment 15 (or 853973, comment 4) and, i think, the same issue as bug 853973.

Comment 26

[Benjamin Smedberg]

Ah, I see the problem, I misunderstood zindex:

"Within a stacking context, child elements are stacked according to the same rules previously explained. Importantly, the z-index values of its child stacking contexts only have meaning in this parent. Stacking contexts are treated atomically as a single unit in the parent stacking context." https://developer.mozilla.org/en-US/docs/CSS/Understanding_z-index/The_stacking_context

bz, is there a way to alter the normal behavior in this case, or are we stuck?

If we are stuck, are there reliable ways to detect this situation so we can suppress the normal interactive UI and fall back to the "hidden plugin" behavior of popping up the doorhanger more aggressively?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I don't know of a way to alter the behavior the way we want here, but roc might...

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could fix this in some cases, but if for example the plugin is in a container element with a CSS transform, it's not readily possible to hoist a descendant out of the transformed element.

Maybe you could call getBoundingClientRect on the plugin, call elementFromPoint on the center of the rect (or some set of sample points in the rect), and if some of them don't return the plugin, activate your backup UI?

Comment 29

[Georg Fritzsche [:gfritzsche]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> We could fix this in some cases

Any hints on which cases we could fix and how? :)
CSS transforms are probably not common for the cases we want to address.
If the fixes are not too complex, we could probably just do both them and the visibility-check.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If someone just used CSS z-index on an ancestor element, but not CSS transforms or opacity or other stuff, then we could try to provide a feature that lets your element break out of the ancestor's stacking context. It would be ugly though, and I'm not all that keen on it :-).

Also, I hope you only want this on trunk. Or do you need something for branches too?

Comment 31

[Georg Fritzsche [:gfritzsche]]

Thanks, i don't think we want to go into ugly specific features just for this.

Falling back to the "hidden plugin" behaviour based on elementFromPoint sounds good to me then. Any objections bsmedberg?

Comment 32

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

I just want to state that I was aware that the patch I reviewed wasn't going to handle other cases than the one in the test case.

I think we can keep this bug closed as RESO-FIXED, and then file a separate bug for the elementFromPoint work.

Comment 33

[Georg Fritzsche [:gfritzsche]]

Note: we will look into the remaining issues in bug 853973.