Closed Bug 853973 Opened 7 years ago Closed 6 years ago

Click-to-play doesn't work if overlayed by an element in a separate sub-tree

Categories

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

x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla29

People

(Reporter: alex_mayorga, Assigned: benjamin)

References

()

Details

Attachments

(2 files, 3 obsolete files)

Steps:
0. Set plugins.click_to_play;true in about:config
1. Load 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
2 Click on "Activate Adobe Flash"

Result:
Nothing happens.

Expected result:
The Flash video plays.

Initial report:
http://forums.mozillazine.org/viewtopic.php?p=12747617#p12747617
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:22.0) Gecko/20130321 Firefox/22.0

Confirmation:
http://forums.mozillazine.org/viewtopic.php?p=12749465#p12749465
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Looks like there's a transparent image and/or some other interfering things over the plugin element.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 752516
Re-opening as this is slightly different (and so we can address bug 752516 independently):
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 simply z-indexing the CtP binding up doesn't help.

Gavin, i was told you might have possible insight into whether there is a way to address this via the binding (going into the content and start patching z-indices up there is probably out of scope for us).
Status: RESOLVED → REOPENED
Flags: needinfo?(gavin.sharp)
Resolution: DUPLICATE → ---
Priority: -- → P3
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Gavin, i was told you might have possible insight into whether there is a
> way to address this via the binding (going into the content and start
> patching z-indices up there is probably out of scope for us).

