980943 - Closing the CTP overlay and then clicking on it trigger the CTP pop-up and restore the "Activate" text

Status: RESOLVED FIXED

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

Description

[Paul Silaghi, QA [:pauly]]

STR:
1. Set Flash to 'Ask to Activate'
2. Open a youtube video
3. Close the CTP overlay from the X button
4. Click on the overlay

AR: The CTP popup shows up and the "Activate" text is restored
Nightly 30.0a1 (2014-03-07), Win 7 x64

Comment 1

[Paul Silaghi, QA [:pauly]]

Regression of bug 853973

Comment 2

[Georg Fritzsche [:gfritzsche]]

Will probably be fixed by bug 979318, taking this to keep track.

Comment 3

[Paul Silaghi, QA [:pauly]]

Not fixed by bug 979318.

Comment 4

[Georg Fritzsche [:gfritzsche]]

The overlay click listener is added here:
http://hg.mozilla.org/mozilla-central/annotate/882826199076/browser/base/content/browser-plugins.js#l600

For this bug it should be enough to remove the listener in the close button handler:
http://hg.mozilla.org/mozilla-central/annotate/882826199076/browser/base/content/browser-plugins.js#l391

We should add test-coverage to this test here:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_CTP_hide_overlay.js

This involves synthesizing a mouse click on the element and checking that the notification is still dismissed, similar to here:
http://hg.mozilla.org/mozilla-central/annotate/882826199076/browser/base/content/test/plugins/browser_pluginnotification.js#l460

Comment 5

[Georg Fritzsche [:gfritzsche]]

... and finally, running the relevant test here is done by: 
> mach mochitest-browser browser/base/content/test/plugins/browser_CTP_hide_overlay.js

Comment 6

[Anup Kumar [:reznord]]

Hi,

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

Thanks in advance,

Regards,
Anup.

Comment 7

[Benjamin Smedberg]

Anup, we typically don't assign owners until you've submitted your first patch. Please feel free to work on this anyway! Let Georg or me know if you have questions.

Comment 8

[Trevor Rowbotham [:rowbot]]

Attachment: bug980943_remove_event_listener.diff

Remove the click event listener from the CTP overlay after the user dismisses the overlay.

Comment 9

[Trevor Rowbotham [:rowbot]]

Attachment: bug980943_test.diff

Test that the overlay remains hidden and that no plugin notification is shown when clicking on the overlay after it is dismissed by the user.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8630063 [details] [diff] [review]
bug980943_remove_event_listener.diff

Yoink

Comment 11

[Trevor Rowbotham [:rowbot]]

I forgot to request review for the patch in comment 9 as well, but I don't have the ability to add a review request after the fact.  So, if you would kindly review that patch as well, I would appreciate it.  And when you get a chance, could you assign this bug to me?  Thanks!

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8630064 [details] [diff] [review]
bug980943_test.diff

Done and done!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8630064 [details] [diff] [review]
bug980943_test.diff

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

Thanks for the test! This looks good - just one suggestion.

::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js
@@ +83,5 @@
> +    // Simulate clicking on the overlay.
> +    utils.sendMouseEvent("mousedown", overlayLeft, overlayTop, 0, 1, 0, false, 0, 0);
> +    utils.sendMouseEvent("mouseup", overlayLeft, overlayTop, 0, 1, 0, false, 0, 0);
> +
> +    return plugin && overlay.classList.contains("visible");

This will return false if plugin is not defined, which will cause the test to pass.

I think I'd prefer this to return true if what is expected is true, namely:

let overlayHidden = yield ContentTask.spawn(... {
  // ...
  return plugin && !overlay.classList.contains("visible");
}

And then:

ok(notification.dismissed, "No notification should be shown");
ok(overlayHidden, "Overlay should be hidden");

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8630063 [details] [diff] [review]
bug980943_remove_event_listener.diff

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

This breaks the case when the overlay is closed, and then re-opened.

STR:

1) Click on X to close the overlay
2) Right click on the plugin area
3) Choose "Activate this plugin" from the context menu
4) Ignore the doorhanger, and try clicking on the plugin overlay

