1333071 - Empty permissions confirmation doorhanger

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[Vasilica Tamas, QA [:vtamas]]

[Affected versions]:
Firefox 53.0a1 (2017-01-23)

[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create extensions.webextPermissionPrompts and set it to true
3.Navigate to https://addons.mozilla.org/en-US/firefox/addon/awesome-screenshot-capture-/?src=ss
4.Install the webextension. 


[Expected Results]:
The confirmation doorhanger is correctly displayed.


[Actual Results]:
- There is no text in confirmation doorhanger and it is automatically closed after about 2 seconds.
- See screencast: https://www.screencast.com/t/JqyAoububf

[Regression Range]:
Last good revision: 487a4e43eb9d1f04a5d8e3dd183fe38dbe105e1f
First bad revision: 160914d65dac284bba2646f52a1ce1a5aa2d22b1
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=487a4e43eb9d1f04a5d8e3dd183fe38dbe105e1f&tochange=160914d65dac284bba2646f52a1ce1a5aa2d22b1

I suspect that this issue was regressed by Bug 1308296.

Comment 1

[krupa raj [:krupa--use this to needinfo]]

I actually see https://www.dropbox.com/s/95er9n4ed5f5ic8/Screenshot%202017-01-23%2007.34.01.png?dl=0

The list of permissions is not very useful or grammatically correct.

Comment 2

[Andrew Swan [:aswan]]

Krupa, it looks like you are running a build from before bug 1316996 landed.
Also, your screenshot is of the permissions dialog, I think this bug is about the "your add-on is ready" dialog that comes after the install is done.
But Awesome Screenshot appears to navigate the browser away from the AMO page immediately when it is installed, I'm not sure how it does that, let me look a little more closely.

Comment 3

[krupa raj [:krupa--use this to needinfo]]

I upgraded to the latest Nightly and I see the issue now.

Comment 4

[:shell escalante]

what is the desired behavior?

Comment 5

[[e-mail]]

(In reply to :shell escalante from comment #4)
> what is the desired behavior?

Looking at the code in AMO if the add-on has a post-install contribution setup then after the addon is installed a callback is fired which causes the page to change to the developers page. The callback is here: https://github.com/mozilla/addons-server/blob/0c2b633c592a882a51698949f2262773d1462b5b/static/js/zamboni/mobile/buttons.js#L8

Ideally I would expect the AMO behaviour to not cause the doorhanger to be closed prematurely.

Comment 6

[Andrew Swan [:aswan]]

Meaning the doorhanger should stay visible even when the foreground tab navigates to a new page?
I think that popup notifications are pretty closely tied to navigation and that would be tricky to implement in a reliable way.

Can the contribution page open in a separate tab?

Comment 7

[Andy McKay]

How popular is the post-install contribution step, is it worth keeping in its current form?

Comment 8

[[e-mail]]

(In reply to Andrew Swan [:aswan] from comment #6)
> Meaning the doorhanger should stay visible even when the foreground tab
> navigates to a new page?
> I think that popup notifications are pretty closely tied to navigation and
> that would be tricky to implement in a reliable way.
> 
> Can the contribution page open in a separate tab?

I think that would be difficult to open a page reliably in a new tab via JS especially if not off a direct user click event.

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1333071 Escape addon name in post-install dialog

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

Comment 10

[Andrew Swan [:aswan]]

We ended up with two different issues here on this bug (my fault).  The attached patch should fix the original popup contents bug.  I'm going to hold off filing a new bug for the AMO navigation issue in the hopes that we decide to address it outside of Firefox.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

The post-install contribution step is enabled for hundreds of active add-ons, including many with very high usage, so I don't think we can just get rid of it.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8829565 [details]
Bug 1333071 Escape addon name in post-install dialog

https://reviewboard.mozilla.org/r/106606/#review107820

r=me. I wonder if we should purposeful insert < & > characters in the add-on used for tests so that such an error would be caught in the future.

Comment 13

[Florian Quèze [:florian]]

(In reply to Andrew Swan [:aswan] from comment #6)
> Meaning the doorhanger should stay visible even when the foreground tab
> navigates to a new page?
> I think that popup notifications are pretty closely tied to navigation and
> that would be tricky to implement in a reliable way.

Look at the 'persistence' option of popupnotifications: http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/toolkit/modules/PopupNotifications.jsm#330

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8829565 [details]
Bug 1333071 Escape addon name in post-install dialog

https://reviewboard.mozilla.org/r/106606/#review107826

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36ced1bb7ae1
Escape addon name in post-install dialog r=florian

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36ced1bb7ae1

Comment 17

[Vasilica Tamas, QA [:vtamas]]

Confirm that permissions confirmation text is successfully displayed on Firefox 54.0a1 (2017-01-25) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.11: https://www.screencast.com/t/lWMyFX0NgUI

Andrew, did you log the issue related to contribution page or should I file a new one?

Comment 18

[Andrew Swan [:aswan]]

I haven't opened a new bug yet since it was not clear whether this should be handled as a Firefox bug or an AMO bug.
The persistence option that Florian mentioned does sound like exactly what we want though, I'll give that a try and open a new bug if it looks workable.  Leaving the ni? to myself until that's done.

Comment 19

[Andrew Swan [:aswan]]

Filed bug 1334354 for the AMO situation

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Does this need an Aurora approval request?

Comment 21

[Andrew Swan [:aswan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Does this need an Aurora approval request?

I don't think so, permissions prompts for webextensions landed initially in 53 but they are not enabled unless a (hidden and undocumented) preference is set.  There have been a number of follow-up bugs fixed in 54 and a few more to go, assuming it all gets fixed in time, the plan is to flip that preference on by default in 54.  But I don't think there's much point in uplifting some subset of the follow-ups and there have been enough of them that I wouldn't propose uplifting them all.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Definitely not, thanks. Updating the status accordingly :)