Closed Bug 921730 Opened 6 years ago Closed 6 years ago

"This plugin is disabled" placeholder cannot be closed

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox24 --- affected
firefox25 --- affected
firefox26 --- affected
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
firefox30 --- verified
firefox-esr24 --- wontfix

People

(Reporter: elbart, Assigned: reznord)

References

()

Details

(Whiteboard: [mentor=gfritzsche][lang=js][good next bug])

Attachments

(3 files, 13 obsolete files)

60.60 KB, image/png
Details
5.94 KB, patch
gfritzsche
: feedback+
Details | Diff | Splinter Review
45.91 KB, image/jpeg
Details
Firefox 23, 24, Nightly 2013-09-27 on XP, Vista and 7
Adobe Flash 11.8.800.168 set to "Never activate"

---

When trying to close a "This plugin is disabled"-field, the placeholder cannot be closed in at least Windows XP, Vista and 7. Check the URL for a site with a Flash-element.

While this is a minor problem for standalone-elements, it becomes a UX-problem when such an obstacle is hiding the menu of a website.

For example http://wiesenmarkt.at/news.php
Move the cursor over the row with "Home", "Wiesn Infos", "Aktuell" etc.
Because the placeholder cannot be closed, the menu becomes unusable.

Please let 24ESR also get the fix and, please test if the other placeholders, like click-to-play, are affected too.
I see the same issue on Linux with the 27 September Nightly. Using the browser console I see the menu items switch from hidden to visible on mouse-over, but the items are behind the "plugin blocked" message.

The same issue exists for "Never Activate" and "Ask to Activate".
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Plug-ins
Product: Firefox → Core
I can confirm that the "plugin disabled" overlay can't be closed on Fx 24 and Nightly.

However, the click-to-play overlay ("ask to activate") is fine. B.J., with the menu effects this sounds like the sites setup prevents you from clicking it. Does it work on [1] for you?

[1] http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86 → All
With Nightly of 2013-10-01:
"ask to activate": closing with the X works; closing with the link works
"never activate": closing with the X does NOT work; closing with the link works.

---

I think B.J. Herbison means that the mouseover-menu-items are behind the plugin-placeholder for both "never activate" and "ask to activate". IIRC I also had the problem when the placeholder was showing that the Flash-plugin was outdated.

Another example of this is http://www.toolgal.com/ by going over "Products".

Maybe this should be filed as another bug?
Another placeholder I cannot close is "A plugin is needed to display this content.", tested on http://benjamin.smedbergs.us/tests/ctptests/java-solo.html with no Java installed.

But I can close the placeholder with "Activate XYZ. This plugin is vulnerable and should be updated.".
Apparently we currently only attach an event handler to the close-button for click-to-play/blocked/vulnerable plugins [1]; maybe we just need to move that handler into handleEvent() [2].