Oops, thought I had commented here already :(

Offhand, I have no idea. Seems like it might require some platform support for a special kind of "always on top" widget.
Flags: needinfo?(gavin.sharp)
Summary: Click on "Activate Adobe Flash" doesn't activate Flash content → Click-to-play doesn't work if overlayed by an element in a separate sub-tree
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> > Gavin, i was told you might have possible insight into whether there is a
> > way to address this via the binding (going into the content and start
> > patching z-indices up there is probably out of scope for us).
> 
> Offhand, I have no idea. Seems like it might require some platform support
> for a special kind of "always on top" widget.

Thanks, i think this out of scope for us then.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → WONTFIX
For who? Do we have any idea how common this is? WONTFIX on the basis of "too hard" seems premature without that information.
I don't understand why this wouldn't be fixed by bug 752516.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> For who? Do we have any idea how common this is? WONTFIX on the basis of
> "too hard" seems premature without that information.

My understanding from bug 752516 was that require more invasive approaches than e.g. style fixing of the overlay binding were out of scope for CtP at this point.
Status: RESOLVED → REOPENED
Priority: P3 → P5
Resolution: WONTFIX → ---
Attached file test case
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I don't understand why this wouldn't be fixed by bug 752516.

I added a simplified test-case. From my understanding from a discussion with (i think) Jaws we can't fix this via styles and would need more invasive measures.
Anyway CSS hack of C2P causes Bug 867760
QA noticed that a few bugs hanging off bug 752516 were not fixed by it, which have a similar setup as the situation here. Let's use this bug to look into this.

bsmedberg, bug 752516, comment 26:
> 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?

roc, bug 752516, comment 28:
> 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?

This sounds good to me so far, with the remaining problem being that that method will yield different results with and without the binding attached.
Are we ok with still attaching the binding, but also doing the more agressive popups like for hidden plugins?
Priority: P5 → P3
Reminder: CTP is now default in FF 26 Beta and Quicktime videos doesn't work with it enabled.
https://bugzilla.mozilla.org/show_bug.cgi?id=752516#c23
Assignee: nobody → benjamin
Priority: P3 → P2
Comment on attachment 829783 [details] [diff] [review]
If a plugin is overlayed by other content on the page, then treat it as a hidden plugin: hide the overlay and present the infobar for click-to-play or crashed plugins

This isn't quite right yet. We're catching cases that were actually fixed in bug 752516.
Attachment #829783 - Flags: review?(jaws)
This patch has the following user-visible side-effect, which I don't think is bad: if a plugin is too small to show the full overlay, but isn't 0x0, we will now show the grey background of the plugin.

In order to use .elementFromPoint, the mainBox overlay element has to be visible. In order to fix this, I'm now using a CSS class to mark whether the rest of the overlay should be visible or not. This cascaded into some minor changes in both Firefox and Fennec frontend code which was finding class="mainBox". This code now uses anonid="main". This means the risk of uplift is slightly greater than Firefox-only code, but it's mostly mechanical changes and the risk is that I didn't find/fix some code's use of class="mainBox" or .style.visibility = "hidden".
Attachment #829783 - Attachment is obsolete: true
Attachment #830220 - Flags: review?(dkeeler)
Note: this requires additional trivial test fixup in browser_CTP_resize.js and browser_pluginnotification.js (forgot to change an "is" to "ok").

This patch series is also currently breaking test_keycodes.xul on Windows for totally mysterious reasons (it doesn't appear to be related at all): I'm investigating that, but I don't think it should affect reviews here. (https://tbpl.mozilla.org/php/getParsedLog.php?id=30426193&tree=Try)
Comment on attachment 830220 [details] [diff] [review]
If a plugin is overlayed by other content on the page, then treat it as a hidden plugin: hide the overlay and present the infobar for click-to-play or crashed plugins

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

This seems reasonable, but I'm not sure I'm the best person to review these changes.

::: browser/base/content/browser-plugins.js
@@ +91,5 @@
> +   *
> +   * This function will handle showing or hiding the overlay.
> +   * @returns true if the plugin is invisible.
> +   */
> +  validateVisibility : function (plugin, overlay) {

Maybe "validateAndUpdateVisibility"

@@ +420,5 @@
>  
>    hideClickToPlayOverlay: function(aPlugin) {
>      let overlay = this.getPluginUI(aPlugin, "main");
>      if (overlay)
> +      overlay.classList.remove("visible");

Maybe we could add braces around this if body while we're here.

@@ +572,5 @@
>      let overlay = this.getPluginUI(aPlugin, "main");
>  
>      if (pluginPermission == Ci.nsIPermissionManager.DENY_ACTION) {
>        if (overlay)
> +        overlay.classList.remove("visible");

Same with the if braces.

::: browser/base/content/test/general/browser_pluginnotification.js
@@ +888,5 @@
> +}
> +
> +function test28b()
> +{
> +  var doc = gTestBrowser.contentDocument;

This file uses both var and let, but let's use let for all new things.
Attachment #830220 - Flags: review?(dkeeler) → feedback+
Comment on attachment 8345325 [details] [diff] [review]
If a plugin is overlayed by other content on the page, then treat it as a hidden plugin: hide the overlay and present the infobar for click-to-play or crashed plugins

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

::: browser/base/content/browser-plugins.js
@@ +1231,5 @@
>  
>      let isShowing = true;
>  
>      // Is the <object>'s size too small to hold what we want to show?
> +    if (this.validateAndUpdateVisibility(plugin, overlay)) {

The previous function name of 'isTooSmall' read better than 'validateAndUpdateVisibility'. Can we keep the function doing only one thing (SRP)? Keep isTooSmallOrOccluded and then introduce helper functions for show and hide.

This can then just be:
> let shouldShow = this.isTooSmallOrOccluded(plugin, overlay);
> this.updateVisibility(shouldShow);
> if (!shouldShow) {
>   // First try hiding ...

I think this is clearer and easier to follow.

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +64,5 @@
> +  visibility: hidden;
> +}
> +
> +.mainBox.visible > .hoverBox,
> +.mainBox.visible > .closeIcon {

We don't need to chain the .mainBox and .visible selectors here, since .hoverBox will never be a child of a .visible element that is not a .mainBox. This can just be |.visible > .hoverBox| and since this rule is specified lower than the the |.mainBox > .hoverBox| it will have greater precedence.
Attachment #8345325 - Flags: review?(jaws) → review-
Attachment #8345325 - Attachment is obsolete: true
Attachment #8347428 - Flags: review?(jaws)
Comment on attachment 8347428 [details] [diff] [review]
The methods names are now .shouldShowOverlay and .setVisibility

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

Thanks!
Attachment #8347428 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/97a3f38d2959
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
29.0a1 (2013-12-18), win 7 x64
Now the overlay is no longer seen over the plugin content, only the notification bar appears.
Testcases from the dependent bugs:
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
http://trailers.apple.com/trailers/marvel/ironman3/
https://bug853973.bugzilla.mozilla.org/attachment.cgi?id=743015
http://www.sohu.com/

Also, the same 'no overlay' behavior in the case of window resizing - bug 951414 - which looks pretty valid to me.
(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> Now the overlay is no longer seen over the plugin content
Actually, by "no overlay" I meant to say that the "Click to activate" text is no longer visible over the overlay.
Does that mean you're verifying that this worked correctly? That is the intended behavior per comment 15.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> Does that mean you're verifying that this worked correctly? That is the
> intended behavior per comment 15.
Yes, it works fine basically. It's just that

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> if a plugin is too small to show the full overlay, but isn't 0x0, we
> will now show the grey background of the plugin.
http://img811.imageshack.us/img811/6460/cw8o.png - do you think this is too small ?
No, it's not too small, but it's overlayed, which is what this bug was about, right?
Yes, I was just surprised I didn't see the 'activate' text on apple trailers.
Verified fixed using the testcases in comment 23 (without www.sohu.com) on FF 29.0a1(2013-12-19) on Win 7, Mac OS X 10.8, Ubuntu 13.04.
The blocked ads from sohu.com from the bottom of the page still open new tabs on click - bug 809785.
Status: RESOLVED → VERIFIED
Depends on: 968762
Depends on: 976769
Depends on: 980943
You need to log in before you can comment on or make changes to this bug.