1405655 - http://www.speedtest.net does not show "Activate flash" message on their landing page

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Romain Testard [:RT]]

http://www.speedtest.net works on Mac buit NOT on Windows (I tested on Windows 10).
The flash area is greyed-out but there is no message displayed inviting the user to enable Flash. Clicking the Flash area although displays the door hanger and everything works as expected if I enable flash.

Comment 1

[Romain Testard [:RT]]

Attachment: screenshot

Comment 2

[Jim Mathies [:jimm]]

I think Ben use to maintain the ctp stuff, not sure. Romain, do you know who owns CTP work?

Comment 3

[Romain Testard [:RT]]

I'm not sure who can help with client-side engineering. Chris do you know?

Comment 4

[Chris Peterson [:cpeterson]]

Romain, does the problem go away if you disable Flash async drawing with the "dom.ipc.plugins.asyncdrawing.enabled" pref? Are you using 32-bit or 64-bit Firefox?

I am not able to reproduce this bug with 64-bit Firefox 56 or 32-bit Firefox Beta 57 on Windows 10.

jimm's team is responsible for Flash plugin bugs. CTP UI work would be Felipe.

Comment 5

[Romain Testard [:RT]]

(In reply to Chris Peterson [:cpeterson] from comment #4)
> Romain, does the problem go away if you disable Flash async drawing with the
> "dom.ipc.plugins.asyncdrawing.enabled" pref? Are you using 32-bit or 64-bit
> Firefox?
> 
> I am not able to reproduce this bug with 64-bit Firefox 56 or 32-bit Firefox
> Beta 57 on Windows 10.
> 
> jimm's team is responsible for Flash plugin bugs. CTP UI work would be
> Felipe.

Really strange I cannot reproduce it today in a normal browsing Window (I use 64 bit Firefox 56.0 on WIndows 10) but I can using a private browsing WIndow.
The problem persists on private browsing if I disable "dom.ipc.plugins.asyncdrawing.enabled".
Felipe, does that sound familiar? ANything I can do to help debug?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Just to confirm your report: the gray area appears correctly, but the icons and text doesn't, right? And the click-to-play icon appears in the URL bar too?

There's a size threshold where we decide whether the ctp icon should be displayed or not (but the gray area remains working nonetheless). It's planned to remove this threshold and always display the icon, and I thought this change had already happened, but I remember it was postponed post-57.

(There was another problem in which this icons didn't appear with Stylo, but that got fixed so I'm assuming it's not some other corner case of this problem. See bug 1381851)

As to why this is happening, I'm not sure. Does it happen intermittently? I can't reproduce it (private browsing window, OSX 10.10.5).
My guess is that the website is doing some resizing of the element, and it depends on timing. The tracking protection on Private Browsing might affect the timing of how/which scripts are loaded..

So, to debug could you test the following things:
 - open the website with the Web Console open and see if any errors are logged there
 - disable Tracking Protection from Private browsing and test it again
 - test it with Stylo disabled

Comment 7

[Romain Testard [:RT]]

(In reply to :Felipe Gomes (needinfo me!) from comment #6)
> Just to confirm your report: the gray area appears correctly, but the icons
> and text doesn't, right? And the click-to-play icon appears in the URL bar
> too?
That's right.
I can now see that the icon and text briefly appear before the pop-up appears(see attached)
> 
> There's a size threshold where we decide whether the ctp icon should be
> displayed or not (but the gray area remains working nonetheless). It's
> planned to remove this threshold and always display the icon, and I thought
> this change had already happened, but I remember it was postponed post-57.
> 
> (There was another problem in which this icons didn't appear with Stylo, but
> that got fixed so I'm assuming it's not some other corner case of this
> problem. See bug 1381851)
> 
> As to why this is happening, I'm not sure. Does it happen intermittently? I
> can't reproduce it (private browsing window, OSX 10.10.5).
I could never experience it on Mac
> My guess is that the website is doing some resizing of the element, and it
> depends on timing. The tracking protection on Private Browsing might affect
> the timing of how/which scripts are loaded..
It happens roughly 60% of the time on Win10, it may indeed be a timing thing
> 
> So, to debug could you test the following things:
>  - open the website with the Web Console open and see if any errors are
> logged there
Here is the log (nothing looks related to the flash element): https://pastebin.com/Rt3K7cEZ
>  - disable Tracking Protection from Private browsing and test it again
I cannot reproduce without tracking protection
>  - test it with Stylo disabled
How can I do that?

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Romain Testard [:RT] from comment #7)
> >  - test it with Stylo disabled
> How can I do that?

Set pref "layout.css.servo.enabled" to false and open the website in a new tab.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

FWIW, I found the code and I think I figured it out. It's not about the size, it's probably about this popup overlay that briefly shows up (can you catch it in a screenshot or a screen recording?)

There's code [1] that makes us not show the icon/text if we think there's something on top of the plugin <object>. That's because some websites put an entire transparent div on top of them, just to avoid users clicking or right-clicking on it.  If we did show the plugin UI, users would try to click and it would not work (because the click would go to the div), and they would think the browser is broken.

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/browser/modules/PluginContent.jsm#319

Comment 10

[Romain Testard [:RT]]

(In reply to :Felipe Gomes (needinfo me!) from comment #9)
> FWIW, I found the code and I think I figured it out. It's not about the
> size, it's probably about this popup overlay that briefly shows up (can you
> catch it in a screenshot or a screen recording?)
> 
> There's code [1] that makes us not show the icon/text if we think there's
> something on top of the plugin <object>. That's because some websites put an
> entire transparent div on top of them, just to avoid users clicking or
> right-clicking on it.  If we did show the plugin UI, users would try to
> click and it would not work (because the click would go to the div), and
> they would think the browser is broken.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/browser/modules/PluginContent.
> jsm#319

OK, that sounds good.
Here is the screen capture for reference: http://recordit.co/pocPod5U6y (the text apperas when I click the grey area)
Is there a bug where we should always display the icon/text when the user closes the transparent div?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

So that screen capture confirms the theory that it's due to the element being positioned on top of that, and not about the size of the element.

The code that we have checks for something on top of the corners of the element, and if there's anything on _any_ corner, we decide not to show the "Activate flash" message.

I think we could ease this restriction and only do this if all four corners are covered, confirming the intention of the website, while preventing accidental things like this one from happening.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

As opposed to right now where we hide if any of the corners or the center of the plugin is covered.

Review commit: https://reviewboard.mozilla.org/r/188508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188508/

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Note: there must be some tests about this (/crosses fingers), but I couldn't find them looking for relevant file names, so I sent this patch to tryserver to see what breaks and then I'll adjust the tests accordingly

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

https://reviewboard.mozilla.org/r/188508/#review193812

Looks good, but I'd like to see the below answered / addressed. Also I'm a bit surprised that no tests are failing for this. How much effort do you think it would be to add one?

::: browser/modules/PluginContent.jsm:334
(Diff revision 1)
>                     [right, top],
>                     [right, bottom],
>                     [centerX, centerY]];
>  
>      if (right <= 0 || top <= 0) {
>        return false;

Does this need to be adjusted too? Seems like it's not quite correct if we want to change the logic to allow partially hidden overlays. I'm curious why it is here to begin with - does elementFromPoint throw if x or y are negative?

Comment 15

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This is a small fix to show the Flash click-to-play UI in more cases instead of blank Flash content.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57. I hope we get a fix ready for uplift soon.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

I too was surprised that there were no tests for this. There were some that tests more limited cases (like being out of view), but not to test this part about being covered by other elements. So I went digging in the history of this code. Here are the findings:

- the normal elementFromPoint indeed doesn't work with negative values (by spec). However, there's a privileged version of it that does, and this code was fixed to use this version on bug 968762.

- The real reason there's the `right < 0 || top < 0` condition is to cover plugins that are fully outside of view, and this was done by bug 932832. This was very important in the past because deciding not to show the overlay also meant deciding to show the infobar. However, this is less critical now that we don't ever show the infobar anymore.

In any case, I made a change that fits the intent of this change here, while preserving the behavior of bug 932832. I removed the `right < 0 || top < 0` part, but in the points loop, any point that is outside of view is skipped.  This means that `if all points` are outside of view, we'll still consider it a hidden plugin. And plugins that are partially in view will follow the rule of checking if any of the corners are obstructed by any other element on top or not.

Writing a test for it.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188508/diff/1-2/

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188508/diff/2-3/

Comment 20

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

https://reviewboard.mozilla.org/r/188508/#review195046

::: browser/base/content/test/plugins/plugin_shouldShowOverlay.html:29
(Diff revision 3)
> +<body>
> +
> +  <div id="testcase1" class="testcase" shouldshow="true"
> +       style="top: -100px">
> +    <!-- Should show overlay even though the top part is outside
> +    	 of the page. -->

Nit: looks like some tab characters snuck in here and below.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188508/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6aaaf9cb7d3
Only hide Flash Activation overlay UI if the entire plugin is covered. r=dthayer

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Approval Request Comment
[Feature/Bug causing the regression]: no regression, but made more prominent by making flash click-to-play by default
[User impact if declined]: this relaxes the rules a bit for when we decide to hide the flash plugin overlay, which causes confusion for users who might see a plain gray box instead of the "Click to Activate Flash" overlay.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This just makes the plugin overlay show in more situations
[String changes made/needed]: none

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Must fix, Beta57+

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Romain, could you please verify this fix works as expected on Nightly? Thanks!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b95894a2554f

Comment 28

[Romain Testard [:RT]]

(In reply to Ritu Kothari (:ritu) from comment #26)
> Hi Romain, could you please verify this fix works as expected on Nightly?
> Thanks!

Using Oct 18th Nightly (most current) I could reproduce the issue. Has the fix landed on Nightly already?

Comment 29

[:Felipe Gomes (needinfo for replies!)]

I think it's only gonna show up o Oct 19th

Comment 30

[Andrei Vaida [:avaida]]

(In reply to :Felipe Gomes (needinfo me!) from comment #24)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: not yet
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Felipe's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 31

[Romain Testard [:RT]]

FYI I can still reproduce today (Oct 19th Nightly)