Closed
Bug 853973
Opened 11 years ago
Closed 11 years ago
Click-to-play doesn't work if overlayed by an element in a separate sub-tree
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla29
People
(Reporter: alex_mayorga, Assigned: benjamin)
References
()
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Comment 3•11 years ago
|
||
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 → ---
Updated•11 years ago
|
Priority: -- → P3
Comment 4•11 years ago
|
||
(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)
Updated•11 years ago
|
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
Comment 5•11 years ago
|
||
(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.
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → WONTFIX
Comment 6•11 years ago
|
||
For who? Do we have any idea how common this is? WONTFIX on the basis of "too hard" seems premature without that information.
Assignee | ||
Comment 7•11 years ago
|
||
I don't understand why this wouldn't be fixed by bug 752516.
Comment 8•11 years ago
|
||
(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 → ---
Comment 9•11 years ago
|
||
(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.
Comment 10•11 years ago
|
||
Anyway CSS hack of C2P causes Bug 867760
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
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 | ||
Comment 13•11 years ago
|
||
Attachment #829783 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → benjamin
Priority: P3 → P2
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #830220 -
Attachment is obsolete: true
Attachment #8345325 -
Flags: review?(jaws)
Comment 19•11 years ago
|
||
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-
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8345325 -
Attachment is obsolete: true
Attachment #8347428 -
Flags: review?(jaws)
Comment 21•11 years ago
|
||
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+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/97a3f38d2959
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 23•11 years ago
|
||
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.
Comment 24•11 years ago
|
||
(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.
Assignee | ||
Comment 25•11 years ago
|
||
Does that mean you're verifying that this worked correctly? That is the intended behavior per comment 15.
Comment 26•11 years ago
|
||
(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 ?
Assignee | ||
Comment 27•11 years ago
|
||
No, it's not too small, but it's overlayed, which is what this bug was about, right?
Comment 28•11 years ago
|
||
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
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•