ER:

The doorhanger should re-show

AR:

The doorhanger does not re-show, since we had removed the click event handler in step 1.


There's so much state involved with this overlay, it's kinda madness. What I'd suggest we do is tie the handling of the click event to the visibility class of the overlay.

So instead of removing the click event listener, can you try doing a check in onOverlayClick for the visibility of the overlay, and ignoring the event if the overlay is not visible?

Comment 15

[Trevor Rowbotham [:rowbot]]

Attachment: bug980943_check_visibility_on_click.diff

> So instead of removing the click event listener, can you try doing a check in
> onOverlayClick for the visibility of the overlay, and ignoring the event if the
> overlay is not visible?
Done.  This works now with your STR in comment 14.

Comment 16

[Trevor Rowbotham [:rowbot]]

Attachment: bug980943_test v2

Implemented your suggestions so that the test fails if plugin is undefined.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8631159 [details] [diff] [review]
bug980943_check_visibility_on_click.diff

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

r=me with these last changes. Thanks!

::: browser/modules/PluginContent.jsm
@@ +627,4 @@
>      // 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('visible') && event.button == 0 && event.isTrusted) {

Busted indentation - this should be aligned with the ! and the ( in the two lines above.

Also, let's break up the next && term onto the next lines, so:

overlay.classList.contains("visible") &&
event.button == 0 &&
event.isTrusted)

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8631165 [details] [diff] [review]
bug980943_test v2

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

Looks great, thanks!

Comment 19

[Trevor Rowbotham [:rowbot]]

Attachment: bug980943_check_visibility_on_click v2

Fixed indentation and broke it into multiple lines as suggested.

Asking for re-review since I made changes.

Whats the actual policy on this anyways, it's not clear to me?  So, you gave my patch an r+ on the basis that I would make a few simple changes, which I am more than happy to make.  Do I still need to request review after making those changes or do I just attach a new patch with the changes in it and obsolete the old one without requesting another review?  I'd hate to waste people's time asking for reviews if it's not necessary to do so.

Also, I don't have the ability to do push a try run, so that is something you will need to do for me.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8631187 [details] [diff] [review]
bug980943_check_visibility_on_click v2

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

(In reply to Trevor Rowbotham from comment #19)
> Created attachment 8631187 [details] [diff] [review]
> bug980943_check_visibility_on_click v2
> 
> Fixed indentation and broke it into multiple lines as suggested.
> 
> Asking for re-review since I made changes.
> 
> Whats the actual policy on this anyways, it's not clear to me?  So, you gave
> my patch an r+ on the basis that I would make a few simple changes, which I
> am more than happy to make.  Do I still need to request review after making
> those changes or do I just attach a new patch with the changes in it and
> obsolete the old one without requesting another review?  I'd hate to waste
> people's time asking for reviews if it's not necessary to do so.

Heh, yeah, sorry, that was my fault. So, for people who have landing abilities, an r+ with fix basically means "Just fix these things, and land r=me". But because you don't have direct landing privileges, it makes less sense for you. I should have r-'d, and I'm sorry for the confusion.

But this version of the patch looks good, so r=me. :)

> 
> Also, I don't have the ability to do push a try run, so that is something
> you will need to do for me.

I can definitely do that for you.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c49ee71cdade

Needinfo'ing myself to check-in once this comes back green.

Comment 22

[Trevor Rowbotham [:rowbot]]

> Heh, yeah, sorry, that was my fault. So, for people who have landing abilities, an r+ with fix basically means "Just fix
> these things, and land r=me". But because you don't have direct landing privileges, it makes less sense for you. I should
> have r-'d, and I'm sorry for the confusion.
It's all good.  Thanks for the explanation, that makes sense.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #21)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c49ee71cdade
> 
> Needinfo'ing myself to check-in once this comes back green.

Green!

Comment 24

[Trevor Rowbotham [:rowbot]]

Awesome! Thanks for checking this in for me.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d0420db83f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce793a58a526

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2d0420db83f
https://hg.mozilla.org/mozilla-central/rev/ce793a58a526

Comment 27

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a007a5ab71