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

Status: VERIFIED FIXED

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

Description

[John Schoenick [:johns]]

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

Comment 1

[John Schoenick [:johns]]

Attachment: Simple test case

Testcase - this should show a fallback frame with CTP

Comment 2

[Benjamin Smedberg]

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?

Comment 3

[taxilian]

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Test case 2

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

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?

Comment 6

[John Schoenick [:johns]]

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.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[John Schoenick [:johns]]

(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

Comment 10

[John Schoenick [:johns]]

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

http://jsfiddle.net/JKEhK/

Comment 11

[Boris Zbarsky [:bzbarsky]]

What John said.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Test-case 3

Improved test-case.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Flip overlay visibility per overflow state and events

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

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Bonus: fixup browser tests nsIPluginTag.enabledState usage

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.

Comment 16

[Georg Fritzsche [:gfritzsche]]

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

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

Comment 18

[Georg Fritzsche [:gfritzsche]]

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

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

I don't know, offhand.  :(

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 21

[Georg Fritzsche [:gfritzsche]]

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 22

[Georg Fritzsche [:gfritzsche]]

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

Try run says i missed something in this patch.

Comment 23

[Georg Fritzsche [:gfritzsche]]

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.

Comment 24

[Georg Fritzsche [:gfritzsche]]

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.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Attachment: Flip overlay visibility per overflow state and events, v2

Updated to the recent test restructuring.

Comment 26

[Benjamin Smedberg]

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

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 28

[Georg Fritzsche [:gfritzsche]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 30

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix test_takeFocus

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

Comment 31

[Georg Fritzsche [:gfritzsche]]

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

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 33

[[e-mail]]

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 34

[[e-mail]]

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

Comment 35

[Georg Fritzsche [:gfritzsche]]

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.

Comment 36

[Paul Silaghi, QA [:pauly]]

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

Comment 37

[Georg Fritzsche [:gfritzsche]]

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.

Comment 38

[Georg Fritzsche [:gfritzsche]]

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

[Approval Request Comment]

This is just test-fixup for the above patch.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Paul Silaghi, QA [:pauly]]

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