Closed Bug 752516 Opened 8 years ago Closed 7 years ago

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

Categories

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

13 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: tomer, Assigned: gfritzsche)

References

Details

(Whiteboard: [CtPDefault:P2])

Attachments

(2 files)

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.
Component: Untriaged → Plug-ins
Product: Firefox → Core
QA Contact: untriaged → plugins
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.
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.
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.
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.
Priority: -- → P3
Duplicate of this bug: 835057
OS: Linux → All
Hardware: x86 → All
Summary: Unable to communicate with external plugins if there is an overlay above the object → Unable to activate click-to-play overlay if there is an overlay above the plugin element
Duplicate of this bug: 835057
Duplicate of this bug: 809785
Whiteboard: [CtPDefault:P3]
Duplicate of this bug: 804580
Duplicate of this bug: 853973
Duplicate of this bug: 855323
This seems to cause a fair amount of bustage, should this be CtPDefault:P2?
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.
Whiteboard: [CtPDefault:P3] → [CtPDefault:P2]
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
(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.
(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?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
Blocks: 838999
Layering the binding on top. 
r?bsmedberg for the toolkit part.
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Attachment #741268 - Flags: review?(benjamin)
Comment on attachment 741268 [details] [diff] [review]
Layer the CtP binding on top

r?jaws for the browser/ test.
Attachment #741268 - Flags: review?(jaws)
Attachment #741268 - Flags: review?(benjamin) → review+
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.
Attachment #741268 - Flags: review?(jaws) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/606109f58516
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
Actually 3. is not a regression of this bug
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Flags: needinfo?(bzbarsky)
I don't know of a way to alter the behavior the way we want here, but roc might...
Flags: needinfo?(bzbarsky) → needinfo?(roc)
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?
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
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?
Flags: needinfo?(roc)
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?
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 867760
Note: we will look into the remaining issues in bug 853973.
Depends on: 870227
Blocks: 976769
You need to log in before you can comment on or make changes to this bug.