John Schoenick [:johns] Reporter
	Description • 12 years ago

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

	John Schoenick [:johns] Reporter
	Comment 1 • 12 years ago

Testcase - this should show a fallback frame with CTP

	Benjamin Smedberg
	Comment 2 • 11 years ago

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?

	taxilian
	Comment 3 • 11 years ago

Should be able to just sign up for a free account on insight.gradecam.com to repro the issue.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 4 • 11 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 5 • 11 years ago

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?

	John Schoenick [:johns] Reporter
	Comment 6 • 11 years ago

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.

	Benjamin Smedberg
	Comment 7 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 11 years ago

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.

	John Schoenick [:johns] Reporter
	Comment 9 • 11 years ago

(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

	John Schoenick [:johns] Reporter
	Comment 10 • 11 years ago

I think overflow events may be what we want here, and will let us run arbitrary script when needed:

http://jsfiddle.net/JKEhK/

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

What John said.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 12 • 11 years ago

Great, thanks John - that makes this much easier than i thought it would be.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 11 years ago

Improved test-case.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 14 • 11 years ago

This fixes the click-to-play overlay visibility via overflow/underflow event handlers and adds a test for this.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 15 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 16 • 11 years ago

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=cabf766f1703

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 17 • 11 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 18 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> * http://jsfiddle.net/LbbJw/5/

Actually: http://jsfiddle.net/LbbJw/11/

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 11 years ago

I don't know, offhand.  :(

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 11 years ago

(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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 21 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 22 • 11 years ago

Comment on attachment 806260 [details] [diff] [review]
Bonus: fixup browser tests nsIPluginTag.enabledState usage

Try run says i missed something in this patch.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 23 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 24 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 25 • 11 years ago

Updated to the recent test restructuring.

	Benjamin Smedberg
	Comment 26 • 11 years ago

Nominating for tracking since this might affect the decision to turn CtP on by default in FF26.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 27 • 11 years ago

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."

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 28 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8677a18553e7

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 11 years ago

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/46138d38c249 for a m-oth failure.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 30 • 11 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 31 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/65f6618c1e64
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d2f7581e27

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 32 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/65f6618c1e64
https://hg.mozilla.org/mozilla-central/rev/a5d2f7581e27

	neil@parkwaycc.co.uk
	Comment 33 • 11 years ago

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.)

	neil@parkwaycc.co.uk
	Comment 34 • 11 years ago

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...

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 35 • 11 years ago

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.

	Paul Silaghi, QA [:pauly]
	Comment 36 • 11 years ago

Verified fixed using the testcase on 27.0a1 (2013-09-25) Win 7, Ubuntu 13.04, Mac OS X 10.8.4

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 37 • 11 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 38 • 11 years ago

Comment on attachment 809222 [details] [diff] [review]
Fix test_takeFocus

[Approval Request Comment]

This is just test-fixup for the above patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/618d9395bfbd
https://hg.mozilla.org/releases/mozilla-aurora/rev/f081b1e1b442

	Paul Silaghi, QA [:pauly]
	Comment 40 • 11 years ago

(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)