Closed Bug 1183164 Opened 7 years ago Closed 7 years ago

Click-to-play overlay does not work if it starts in a non-visible state

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: rowbot, Assigned: rowbot)

Details

Attachments

(2 files, 1 obsolete file)

This is a regression from Bug 980943.  What seems to be happening in this case at least is that we have elements overlaying the plugin content and shouldShowOverlay[1] is returning false, which is putting the overlay in a non-visible state.

STR:
1) Visit http://www.nick.com/games/super-brawl-3.html

Actual Results:
Clicking on the Click-to-play overlay does nothing.

Expected Results:
Clicking on the Click-to-play overlay should show the click-to-play notification prompting the user to Allow the plugin to run.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#212
:mconley and I discussed this on IRC and thought that adding a new dismissed state to handle the case when the user intentionally dismisses the plugin overlay would work here since it is impossible to distinguish any specific state from the current method of changing the visible state of the plugin overlay.  This fixes the regression noted in this bug.
Attachment #8632928 - Flags: feedback?(mconley)
:bsmedberg and :gfritzsche, :mconely thought it would be a good idea to bring you in on this.  The patch I wrote in Bug 980943 uncovered this situation, but the underlying cause of the bug is the shouldShowOverlay[1] method returning false[2] because of content that overlaps the plugin.  Currently, that method functions as intended in that if any content overlaps the plugin, we hide the visibility of the plugin overlay.  The problem is that it does this regardless of the size of the content that is overlapping the plugin.  So, in the case of the URL in comment 0, we have a plugin that covers the entire screen and there are a few elements around the edges of the screen that overlap the plugin.  This causes us to hide the visibility of the plugin overlay despite a large area of the plugin can be clicked on and interacted with.

What I proposed to :mconely was to do away with most of the state management revolving around the plugin overlay's visibility.  Currently, in theory, a 1px x 1px element overlapping the plugin will cause us to hide the visibility of the plugin overlay, which, is really conservative.  The removal of the visible state also disables the cursor: pointer; style on the overlay which helps to convey to the user that this is something they can click on, so this is bad from a usability/discoverability perspective.  We could hide the visibility of the overlay only if the entire plugin is completely occluded by another element, but I'm not sure there is much point to actually hiding the overlay in this case since you would not be able to interact with the plugin overlay in this case.  The only reason I can think of to hide the plugin overlay when the plugin is completely occluded would be to handle the case where someone uses a transparent element so that the "Activate <Insert plugin name here>" text can be seen behind it to try and get the user to click on it for tracking them or whatever the case may be.  Of course, a user may still try to click there anyways if they see the blank grey plugin overlay and and have associated a blank grey box with click-to-play plugins.

So, I guess the question is, do we still want to hide the visibility of the plugin overlay when anything of any size overlaps it, even though it is unlikely that it would prevent the user from actually clicking on the overlay to activate the plugin?

I think I covered everything.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#212
[2] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#254
Flags: needinfo?(gfritzsche)
Flags: needinfo?(benjamin)
While the patch in comment #1 passes all tests locally, the test I wrote in Bug 980943 should probably be modified to also check that the dismissed class exists, should we decide to go the route of adding a new state.
Here are the goals:

* If the user can't reliably click on the plugin, we shouldn't show the user UI to click on the plugin
* If the user can't click on the plugin, we need to trigger the notification bar instead

Within those constraints you have some room to work. If for example only the corner is covered, it is theoretically possible to shrink the size of the UI and still have it fully functional. In practice, that's pretty difficult, so we haven't done that extra logic.
Flags: needinfo?(gfritzsche)
Flags: needinfo?(benjamin)
> Here are the goals:
> 
> * If the user can't reliably click on the plugin, we shouldn't show the user UI to click on the plugin
> * If the user can't click on the plugin, we need to trigger the notification bar instead
The whole visibility thing of the overlay is a little bit confusing here.  Currently, when we "hide" the visibility of the overlay, all we are doing is visibly hiding the plugin icon and the text that says "Activate <Insert plugin name here>".  The overlay itself is still present, still visible, and can still be clicked on to trigger the plugin notification.  So, technically, no functionality would be changed.  My proposal was more about just reducing code complexity, which boils down to always show the icon and text unless the dimensions of the plugin are too small to contain it.

From what I have seen out in the wild there are 2 things that happen.
1) Some element completely covers the plugin.  There is no way around this and the user will never be able to click on the overlay.  In most circumstances, the user won't be able to see the overlay either as the site is probably using some sort of poster image as a preview.

