Closed Bug 1405655 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: RT, Assigned: Felipe)

References

Details

User Story

Website: http://www.speedtest.net 
Environment: Win10 with Firefox 56.0 release
Issue: Flash area does not display message to enable flash.

Attachments

(2 files)

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.
Attached image screenshot
User Story: (updated)
I think Ben use to maintain the ctp stuff, not sure. Romain, do you know who owns CTP work?
Flags: needinfo?(rtestard)
I'm not sure who can help with client-side engineering. Chris do you know?
Flags: needinfo?(rtestard) → needinfo?(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.
Flags: needinfo?(cpeterson) → needinfo?(rtestard)
(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?
Flags: needinfo?(rtestard) → needinfo?(felipc)
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
Flags: needinfo?(felipc)
(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?
(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.
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
(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?
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.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Blocks: 1402487
Attachment #8917541 - Flags: review?(dothayer)
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 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?
Attachment #8917541 - Flags: review?(dothayer) → review-
[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.
OS: Windows 10 → Unspecified
Tracked for 57. I hope we get a fix ready for uplift soon.
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 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.
Attachment #8917541 - Flags: review?(dothayer) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a6aaaf9cb7d3
Only hide Flash Activation overlay UI if the entire plugin is covered. r=dthayer
https://hg.mozilla.org/mozilla-central/rev/a6aaaf9cb7d3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8917541 - Flags: approval-mozilla-beta?
Comment on attachment 8917541 [details]
Bug 1405655 - Only hide Flash Activation overlay UI if the entire plugin is covered.

Must fix, Beta57+
Attachment #8917541 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Romain, could you please verify this fix works as expected on Nightly? Thanks!
Flags: needinfo?(rtestard)
(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?
Flags: needinfo?(rtestard)
I think it's only gonna show up o Oct 19th
(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.
Flags: qe-verify-
FYI I can still reproduce today (Oct 19th Nightly)
No longer blocks: 1402487
Duplicate of this bug: 1402487
You need to log in before you can comment on or make changes to this bug.