(In reply to elbart from comment #3)
> Another example of this is http://www.toolgal.com/ by going over "Products".
> 
> Maybe this should be filed as another bug?

No, we force the plugin overlay to be always on top as it was blocked/hidden in other use-cases.
We may emulate the z-index behavior of Flash specifically more closely in later releases though (can't find the bug right now).
[1] http://hg.mozilla.org/mozilla-central/annotate/e3c84e9f2490/browser/base/content/browser-plugins.js#l537
[2] http://hg.mozilla.org/mozilla-central/annotate/e3c84e9f2490/browser/base/content/browser-plugins.js#l215
> Apparently we currently only attach an event handler to the close-button for
> click-to-play/blocked/vulnerable plugins [1]; maybe we just need to move
> that handler into handleEvent() [2].

Yes, this sounds correct. I don't think we need the custom handler at all.
Simple steps to reproduce:
* go to Tools->Addons
* set Flash to disabled
* navigate to [1]
* click on the (x)
=> it should hide the "plugin disabled" overlay, but doesn't
Whiteboard: [mentor=gfritzsche][lang=js][good next bug]
(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> However, the click-to-play overlay ("ask to activate") is fine. B.J., with
> the menu effects this sounds like the sites setup prevents you from clicking
> it. Does it work on [1] for you?
> 
> [1] http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html

elbart was correct. Flash works correctly, but I can't use the menus on the original page without activating Flash.
Made changes as suggested in above comments
Attachment #814356 - Flags: review?(georg.fritzsche)
Comment on attachment 814356 [details] [diff] [review]
Moved event handler to handleEvent

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

Thanks, that looks close! Besides the issues mentioned, did you check it is working for some variations? In particular:
* Does the close button work when the plugin is set to disabled or "ask to activate" in Tools->Addons->Plugins?
* Does it also work for crashed plugins? To test that you can kill the plugin container process (e.g. plugin-container.exe on Windows).

::: browser/base/content/browser-plugins.js
@@ +331,5 @@
>            }
>          });
>        }
>      }
> +    if (overlay) {

Minor nitpick: I think we should have an empty line before the if block.

@@ +336,5 @@
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> +      let closeIcon = this.getPluginUI(aPlugin, "closeIcon");
> +      closeIcon.addEventListener("click", function(aEvent) {
> +        if (aEvent.button == 0 && aEvent.isTrusted)
> +          gPluginHandler.hideClickToPlayOverlay(aPlugin);

I think "aPlugin" needs to be "plugin" when you move the code here.

@@ +343,1 @@
>      // Only show the notification after we've done the isTooSmall check, so

Same here, i think we should have an empty line after the if-block closing brace.
Attachment #814356 - Flags: review?(georg.fritzsche)
Attached patch Made changes (obsolete) — Splinter Review
I made the above mentioned changes. No, I haven't. Will try them out and let you know here.
Attachment #814356 - Attachment is obsolete: true
Attachment #814446 - Flags: review?(georg.fritzsche)
That sounds good. If you need any help getting a build working or your changes in it, you can always ask in #introduction [1] or mail me.

[1] https://developer.mozilla.org/en-US/docs/Introduction
Comment on attachment 814446 [details] [diff] [review]
Made changes

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

Great, let me know when you had a chance to test.

::: browser/base/content/browser-plugins.js
@@ +336,5 @@
> +    if (overlay) {
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> +      let closeIcon = this.getPluginUI(plugin, "closeIcon");
> +      closeIcon.addEventListener("click", function(aEvent) {
> +        if (aEvent.button == 0 && aEvent.isTrusted)

I think you have a similar problem as with "plugin" on the two lines here - "aEvent" should be "event".
Attached patch made change from aEvent to event (obsolete) — Splinter Review
I added a line to initialize the overlay variable.Also, I made the changes from aEvent to event(I think it was a formal parameter in the anonymous function) This works for all the test cases mentioned above.
Attachment #814446 - Attachment is obsolete: true
Attachment #814446 - Flags: review?(georg.fritzsche)
Attachment #818947 - Flags: review?(georg.fritzsche)
Will ESR get the patch?
Comment on attachment 818947 [details] [diff] [review]
made change from aEvent to event

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

Cool, this looks good now, behaves right for me locally too. There are only whitespace nitpicks now, if you check out the "splinter review" for your patch you will probably see better what i mean.
I pushed this to our try server so we can see if this affects any tests:
https://tbpl.mozilla.org/?tree=Try&rev=a8ae29dcde76

After fixing the whitespace, please request review from jaws (Jared Wein), as i'm not a peer for the browser code.

::: browser/base/content/browser-plugins.js
@@ +332,5 @@
>          });
>        }
>      }
>  
> +	let overlay = this.getPluginUI(plugin, "main");

I think this is indented with a tab instead of spaces - we only use spaces for indentation.

@@ +333,5 @@
>        }
>      }
>  
> +	let overlay = this.getPluginUI(plugin, "main");
> +    

There are left-over spaces at the start of this line, please remove them.
Attachment #818947 - Flags: review?(georg.fritzsche) → feedback+
(In reply to elbart from comment #16)
> Will ESR get the patch?

The patch missed the normal train for ESR24, so we'll have to ask release management if they are willing to take this after it was verified in Firefox Nightly (not sure as it's not a security or other high priority fix).
Attached patch Minor changes (obsolete) — Splinter Review
Minor changes as asked. Thanks a lot for your help gfritzsche :)
Attachment #818947 - Attachment is obsolete: true
Attachment #819073 - Flags: review?(jaws)
Comment on attachment 819073 [details] [diff] [review]
Minor changes

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

Thanks for the patch, just a couple things to address.

