Closed Bug 790483 Opened 7 years ago Closed 6 years ago

click-to-play plugins fail to show placeholder after resizing

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 --- verified

People

(Reporter: johns, Assigned: gfritzsche)

References

()

Details

(Whiteboard: [CtPDefault:P2])

Attachments

(3 files, 4 obsolete files)

When a plugin tag is created as click-to-play, we'll hide the placeholder UI if the frame is too small, but fail to show it when the frame is later resized. This breaks sites like:

http://news.yahoo.com/blogs/trending-now/long-lost-renoir-masterpiece-found-among-junk-flea-170238665.html
Attached file Simple test case (obsolete) —
Testcase - this should show a fallback frame with CTP
Whiteboard: [CtPDefault:P3]
Priority: -- → P3
This bug as filed is different from the issue being discussed on firefox-dev: in that case, the plugin shows up as completely black. In this case it shows up with the "small plugin UI".

I don't think it's critical to fix the small plugin UI resize to large UI issue.

I do think it's critical to fix the black rectangle issue.

gfritzsche can you investigate the differences (perhaps get a testcase from Richard) and fix at least the first part?
Assignee: nobody → georg.fritzsche
Priority: P3 → P2
Whiteboard: [CtPDefault:P3] → [CtPDefault:P2]
Should be able to just sign up for a free account on insight.gradecam.com to repro the issue.
Attached file Test case 2 (obsolete) —
Variation of the previous test-case with the plugin going from 1x1 px to a size big enough to hold the binding.
This is what GradeCam uses too (and what i suspected), the black background in [1] just being the background color.

The original test-case currently works, but probably just because it resizes before "PluginBindingAttached".

[1] http://cl.ly/image/0h343L0t221D
The best thing i can think of right now is
(1) firing a "resized" event when the plugins size didn't change for N ms or
(2) firing a "resized" event every M ms

Both would avoid continuously firing the event while an element gets animated etc., with (1) probably being sufficient.

johns, do you have a suggestion where we could hook into to track size changes?
Flags: needinfo?(jschoenick)
Can't we make use of CSS selectors here to just always render the overlay in a way the gracefully degrades (and shows nothing below a certain size)? It seems hacky to be using javascript to rewrite things based on their size.

If we absolutely need a resize hook, I'm guessing nsObjectFrame would need to fire a callback that fires an event, or something, but I'm not to familiar with the layout code -- there may be a cleaner way.
Flags: needinfo?(jschoenick)
You can do CSS selectors based on the viewport size, but I don't know of any CSS selectors that let you select based on the actual size of an element.
bz, do you have a suggestion on what the best option is for this?

We need to have the click-to-play binding UI for click-to-play elements hidden if they are too small to hold it. When the element gets resized to a sufficient size however, we need to show it.
Flags: needinfo?(bzbarsky)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> You can do CSS selectors based on the viewport size, but I don't know of any
> CSS selectors that let you select based on the actual size of an element.

Right, but you can use CSS to have something degrade gracefully when it is small, e.g. pushing all content into overflow when it is below a certain size.

We could also maybe use overflow events to detect when the placeholder no longer fits:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/overflow
I think overflow events may be what we want here, and will let us run arbitrary script when needed:

http://jsfiddle.net/JKEhK/
What John said.
Flags: needinfo?(bzbarsky)
Great, thanks John - that makes this much easier than i thought it would be.
Attached file Test-case 3
Improved test-case.
Attachment #800295 - Attachment is obsolete: true
This fixes the click-to-play overlay visibility via overflow/underflow event handlers and adds a test for this.
Attachment #806256 - Flags: review?(jaws)
Bonus patch: Fixup the browser tests, similar to the fixups in bug 899080, utilizing the utility function added by the previous patch.

This fixes some stale/non-default enabledState and removes the need to explicitly reset it.
Attachment #806260 - Flags: review?(jaws)
I accidentally stumbled over the fact that the "underflow" event fires even if only one dimension actually underflows.
Boris, do you know if this is intentional?