2) There are a number of smaller elements that are positioned over the plugin, such as a loading spinner, some links, buttons, etc., but more than 90% of the plugin's area does not have any overlapping content.

I don't think I've really seen anything that falls in between these two ends of the spectrum.

I completely understand where you are coming from in that you don't want to tell the user, "Hey! Click on me!" if their is a chance that their mouse click won't actually get through to the plugin overlay and perform whatever action is expected.  You could also argue that these same elements that are making it unreliable for the user to click on the plugin overlay will likely remain there after activating the plugin and make interacting with the plugin content unreliable, and it seems likely that Firefox would get blamed either way because they clicked on one of the overlapping elements, but expected their click to be handled by the plugin.  It also seems bad to present them with a large gray box that doesn't say anything about what it is or what it does, changes color when you hover over it, is right where they expect content to be, and does... stuff when they click on it.

> If for example only the corner is covered, it is theoretically possible to shrink the size of the UI and still have it
> fully functional. In practice, that's pretty difficult, so we haven't done that extra logic.
I would have to agree that it isn't worth the added complexity of trying to put together what essentially becomes a jigsaw puzzle of the overlapping content and then trying to figure out just how much of the plugin's content area is not overlapped by other content to determine whether or not the UI should be hidden or shown.
It is very common for the plugin to be covered by a transparent DOM element. This is what happens for instance on the apple video trailers site. It also happens on several other major websites.

It is essential that we do not show the activation text if the user cannot actually click it.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> It is very common for the plugin to be covered by a transparent DOM element.
> This is what happens for instance on the apple video trailers site. It also
> happens on several other major websites.
I had made mention of a similar type of scenario involving a transparent element in comment 2, but forgot to touch on it again in comment 5.  Sorry for the confusion.

> It is essential that we do not show the activation text if the user cannot
> actually click it.
Understood. I'll drop the subject. :)
Comment on attachment 8632928 [details] [diff] [review]
bug1183164_add_dismissed_state.diff

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

This looks like it should do the job.

::: browser/modules/PluginContent.jsm
@@ +199,5 @@
>     */
>    setVisibility : function (plugin, overlay, shouldShow) {
>      overlay.classList.toggle("visible", shouldShow);
> +    if (shouldShow) {
> +      overlay.classList.remove("dismissed");

I would use an attribute, personally, rather than a class in order to set state on this thing.

So I'd have this be:

overlay.removeAttribute("dismissed");

@@ +459,2 @@
>            this.hideClickToPlayOverlay(plugin);
> +          overlay.classList.add("dismissed");

And overlay.setAttribute("dismissed", "true");

@@ +631,5 @@
>      let overlay = this.getPluginUI(plugin, "main");
>      // Have to check that the target is not the link to update the plugin
>      if (!(event.originalTarget instanceof contentWindow.HTMLAnchorElement) &&
>          (event.originalTarget.getAttribute('anonid') != 'closeIcon') &&
> +        !overlay.classList.contains('dismissed') &&

and finally,

!overlay.hasAttribute("dismissed")
Attachment #8632928 - Flags: feedback?(mconley) → feedback+
This uses an attribute to keep track of the state rather than a class as suggested.
Attachment #8632928 - Attachment is obsolete: true
Attachment #8633583 - Flags: review?(mconley)
This just modifies the test I wrote in Bug 980943 to also check for the dismissed attribute.

I also removed the check for plugin in the return statement since, during our discussion for Bug 980943, we had determined that the plugin check didn't guard against anything since the entire content task is predicated on plugin existing and would have thrown an error long before hitting the return statement.
Attachment #8633588 - Flags: review?(mconley)
Attachment #8633588 - Flags: review?(mconley) → review+
Comment on attachment 8633583 [details] [diff] [review]
bug1183164_add_dismissed_state v2

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

Looks good. Thanks Trevor!
Attachment #8633583 - Flags: review?(mconley) → review+
Awesome! Thanks for the reviews.  Could you assign this bug to me?  And looks like you still need to flag this as checkin-needed.
Assignee: nobody → smokey101stair
I am able to assign myself now.  Looks like you still need to add checkin-needed though, unless I am blind and just can't find the place to add it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I was blind, silly experimental interface.  I got it now!  Sorry for the comment spam.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc1975920303
https://hg.mozilla.org/mozilla-central/rev/9435a7c512ec
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.