::: browser/base/content/browser-plugins.js
@@ +332,5 @@
>          });
>        }
>      }
>  
> +    let overlay = this.getPluginUI(plugin, "main");

There is already a variable named |overlay| declared about 10 lines above that can be moved and reused.

@@ +334,5 @@
>      }
>  
> +    let overlay = this.getPluginUI(plugin, "main");
> +    if (overlay) {
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);

Adding event listeners here seems quite indeterminate (they should at least be moved to one of the blocks within the event switch).
Attachment #819073 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] from comment #20)
> @@ +334,5 @@
> >      }
> >  
> > +    let overlay = this.getPluginUI(plugin, "main");
> > +    if (overlay) {
> > +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> 
> Adding event listeners here seems quite indeterminate (they should at least
> be moved to one of the blocks within the event switch).

Right, this triggered the failures in the try run [1] as well. You can test this locally by running e.g. single tests:
./mach mochitest-browser browser/base/content/test/general/browser_CTP_drag_drop.js
./mach mochitest-browser browser/base/content/test/general/browser_pluginnotification.js

... or all the general browser tests (takes a bit though):
./mach mochitest-browser browser/base/content/test/general/

The line adding the _overlayClickListener should probably just stay where the code originally was in PH_handleClickToPlayEvent().

[1] https://tbpl.mozilla.org/?tree=Try&rev=a8ae29dcde76
I assume this won't make its way into 26, right?
(In reply to Elbart from comment #22)
> I assume this won't make its way into 26, right?

No - even when it was finished it is not critical enough to warrant putting it into a Beta.
Hi,

I am interested in working on this bug. Can you please assign it to me?

Thanks in advance.
Sure, done :)
Assignee: nobody → allamsetty.anup
Attached patch bug-921730.patch (obsolete) — Splinter Review
working for almost all the tests.
Attachment #8361111 - Flags: review?(gfritzsche)
Could you expand on which test failed with which error?
Attached image error screenshot
> ... or all the general browser tests (takes a bit though):
> ./mach mochitest-browser browser/base/content/test/general/

When I ran the following test in my browser I got the following error saying that

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_CTP_crashreporting.js | Submission UI visibility should be correct - Got none, expected block
> > ... or all the general browser tests (takes a bit though):
> > ./mach mochitest-browser browser/base/content/test/general/
> 
> When I ran the following test in my browser I got the following error saying
> that
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_CTP_crashreporting.js | Submission UI visibility should be correct -
> Got none, expected block

In addition to this error I am getting the following error also:

 5:31.55 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_CTP_crashreporting.js | Test timed out
 5:31.55 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popupNotification.js | Test timed out
Attachment #819073 - Attachment is obsolete: true
Comment on attachment 8361111 [details] [diff] [review]
bug-921730.patch

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

This looks good so far. Let's fix the suspected timing error in the test that we talked about, then this should be good for a review!

::: browser/base/content/browser-plugins.js
@@ +379,5 @@
>          plugin.addEventListener("overflow", resizeListener);
>          plugin.addEventListener("underflow", resizeListener);
>        }
>      }
> +    

You have some trailing spaces (whitespace) on this and other lines.
If you click the "review" link for your patch you will see those spaces highlighted in red.
Attachment #8361111 - Flags: review?(gfritzsche) → feedback+
Attached patch bug.patch (obsolete) — Splinter Review
removed the trailing white spaces.
Attachment #8361111 - Attachment is obsolete: true
Attachment #8365120 - Flags: review?(georg.fritzsche)
Attached patch bugs.patch (obsolete) — Splinter Review
Changes made
Attachment #8365120 - Attachment is obsolete: true
Attachment #8365120 - Flags: review?(georg.fritzsche)
Attachment #8365514 - Flags: review?(georg.fritzsche)
Attached patch bugs.patch (obsolete) — Splinter Review
white spaces fixed
Attachment #8365514 - Attachment is obsolete: true
Attachment #8365514 - Flags: review?(georg.fritzsche)
Attachment #8365522 - Flags: review?(georg.fritzsche)
Attached patch bugs.patch (obsolete) — Splinter Review
White space fixed
Attachment #8365524 - Flags: review?(georg.fritzsche)
Attached patch new.patch (obsolete) — Splinter Review
white spaces removed
Attachment #8365522 - Attachment is obsolete: true
Attachment #8365524 - Attachment is obsolete: true
Attachment #8365522 - Flags: review?(georg.fritzsche)
Attachment #8365524 - Flags: review?(georg.fritzsche)
Attachment #8365525 - Flags: review?(georg.fritzsche)
Comment on attachment 8365525 [details] [diff] [review]
new.patch

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