STR:
* http://jsfiddle.net/LbbJw/5/
* click "Shrink" -> overflow fired, #inner hidden
* click one of "Grow width" or "Grow height" -> underflow fired, #inner shown but actually still overflowing in the other dimension
Flags: needinfo?(bzbarsky)
(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> * http://jsfiddle.net/LbbJw/5/

Actually: http://jsfiddle.net/LbbJw/11/
I don't know, offhand.  :(
Flags: needinfo?(bzbarsky)
(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> I accidentally stumbled over the fact that the "underflow" event fires even
> if only one dimension actually underflows.
> Boris, do you know if this is intentional?
> 
> STR:
> * http://jsfiddle.net/LbbJw/5/
> * click "Shrink" -> overflow fired, #inner hidden
> * click one of "Grow width" or "Grow height" -> underflow fired, #inner
> shown but actually still overflowing in the other dimension

Upon getting the overflow or underflow event, you should be able to use scrollTopMax and scrollTopLeft to determine if the content is still overflowed in either direction. See bug 766937 for the implementation.
The patch already covers this using the existing gPluginHandler.isTooSmall() utility function.
The open question is just if i'm working around a bug or intentional behavior.
Comment on attachment 806260 [details] [diff] [review]
Bonus: fixup browser tests nsIPluginTag.enabledState usage

Try run says i missed something in this patch.
Attachment #806260 - Flags: review?(jaws)
Comment on attachment 806260 [details] [diff] [review]
Bonus: fixup browser tests nsIPluginTag.enabledState usage

As it turns out we have a few tests that actually depended on the enabledState not being reset properly. I'll move this to a new bug.
Attachment #806260 - Attachment is obsolete: true
Blocks: 917931
The actual fix alone looks fine on try:
https://tbpl.mozilla.org/?tree=Try&rev=00b467f1006e

Given the current discussion on dev-firefox, the test should probably be renamed to browser_CTP_resize.js.
Updated to the recent test restructuring.
Attachment #806256 - Attachment is obsolete: true
Attachment #806256 - Flags: review?(jaws)
Attachment #808562 - Flags: review?(jaws)
Nominating for tracking since this might affect the decision to turn CtP on by default in FF26.
Comment on attachment 808562 [details] [diff] [review]
Flip overlay visibility per overflow state and events, v2

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

Looks good.

::: toolkit/mozapps/plugins/content/pluginProblemBinding.css
@@ +25,5 @@
>  object:-moz-handler-playpreview,
>  object:-moz-handler-vulnerable-updatable,
>  object:-moz-handler-vulnerable-no-update {
>      display: inline-block;
> +    visibility: hidden;

Please add a comment here to say: "Initialize the overlay with visibility:hidden to prevent flickering if the plugin is too small to show the overlay."
Attachment #808562 - Flags: review?(jaws) → review+
As try runs [1] and [2] show, the timeouts were just a problem with test_takeFocus depending on stale plugin enabledState.

[1] https://tbpl.mozilla.org/?tree=Try&rev=cdcf82a19b3e
[2] https://tbpl.mozilla.org/?tree=Try&rev=1b2672210709
Attachment #809222 - Flags: review?(surkov.alexander)
Attachment #809222 - Flags: review?(surkov.alexander) → review+
Comment on attachment 808562 [details] [diff] [review]
Flip overlay visibility per overflow state and events, v2

>+          overlay.style.visibility = "visible";
...
>@@ -21,12 +21,13 @@ applet:-moz-handler-vulnerable-no-update
> object:-moz-handler-disabled,
> object:-moz-handler-blocked,
> object:-moz-handler-crashed,
> object:-moz-handler-clicktoplay,
> object:-moz-handler-playpreview,
> object:-moz-handler-vulnerable-updatable,
> object:-moz-handler-vulnerable-no-update {
>     display: inline-block;
>+    visibility: hidden;
>     overflow: hidden;
>     opacity: 1 !important;
>     -moz-binding: url('chrome://mozapps/content/plugins/pluginProblem.xml#pluginProblem') !important;
> }
You're making the object itself hidden, and the overlay visible... did you mean to do that? (It seems to work, it just looks odd to code it this way.)
Comment on attachment 808562 [details] [diff] [review]
Flip overlay visibility per overflow state and events, v2

>     // Hide the in-content UI if it's too big. The crashed plugin handler already did this.
This comment is out of date - you're now showing the in-content UI unless it's too big. But the crashed plugin handler doesn't do this...
Thanks for the catch Neil, apparently i only quickly checked the crashed overlay after CTP-activating the plugin ... :(

Will address in a follow-up today.
Depends on: 920927
Attachment #660275 - Attachment is obsolete: true
Verified fixed using the testcase on 27.0a1 (2013-09-25) Win 7, Ubuntu 13.04, Mac OS X 10.8.4
Status: RESOLVED → VERIFIED
Component: General → Plug-ins
Product: Firefox → Core
Target Milestone: Firefox 27 → ---
Target Milestone: --- → mozilla27
Depends on: 921152
Depends on: 923690
Comment on attachment 808562 [details] [diff] [review]
Flip overlay visibility per overflow state and events, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign
User impact if declined: click-to-play overlay not visible in some scenarios.
Testing completed (on m-c, etc.): On m-c, follow-ups also there now.
Risk to taking this patch (and alternatives if risky): Low, worst-case is affecting visibility of plugin click-to-play overlay.
String or IDL/UUID changes made by this patch: None.

Note: this depends on the patches from bug 920927.
Attachment #808562 - Flags: approval-mozilla-aurora?
Comment on attachment 809222 [details] [diff] [review]
Fix test_takeFocus

[Approval Request Comment]

This is just test-fixup for the above patch.
Attachment #809222 - Flags: approval-mozilla-aurora?
Attachment #808562 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #809222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Paul Silaghi, QA [:pauly] from comment #36)
> Verified fixed using the testcase on 27.0a1 (2013-09-25) Win 7, Ubuntu
> 13.04, Mac OS X 10.8.4
Verified fixed 26.0a2 (2013-10-20)
You need to log in before you can comment on or make changes to this bug.