972237 - <100% page zoom breaks Click to Play on flash objects in iframes

Status: VERIFIED FIXED

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

Description

[tyaremco]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140127194636

Steps to reproduce:

plugins.click_to_play = true
Flash -> Ask to Activate

1) Go to http://www.reddit.com/r/videos/
2) Set page zoom less than 100% (CTRL + -)
3) Expand any embedded video


Actual results:

A black box is shown.


Expected results:

The Click to Play overlay should be shown.

Comment 1

[tyaremco]

This bug seems to apply whenever a flash object is in an iframe.  I experience the same symptoms with the video on this page
http://arstechnica.com/gadgets/2014/02/googles-project-tango-is-a-smartphone-with-kinect-style-computer-vision/

Comment 2

[sjw]

Works for me using current Nightly. Have you tried in safe mode?

Comment 3

[tyaremco]

Confirmed in Nightly, all add-ons disabled.

plugins.click_to_play = true
Flash -> Ask to Activate

1) Go to http://www.reddit.com/r/videos/
2) Set page zoom less than 100% (CTRL + -)
3) Expand any embedded video


Actual results:

FF28 and below
A black box is shown, no text.
Default (arrow) cursor on mouseover.
Clicking box does nothing.

FF29 and above (FF32)
A grey box is shown, no text.
Default (arrow) cursor on mouseover.
Clicking box prompts to activate plugin.


Expected results:
A grey box with "Activate Adobe Flash" text overlay.  
Pointer (hand) cursor on mouseover.

Comment 4

[sjw]

I followed exactly these steps, but I got a normal gray box with plugin icon, close button, pointer mouse and the text. May be this happens only on Windows?

Comment 5

[tyaremco]

For this example:
http://arstechnica.com/gadgets/2014/02/googles-project-tango-is-a-smartphone-with-kinect-style-computer-vision/

Set the page zoom less than 100%, then refresh the page.

Comment 6

[tyaremco]

(In reply to sjw from comment #4)
> I followed exactly these steps, but I got a normal gray box with plugin
> icon, close button, pointer mouse and the text. May be this happens only on
> Windows?

Seems likely.  I don't have another OS to test...

Comment 7

[Paul Silaghi, QA [:pauly]]

I can't reproduce on 32.0a1 (2014-05-05), Win 7 x64. The Click to Play overlay is properly displayed.

Comment 8

[Georg Fritzsche [:gfritzsche]]

I can reproduce this on e.g. http://arstechnica.com/gadgets/2014/02/googles-project-tango-is-a-smartphone-with-kinect-style-computer-vision/

For some reason this doesn't happen on all zoom levels, i can reproduce by:
* set flash to "ask to activate"
* de- or increase zoom by one step
* reload
* if it's not broken, repeat

... there is probably something going wrong with how we decide whether to show the overlay here:
http://hg.mozilla.org/mozilla-central/annotate/81651ad5e43c/browser/base/content/browser-plugins.js#l98

Comment 9

[Benjamin Smedberg]

This probably isn't a top priority, but would make a great mentored bug.

Comment 10

[Biru [:poiru]]

Attachment: Fix missing plugin overlay on some zoom levels

This is caused by `overlay.scrollWidth > pluginRect.width` evaluating to true on some zoom levels. In such cases, pluginRect.width might be e.g. 639.75 while overlay.scrollWidth is 640.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8455043 [details] [diff] [review]
Fix missing plugin overlay on some zoom levels

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

Thanks for the patch!
I'm not entirely sure why pluginRect.width would behave that way, but this seems sensible in this context.
Trying to find a Firefox peer for review.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Would you want to try to write a test for this as well?

You could start basing this on something like the test browser_CTP_resize.js [0] and instead start out with a normal-sized plugin element and check that the overlay is visible. Then you change the zoom and test that it still is visible.

If you make it an async test [1], zooming should be possible using FullZoomHelper.enlarge() like so:
> // ... test plugin overlay with normal zoom
> yield FullZoomHelper.enlarge();
> // ... test plugin overlay with increased zoom
... and set up the cleanup function to do FullZoomHelper.reset().

Feel free to contact me via IRC, mail or here with any questions.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_CTP_resize.js?from=browser_CTP_resize.js
[1] https://developer.mozilla.org/en/docs/Browser_chrome_tests#Test_functions
[2] http://hg.mozilla.org/mozilla-central/annotate/7f9db2379b3f/browser/base/content/test/general/head.js#l418

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8455043 [details] [diff] [review]
Fix missing plugin overlay on some zoom levels

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

::: browser/base/content/browser-plugins.js
@@ +105,5 @@
>      // Is the <object>'s size too small to hold what we want to show?
>      let pluginRect = plugin.getBoundingClientRect();
>      // XXX bug 446693. The text-shadow on the submitted-report text at
>      //     the bottom causes scrollHeight to be larger than it should be.
> +    let overflows = (overlay.scrollWidth > Math.round(pluginRect.width)) ||

You'll want to use `Math.ceil` here instead of `Math.round`.

Comment 14

[avada]

Will this fix CTP for zoom levels above 100% also. ( I have pages load at 115% with an addon for better readability.) Or does that have a separate cause?

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to avada from comment #14)
> Will this fix CTP for zoom levels above 100% also. ( I have pages load at
> 115% with an addon for better readability.) Or does that have a separate
> cause?

Same issue, this bug should fix that too.

Comment 16

[Biru [:poiru]]

Attachment: Fix missing plugin overlay with some zoom levels and add test

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> Would you want to try to write a test for this as well?

Done. Apologies for the delay!

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> > +    let overflows = (overlay.scrollWidth > Math.round(pluginRect.width)) ||
> 
> You'll want to use `Math.ceil` here instead of `Math.round`.

Fixed.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8463058 [details] [diff] [review]
Fix missing plugin overlay with some zoom levels and add test

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

Thanks for the test! Could you post another patch that simplifies the unit test a bit?

With that, it'll be r=me :)

::: browser/base/content/test/plugins/browser_CTP_zoom.js
@@ +67,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +let enlargeCount = 2;

I'm not sure if you hit the fractional zoom level when enlarging only two times. I'm quite sure that you can increase this number.

Also, did you run this test
1) with you patch applied and
2) WITHOUT your patch applied to see if it failed?