Looks good and works fine for me now!
One small issue left that i missed before.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ee75433d00eb

::: browser/base/content/browser-plugins.js
@@ +381,5 @@
>        }
>      }
>  
> +    let closeIcon = this.getPluginUI(plugin, "closeIcon");
> +    if (closeIcon != null) {

This should be: if (closeIcon)
Attachment #8365525 - Flags: review?(jaws)
Attachment #8365525 - Flags: review?(georg.fritzsche)
Attachment #8365525 - Flags: feedback+
The above try run was from a broken repository, pushed again: 
https://tbpl.mozilla.org/?tree=Try&rev=8662f4d53053
Comment on attachment 8365525 [details] [diff] [review]
new.patch

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

Anup, can you add a test for this to make sure that it doesn't break again? Also please make the change that Georg requested.
Attachment #8365525 - Flags: review?(jaws) → review+
> Anup, can you add a test for this to make sure that it doesn't break again?
> Also please make the change that Georg requested.

I tested it before I have submitted the patch and also made the changes made which all are required i.e removed the white spaces and the trailing spaces.
Talked with Anup about what we need tests for, we're looking into it.
Attached patch bug-921730.patch (obsolete) — Splinter Review
Included the test for that also.
Attachment #8365525 - Attachment is obsolete: true
Attachment #8377615 - Flags: review?(jaws)
Attachment #8377615 - Flags: review?(georg.fritzsche)
Comment on attachment 8377615 [details] [diff] [review]
bug-921730.patch

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

This looks good to me, i only noticed minor issues now.

::: browser/base/content/browser-plugins.js
@@ +380,5 @@
>          plugin.addEventListener("underflow", resizeListener);
>        }
>      }
>  
> +	let closeIcon = this.getPluginUI(plugin, "closeIcon");

This is indented a bit too much.

::: browser/base/content/test/general/browser.ini
@@ +101,5 @@
>    test_no_mcb_on_http_site_font2.html
>    test_no_mcb_on_http_site_font2.css
>    xul_tooltiptext.xhtml
>  
> +[browser_CTP_hide_overlay.js]

I think we want to have those entries alphabetically sorted, so this should probably go after [browser_CTP_drag_drop.js].

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +60,5 @@
> +    elems[0].clientTop;
> +    executeSoon(func);
> +  };
> +}
> +

Looking at it now, i think it would be good to put a comment here to describe what the test is for.
Attachment #8377615 - Flags: review?(georg.fritzsche) → feedback+
Attached patch bug_921730.patch (obsolete) — Splinter Review
Added the comment for the test and removed the trailing white spaces.
Attachment #8377615 - Attachment is obsolete: true
Attachment #8377615 - Flags: review?(jaws)
Attachment #8378126 - Flags: review?(jaws)
Attachment #8378126 - Flags: review?(georg.fritzsche)
Comment on attachment 8378126 [details] [diff] [review]
bug_921730.patch

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

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +21,5 @@
> +  gTestBrowser = gBrowser.selectedBrowser;
> +  gTestBrowser.addEventListener("load", pageLoad, true);
> +
> +  Services.prefs.setBoolPref("plugins.click_to_play", true);
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY);

I completely missed this before: This should set to STATE_DISABLED, not STATE_CLICKTOPLAY.

@@ +61,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +// Test that checks the overlay visibility of the closeIcon(x) when the user is prompted to activate the flash plugin. 

How about something like:
Test that the overlay can be hidden for disabled plugins using the close icon.
Attachment #8378126 - Flags: review?(georg.fritzsche)
Attached patch bug_921730.patch (obsolete) — Splinter Review
Changes made according to the comment 44. 

Sorry for that, I forgot about that before.
Attachment #8378126 - Attachment is obsolete: true
Attachment #8378126 - Flags: review?(jaws)
Attachment #8378219 - Flags: review?(jaws)
Attachment #8378219 - Flags: review?(georg.fritzsche)
Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

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

This looks good to me now, thanks!
Only a minor whitespace issue that can be fixed after jaws reviewed.

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +61,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +// Tests that the overlay can be hidded for disabled plugins using the close icon. 

There is a trailing space here.
Attachment #8378219 - Flags: review?(georg.fritzsche) → feedback+
Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

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

With this patch applied, I am still unable to close the plugin placeholder on the website linked to in comment #0.

Instead, I actually see no plugin placeholder overlay content (just the a blank grey box). This may be already present as people who haven't applied this patch have encountered this too. See http://screencast.com/t/Om5TyCDcS for a screenshot of what I'm seeing.
Attachment #8378219 - Flags: review?(jaws) → review-
Hm, for me the overlay is showing & closing fine with the patch applied for both disabled and click-to-play Flash.
I tried:
http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
http://wiesenmarkt.at/news.php
http://ce.uwex.edu/techsupport/flashtest.aspx
(In reply to Jared Wein [:jaws] (please needinfo? me) [PTO 2/20-2/23] from comment #47)
> See http://screencast.com/t/Om5TyCDcS for a screenshot of what I'm seeing.

Hm, looking at this again i'm pretty sure we have some other issue:
The notification bar is visible & the overlay hidden, so it looks like the browser-plugins.js code decided that the plugin was not visible for some reason.
(In reply to Jared Wein [:jaws] (please needinfo? me) [PTO 2/20-2/23] from comment #47)
> See http://screencast.com/t/Om5TyCDcS
> for a screenshot of what I'm seeing.

Was the embed not fully visible upon loading the website?
If yes, that's probably covered by bug 968762.
Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

Redoing the review since the issue I have is not related to this patch.
Attachment #8378219 - Flags: review- → review?(jaws)
Attachment #8378219 - Flags: review?(jaws) → review+
Anup, can you fix the whitespace and change the commit message to use r=jaws (as jaws did the actual review, not me)?
Then this should be good to go.
Attached patch bug_921730.patchSplinter Review
Removed the white spaces and changed the reviewer as requested in the above comment.
Attachment #8378219 - Attachment is obsolete: true
Attachment #8381388 - Flags: review?(jaws)
Attachment #8381388 - Flags: review?(georg.fritzsche)
Comment on attachment 8381388 [details] [diff] [review]
bug_921730.patch

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

Thanks, this looks good. This was already reviewed and only addresses previous comments, so it's good to go.
Attachment #8381388 - Flags: review?(jaws)
Attachment #8381388 - Flags: review?(georg.fritzsche)
Attachment #8381388 - Flags: feedback+
Testing the URL of comment #0 with https://hg.mozilla.org/integration/mozilla-inbound/rev/395177ab859b and Flash being either set to "ask" or disabled or not installed, clicking the X hides the the placeholder-text and the X-button, but not the gray area itself.

Is this intended behavior? 

Because this results in the javascript-menu remaining unreachable. See the arrow pointing at the bottom border of the menu being hidden by the placeholder.
(In reply to Elbart from comment #56)
> Testing the URL of comment #0 with
> https://hg.mozilla.org/integration/mozilla-inbound/rev/395177ab859b and
> Flash being either set to "ask" or disabled or not installed, clicking the X
> hides the the placeholder-text and the X-button, but not the gray area
> itself.
> 
> Is this intended behavior?

Yes, the actual close behaviour didn't change - it just hides the overlay but not the actual plugin element.
Can you file a new bug about that issue?
(In reply to Georg Fritzsche [:gfritzsche] from comment #57)
> Yes, the actual close behaviour didn't change - it just hides the overlay
> but not the actual plugin element.
> Can you file a new bug about that issue?

Done: bug 976769
https://hg.mozilla.org/mozilla-central/rev/3c2965b5214b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(In reply to Georg Fritzsche [:gfritzsche] from comment #48)
> Hm, for me the overlay is showing & closing fine with the patch applied for
> both disabled and click-to-play Flash.
Confirmed. Verified fixed in nightly 30.0a1(2014-02-26) Win 7, Mac OS X 10.8.5, Ubuntu 13.04.
Further work continues in bug 976769.
Status: RESOLVED → VERIFIED
I'm marking this as won't fix for ESR24 based on the low severity.
You need to log in before you can comment on or make changes to this bug.