@@ +87,5 @@
> +    FullZoom.reset();
> +    clearAllPluginPermissions();
> +    finishTest();
> +  }
> +}

Since there's only one test, could you simplify this testcase by reducing the amount of indirection here?

I mean, just have one function that is executed each time after pageLoad --> plugin bindings attached.

Comment 18

[Biru [:poiru]]

Attachment: Fix missing plugin overlay with some zoom levels and add test

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Comment on attachment 8463058 [details] [diff] [review]
> Fix missing plugin overlay with some zoom levels and add test
> 
> Review of attachment 8463058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, did you run this test
> 1) with you patch applied and
> 2) WITHOUT your patch applied to see if it failed?

Yep.

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Birunthan Mohanathas [:poiru] from comment #18)
> Yep.

Alright, cool :)

I also requested one other (relatively small) thing:

Since there's only one test, could you simplify this testcase by reducing the amount of indirection here?
I mean, just have one function that is executed each time after pageLoad --> plugin bindings attached.

A mochitest-only try run on a single, low-load server for this patch would be appreciated. Do you have L1 access? I can do it too if you like.

Comment 20

[Biru [:poiru]]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> I also requested one other (relatively small) thing:
> 
> Since there's only one test, could you simplify this testcase by reducing
> the amount of indirection here?
> I mean, just have one function that is executed each time after pageLoad -->
> plugin bindings attached.

Already did that in the last patch :) Sorry, should have mentioned that in the comment.

> A mochitest-only try run on a single, low-load server for this patch would
> be appreciated. Do you have L1 access? I can do it too if you like.

Yeah, will do a try run before I check it in.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8465583 [details] [diff] [review]
Fix missing plugin overlay with some zoom levels and add test

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

\o/ Excellent!

::: browser/base/content/test/plugins/plugin_zoom.html
@@ +4,5 @@
> +<meta charset="utf-8">
> +</head>
> +<body>
> +<!-- The odd width and height are here to trigger bug 972237. -->
> +<embed id='test' style='width: 99.789%; height: 99.123%' type='application/x-test'>

nit: please use double quotes.

Comment 22

[Biru [:poiru]]

Checked in patch with comment 21 addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e76d723927e1

Try push: https://tbpl.mozilla.org/?tree=Try&rev=12c029f7de77

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e76d723927e1

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e76d723927e1

Comment 25

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on Firefox 27 using steps from comment 0 and 1, verified that the issue is fixed on Windows 7 64bit, Ubuntu 12.04 32bit and Mac OS X 10.9.4 using latest Nightly 34.0a1.