Implement the first pieces of the all-doorhanger install flow for add-ons installed from websites

VERIFIED FIXED in Firefox 39

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mossop, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla39
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox39 verified)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

2 years ago
This is a first step for bug 1123914 that we can do now while waiting on the signing code to be finished.

The existing installation process shows a doorhanger for download progress, then a modal dialog for installing, then a doorhanger for the completed install. We want to change this to be all doorhanger.

We should be able to implement the top-line certified mockups from https://bug1120996.bugzilla.mozilla.org/attachment.cgi?id=8560498 (because at this point we have no signing requirement) and then later on add the other messages/errors when signing code is available.

We still need final imagery here and answers to a few questions, but implementation work can probably start even without that.

Some comments from a chat with Markus earlier in the week:

The mockups don't include error messages for blockisted/corrupt files etc. We should just use the existing error messages for these cases for the time-being.

What to do when the user switches away from the installing tab? We should switch back to the tab when installation is ready to proceed.

What to do when the user closes the installing tab? We should cancel the install.

What to do when the user navigates to a new website in the installing tab? Markus was going to think about this one.
(Reporter)

Comment 1

2 years ago
(In reply to Dave Townsend [:mossop] from comment #0)
> We should be able to implement the top-line certified mockups from
> https://bug1120996.bugzilla.mozilla.org/attachment.cgi?id=8560498 (because
> at this point we have no signing requirement) and then later on add the
> other messages/errors when signing code is available.

Actually this is not quite correct as we should still warn the user about installing add-ons. The uncertified line looks right though we might want to tweak the wording of the warning panel.
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 2

2 years ago
I know very little about how the add-on install flow works under the hood, so I can't really estimate how much work this will be. I'll just make a very wild guess...
Assignee: nobody → dao
Iteration: --- → 39.2 - 23 Mar
Points: --- → 8

Updated

2 years ago
Status: NEW → ASSIGNED
Created attachment 8578019 [details]
150316_All-Doorhanger-Add-On-Install.jpg

Updated flow for the all-doorhanger install. Basic case for certified add-ons including errors and aborting install as discussed with Dave.
Please tell me if I have missed any cases.
Otherwise I will get Visual Design and Copy reviewed and create a mockup visualizing the transition of content in a doorhanger.
(Assignee)

Comment 4

2 years ago
Created attachment 8579344 [details] [diff] [review]
WIP

Couple of notes / questions:

- This leaves xpinstallConfirm.xul for other applications. Should we remove it and let other apps implement it themselves?

- I'll probably need to update tests that deal with xpinstallConfirm.xul (or should I just have them flip xpinstall.customConfirmationUI for now?)

- I didn't make changes to the existing popup notifications, as these were irrelevant for replacing the confirmation dialog window with a notification and/or require us to further fork the standard popupnotification binding. I think the latter is bad both in terms of consistency across all popup notifications as well as maintenance-wise; instead, I guess we should properly implement bug 1064257 at some point.

- For consistency across our popup notifications, I didn't add the domain name as a heading for the new notification. The old dialog doesn't highlight the domain name (it does however spell out the XPI URL), so I don't think this would be a strict requirement for the notification. It's also unclear to me how this layout should work for installations from different sources that can theoretically end up in the same notification, can't they?

- Related to the above point, I'm not sure how exactly how the domain change restriction should be implemented, but I think that's also orthogonal to converting the dialog to a notification just like the existing notifications which never had any such restriction.

- I haven't added a Cancel button to the new notification, because this deviates from the standard design again. It would also make the notification look modal when in fact users can just dismiss it and ignore it for however long they wish. I could add "Cancel" to the secondary actions (under the dropdown next to the Install button), but there's already "Not Now"...

- I made it so that the Install button is initially disabled, but I'm thinking that I should actually drop this given that all popup notifications already have a security delay without visibly disabling the button (implemented in bug 583175).

- I haven't added a Learn More link, because I don't know what URL that should open.
Attachment #8579344 - Flags: feedback?(dtownsend)
(Assignee)

Comment 5

2 years ago
Created attachment 8579348 [details] [diff] [review]
WIP 2

I missed this part, now implemented:

> What to do when the user switches away from the installing tab? We should
> switch back to the tab when installation is ready to proceed.
Attachment #8579344 - Attachment is obsolete: true
Attachment #8579344 - Flags: feedback?(dtownsend)
Attachment #8579348 - Flags: feedback?(dtownsend)
(Reporter)

Comment 6

2 years ago
(In reply to Dão Gottwald [:dao] from comment #4)
> Created attachment 8579344 [details] [diff] [review]
> WIP
> 
> Couple of notes / questions:
> 
> - This leaves xpinstallConfirm.xul for other applications. Should we remove
> it and let other apps implement it themselves?

Let's leave it for now

> - I'll probably need to update tests that deal with xpinstallConfirm.xul (or
> should I just have them flip xpinstall.customConfirmationUI for now?)

The tests in toolkit/mozapps/extensions/test/xpinstall can probably flip that and stay as is. Other tests, particularly browser_bug553455.js should be updated for the new notification.

> - I didn't make changes to the existing popup notifications, as these were
> irrelevant for replacing the confirmation dialog window with a notification
> and/or require us to further fork the standard popupnotification binding. I
> think the latter is bad both in terms of consistency across all popup
> notifications as well as maintenance-wise; instead, I guess we should
> properly implement bug 1064257 at some point.

I think at least the wording updates need to go in but probably also the button changes. IIRC the cancel download button is different to that in the mockups.

> - For consistency across our popup notifications, I didn't add the domain
> name as a heading for the new notification. The old dialog doesn't highlight
> the domain name (it does however spell out the XPI URL), so I don't think
> this would be a strict requirement for the notification. It's also unclear
> to me how this layout should work for installations from different sources
> that can theoretically end up in the same notification, can't they?

The source domain should be the installing page rather than the XPI's domain. This should be the same even if the same page installs many XPIs at a time.

> - Related to the above point, I'm not sure how exactly how the domain change
> restriction should be implemented, but I think that's also orthogonal to
> converting the dialog to a notification just like the existing notifications
> which never had any such restriction.

I'm not sure what restriction you're referring to here?

> - I haven't added a Cancel button to the new notification, because this
> deviates from the standard design again. It would also make the notification
> look modal when in fact users can just dismiss it and ignore it for however
> long they wish. I could add "Cancel" to the secondary actions (under the
> dropdown next to the Install button), but there's already "Not Now"...
> 
> - I made it so that the Install button is initially disabled, but I'm
> thinking that I should actually drop this given that all popup notifications
> already have a security delay without visibly disabling the button
> (implemented in bug 583175).

I forgot to mention this. We'd talked about still having a delay but that the delay would be measured from the time the first doorhanger appears so yes in many cases if the download is slow enough the install button would be immediately enabled.

> - I haven't added a Learn More link, because I don't know what URL that
> should open.

Markus, can you comment on the above points too?
Flags: needinfo?(mjaritz)
(Reporter)

Comment 7

2 years ago
Dan, can you confirm whether the add-on author's name appears in the cert or will that just be Mozilla?
Flags: needinfo?(dveditz)
(Reporter)

Comment 8

2 years ago
Comment on attachment 8579348 [details] [diff] [review]
WIP 2

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

Looks like this is along the right path, I'd like Markus to chime in on some of your questions though.

::: browser/base/content/browser-addons.js
@@ +184,5 @@
> +              let cert = document.createElement("label");
> +              name.setAttribute("value", install.addon.name);
> +              cert.setAttribute("value", install.certName ?
> +                gNavigatorBundle.getFormattedString("addonConfirmInstall.authorSigned", [install.certName]) :
> +                gNavigatorBundle.getString("addonConfirmInstall.authorUnsigned"));

The intent is to only display the author now as in the future all add-ons will be signed, I think they will all be signed by Mozilla too so we probably just need to display the author name (install.addon.creator) but I've asked Dan to confirm that the name isn't in the cert.

::: toolkit/modules/PopupNotifications.jsm
@@ +202,5 @@
>     *            pressed.
>     *          - [optional] dismiss (boolean): If this is true, the notification
>     *            will be dismissed instead of removed after running the callback.
> +   *          - [optional] securityDelay (boolean): If this is true, the button
> +   *            will initially be disabled and become enabled with a delay.

As I said in the bug we'd like to only delay depending on how long the doorhangers have been opened so maybe this should be a number for how long to delay for.

::: toolkit/mozapps/extensions/amWebInstallListener.js
@@ +153,5 @@
>      // If none of the downloads were successful then exit early
>      if (this.downloads.length == 0)
>        return;
>  
> +    try {

Rather than the try...catch lets use Preferences.jsm here.
Attachment #8579348 - Flags: feedback?(dtownsend) → feedback+
(Assignee)

Comment 9

2 years ago
(In reply to Dave Townsend [:mossop] from comment #6)
> > - For consistency across our popup notifications, I didn't add the domain
> > name as a heading for the new notification. The old dialog doesn't highlight
> > the domain name (it does however spell out the XPI URL), so I don't think
> > this would be a strict requirement for the notification. It's also unclear
> > to me how this layout should work for installations from different sources
> > that can theoretically end up in the same notification, can't they?
> 
> The source domain should be the installing page rather than the XPI's
> domain. This should be the same even if the same page installs many XPIs at
> a time.

Is this information already available to browser-addons.js? I assume that I can't just use the currently loaded page's domain, because the installing page might be in an iframe from a different domain. Also, what about the user dropping a locally stored XPI on the browser? I'm not sure if there are more such edge cases...

> > - Related to the above point, I'm not sure how exactly how the domain change
> > restriction should be implemented, but I think that's also orthogonal to
> > converting the dialog to a notification just like the existing notifications
> > which never had any such restriction.
> 
> I'm not sure what restriction you're referring to here?

I meant cancelling the installation when the web site is being navigated away from.
(In reply to Dave Townsend [:mossop] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > 
> > Couple of notes / questions:
> > 

> > - I didn't make changes to the existing popup notifications, as these were
> > irrelevant for replacing the confirmation dialog window with a notification
> > and/or require us to further fork the standard popupnotification binding. I
> > think the latter is bad both in terms of consistency across all popup
> > notifications as well as maintenance-wise; instead, I guess we should
> > properly implement bug 1064257 at some point.
> 
> I think at least the wording updates need to go in but probably also the
> button changes. IIRC the cancel download button is different to that in the
> mockups.
> 
> > - For consistency across our popup notifications, I didn't add the domain
> > name as a heading for the new notification. The old dialog doesn't highlight
> > the domain name (it does however spell out the XPI URL), so I don't think
> > this would be a strict requirement for the notification. It's also unclear
> > to me how this layout should work for installations from different sources
> > that can theoretically end up in the same notification, can't they?
> 
> The source domain should be the installing page rather than the XPI's
> domain. This should be the same even if the same page installs many XPIs at
> a time.

Yes. This should be the page initiating the install. 
And you mention it is possible to install multiple XPIs at a time? 
Do you have an example, how often is this used, and how many at once?

> > - Related to the above point, I'm not sure how exactly how the domain change
> > restriction should be implemented, but I think that's also orthogonal to
> > converting the dialog to a notification just like the existing notifications
> > which never had any such restriction.
> 
> I'm not sure what restriction you're referring to here?
> 
> > - I haven't added a Cancel button to the new notification, because this
> > deviates from the standard design again. It would also make the notification
> > look modal when in fact users can just dismiss it and ignore it for however
> > long they wish. I could add "Cancel" to the secondary actions (under the
> > dropdown next to the Install button), but there's already "Not Now"...

I know that a lot of doorhangers have the dropdown, but I have seen in Stephens Design resources that it is also an option to provide multiple options. Yes it is more like a notification then, but at one point after starting the install (download), the user has to decide if they want to install or not. Therefor the cancel needs to be more prominent. Not now is not required in this case.

> > - I haven't added a Learn More link, because I don't know what URL that
> > should open.

I will follow up on that.
Flags: needinfo?(mjaritz)
> Is this information already available to browser-addons.js? I assume that I
> can't just use the currently loaded page's domain, because the installing
> page might be in an iframe from a different domain. Also, what about the
> user dropping a locally stored XPI on the browser? I'm not sure if there are
> more such edge cases...

It is great that we see all this edge cases that we have to solve. 
- multiple XPIs
- iframe
- dropping local XPI

Do you know how often those occur? With this basic case I was hoping to optimize for the most used cases and to deviate from that for every edge case once we have that running smooth.
(Reporter)

Comment 12

2 years ago
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #6)
> > > - For consistency across our popup notifications, I didn't add the domain
> > > name as a heading for the new notification. The old dialog doesn't highlight
> > > the domain name (it does however spell out the XPI URL), so I don't think
> > > this would be a strict requirement for the notification. It's also unclear
> > > to me how this layout should work for installations from different sources
> > > that can theoretically end up in the same notification, can't they?
> > 
> > The source domain should be the installing page rather than the XPI's
> > domain. This should be the same even if the same page installs many XPIs at
> > a time.
> 
> Is this information already available to browser-addons.js? I assume that I
> can't just use the currently loaded page's domain, because the installing
> page might be in an iframe from a different domain. Also, what about the
> user dropping a locally stored XPI on the browser? I'm not sure if there are
> more such edge cases...

The installInfo object has an originatingURI property, it will be null for dropped files IIRC so perhaps in that case we should show a message relevant to that.

> > > - Related to the above point, I'm not sure how exactly how the domain change
> > > restriction should be implemented, but I think that's also orthogonal to
> > > converting the dialog to a notification just like the existing notifications
> > > which never had any such restriction.
> > 
> > I'm not sure what restriction you're referring to here?
> 
> I meant cancelling the installation when the web site is being navigated
> away from.

So should be as straightforward as cancelling the installs when oritinatingURI.host doesn't match a new page loaded in the tab.
(Assignee)

Comment 13

2 years ago
Created attachment 8580767 [details] [diff] [review]
patch

I'll file follow-up bugs on the Learn More link and on removing the notifications when the host changes. I still think the latter should not block converting the window to a notification, as the existing notifications have never behaved this way. I'm also unsure whether preventing the page from unloading and reusing the tab-modal dialog for that is feasible (or a good idea anyway).
Attachment #8579348 - Attachment is obsolete: true
Attachment #8580767 - Flags: review?(dtownsend)
(Reporter)

Comment 14

2 years ago
So what happens with the current patch when the page changes? The notification is just still available on the tab?

How about when the tab is closed? Do the installs just sit dormant somehow?
(Assignee)

Comment 15

2 years ago
(In reply to Dave Townsend [:mossop] from comment #14)
> So what happens with the current patch when the page changes? The
> notification is just still available on the tab?

I believe the existing notifications stay for at least 30 seconds and then disappear when navigating after that point; the new notification copies that behavior.

> How about when the tab is closed? Do the installs just sit dormant somehow?

These popup notifications are always tab-specific, so they would go away with a tab. However I'm not sure PopupNotifications.jsm fires a "removed" event in that case, so the installs might linger in the background... and are hopefully automatically cancelled when restarting the application? There's also the scenario of the whole window closing, in which case there's no TabClose event, so just listening to that wouldn't be sufficient.

Oh, and on the security delay for the install button... I've never seen the progress notification stay shorter than approximately 2 or 3 seconds (for comparison, security.dialog_enable_delay is one second), so I'm not sure it's necessary to add a conditional delay. Could probably be a followup bug too if you still think it's needed.
(Reporter)

Comment 16

2 years ago
(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Dave Townsend [:mossop] from comment #14)
> > So what happens with the current patch when the page changes? The
> > notification is just still available on the tab?
> 
> I believe the existing notifications stay for at least 30 seconds and then
> disappear when navigating after that point; the new notification copies that
> behavior.
> 
> > How about when the tab is closed? Do the installs just sit dormant somehow?
> 
> These popup notifications are always tab-specific, so they would go away
> with a tab. However I'm not sure PopupNotifications.jsm fires a "removed"
> event in that case, so the installs might linger in the background... and
> are hopefully automatically cancelled when restarting the application?
> There's also the scenario of the whole window closing, in which case there's
> no TabClose event, so just listening to that wouldn't be sufficient.

We should get a follow-up bug on file for doing sane things here rather than just dropping the installs on the floor. At the least I note that the add-ons manager UI looks bad for these cases so we either need to fix that or properly cancel the installs when they are no longer reachable through the UI.
(Reporter)

Comment 17

2 years ago
Comment on attachment 8580767 [details] [diff] [review]
patch

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

Getting to the install prompt in no time at all is pretty trivial. Go to AMO, start installing an add-on then cancel at the install doorhanger. Now try installing it again. Firefox has the file cached so download is near instant and we get straight to the install prompt. This is probably easy for a website to trigger too so I think we need the delay in place.

I think the most straightforward way to handle this is this: The progress notification currently hides itself when everything is downloaded (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1753). Instead just have it stay open, maybe switch the progress bar to undetermined state, and have the code for showing the failed/confirmation doorhangers close the progress doorhanger explicitly. Then when the notification to show the confirmation arrives you just have to wait however long before triggering the confirmation UI.

Can we add a test that switching back to the originating tab works?

::: browser/base/content/browser-addons.js
@@ +122,5 @@
> +            break;
> +          case "shown":
> +            setTimeout(() => {
> +              this._progressNotificationSize =
> +                document.getElementById("addon-progress-notification").getBoundingClientRect();

It's unlikely but possible that two sites can trigger installs close together in which case the latter will clobber the size of the first. The same problem will probably come up for handling the timer too.

How about adding a property to nsIInstallInfo that is guaranteed to be unique for different install attempts then use a weakmap in this code to map from install attempt to whatever properties we want to persist. Just a simple {} would be different everytime.

@@ +143,4 @@
>        // TODO This isn't terribly ideal for the multiple failure case
>        for (let install of installInfo.installs) {
>          let host = (installInfo.originatingURI instanceof Ci.nsIStandardURL) &&
>                     installInfo.originatingURI.host;

Might as well have this use options.originHost now.

@@ +223,5 @@
> +
> +        Services.telemetry
> +                .getHistogramById("SECURITY_UI")
> +                .add(Ci.nsISecurityUITelemetry.WARNING_CONFIRM_ADDON_INSTALL_CLICK_THROUGH);
> +      };

As above, a second install by a page will clobber acceptInstallation leading to accepting the first install actually installing the second's add-ons.

@@ +225,5 @@
> +                .getHistogramById("SECURITY_UI")
> +                .add(Ci.nsISecurityUITelemetry.WARNING_CONFIRM_ADDON_INSTALL_CLICK_THROUGH);
> +      };
> +
> +      if (this._progressNotificationSize) {

This doesn't seem to be working for me

::: toolkit/content/widgets/notification.xml
@@ +478,3 @@
>        <xul:vbox flex="1">
> +        <xul:label class="popup-notification-originHost header"
> +                         xbl:inherits="value=originhost"/>

The mockups show the site favicon here too, can we add that? Can be in a follow-up.

::: toolkit/mozapps/extensions/amWebInstallListener.js
@@ +157,5 @@
>  
> +    if (Preferences.get("xpinstall.customConfirmationUI", false)) {
> +      notifyObservers("addon-install-confirmation", this.browser, this.url, this.downloads);
> +      return;
> +    }

Just realised we could have just used the web-install-prompt component below to override this instead, but I think the pref works well since it gives us a way to override the method for testing.
Attachment #8580767 - Flags: review?(dtownsend)
(Assignee)

Comment 18

2 years ago
(In reply to Dave Townsend [:mossop] from comment #17)
> It's unlikely but possible that two sites can trigger installs close
> together in which case the latter will clobber the size of the first. The
> same problem will probably come up for handling the timer too.

You mean two triggers from the same tab or from different tabs?
Flags: needinfo?(dtownsend)
(Assignee)

Comment 19

2 years ago
Created attachment 8581181 [details] [diff] [review]
patch v2
Attachment #8580767 - Attachment is obsolete: true
Flags: needinfo?(dtownsend)
Attachment #8581181 - Flags: review?(dtownsend)
(Assignee)

Comment 20

2 years ago
(In reply to Dave Townsend [:mossop] from comment #17)
> The mockups show the site favicon here too, can we add that? Can be in a
> follow-up.

I tend to think that's a bad idea. A site could use the Firefox logo, a lock or some other icon that conveys trust / security. That's part of the reason why we removed the favicon from the location bar. In this case malicious web content could even temporarily change its icon to a misleading one right before the user clicks on an install link.
(Assignee)

Comment 21

2 years ago
Created attachment 8581183 [details] [diff] [review]
patch v3

> Can we add a test that switching back to the originating tab works?

I think I misunderstood what you meant here... I've now updated the test to do what I think you really meant.
Attachment #8581181 - Attachment is obsolete: true
Attachment #8581181 - Flags: review?(dtownsend)
Attachment #8581183 - Flags: review?(dtownsend)
(Assignee)

Comment 22

2 years ago
Created attachment 8581188 [details] [diff] [review]
patch v3

updated to tip
Attachment #8581183 - Attachment is obsolete: true
Attachment #8581183 - Flags: review?(dtownsend)
Attachment #8581188 - Flags: review?(dtownsend)
(Reporter)

Comment 23

2 years ago
(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Dave Townsend [:mossop] from comment #17)
> > It's unlikely but possible that two sites can trigger installs close
> > together in which case the latter will clobber the size of the first. The
> > same problem will probably come up for handling the timer too.
> 
> You mean two triggers from the same tab or from different tabs?

I don't think it matters does it? gXPInstallObserver is global for the window.
(Assignee)

Comment 24

2 years ago
I got rid of _progressNotificationSize and moved acceptInstallation to the "shown" handler; this should take care of it. We don't support having multiple notifications with the same id within the same tab.
(Reporter)

Comment 25

2 years ago
Created attachment 8581882 [details]
Screenshot 2015-03-23 11.50.59.png

I see one remaining bug here. Although the patch sets the install confirmation doorhanger to the right size the controls inside aren't flexing to fit that so the buttons all jump when switching from the progress doorhanger.
(Reporter)

Comment 26

2 years ago
Comment on attachment 8581188 [details] [diff] [review]
patch v3

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

Other than the bug I noted in the last comment this is looking great.

::: browser/base/content/urlbarBindings.xml
@@ +1733,5 @@
> +              this.progresstext.value = this.progresstext.tooltipText =
> +                gNavigatorBundle.getString("addonDownloadVerifying");
> +            } else {
> +              PopupNotifications.remove(this.notification);
> +            }

Awesome, thanks for keeping this pref controlled!
Attachment #8581188 - Flags: review?(dtownsend) → review-

Updated

2 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
(Assignee)

Comment 27

2 years ago
Created attachment 8582495 [details] [diff] [review]
patch v4
Attachment #8581188 - Attachment is obsolete: true
Attachment #8581882 - Attachment is obsolete: true
Attachment #8582495 - Flags: review?(dtownsend)
(Reporter)

Comment 28

2 years ago
Comment on attachment 8582495 [details] [diff] [review]
patch v4

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

Awesome, thanks
Attachment #8582495 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/37bd20ca096a
Backed out for test_blocklistchange.js xpcshell failures.
https://hg.mozilla.org/integration/fx-team/rev/6dd5b23f5558

https://treeherder.mozilla.org/logviewer.html#?job_id=2388258&repo=fx-team
(Assignee)

Comment 31

2 years ago
test_blocklistchange.js provides its own web-install-prompt component, but that isn't used when xpinstall.customConfirmationUI is true. I've changed amWebInstallListener.js to let the component take precedence over the pref.

https://hg.mozilla.org/integration/fx-team/rev/74cf6d15cfdd
Comment on attachment 8582495 [details] [diff] [review]
patch v4

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +29,5 @@
>  xpinstallDisabledMessage=Software installation is currently disabled. Click Enable and try again.
>  xpinstallDisabledButton=Enable
>  xpinstallDisabledButton.accesskey=n
>  
> +# LOCALIZATION NOTE (addonDownloading):

The string ID is "addonDownloadingAndVerifying"

@@ +34,4 @@
>  # Semicolon-separated list of plural forms. See:
>  # http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  # Also see https://bugzilla.mozilla.org/show_bug.cgi?id=570012 for mockups
> +addonDownloadingAndVerifying=Downloading and verifying add-on…;Downloading and verifying add-ons…

This doesn't work: locales with complex plural forms need to display the actual number with the noun. 

The existing string was already broken, but this doesn't mean we should keep landing new code and repeat the same mistake.

If you want to drop the number in English is fine, as long as you provide instruction in the localization comment on which variable should be used if you want to display the number. This is obviously not necessary if the second form is already using the variable.
(Assignee)

Comment 33

2 years ago
(In reply to Francesco Lodolo [:flod] (UTC+1) from comment #32)
> Comment on attachment 8582495 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8582495 [details] [diff] [review]:

Addressed your comments: https://hg.mozilla.org/integration/fx-team/rev/a078fa84f760
https://hg.mozilla.org/mozilla-central/rev/74cf6d15cfdd
https://hg.mozilla.org/mozilla-central/rev/a078fa84f760
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 35

2 years ago
I've filed bug 1147805, bug 1147808 and bug 1147812.

Updated

2 years ago
Depends on: 1147823
QA Contact: vasilica.mihasca
(In reply to Dave Townsend [:mossop] from comment #7)
> Dan, can you confirm whether the add-on author's name appears in the cert or
> will that just be Mozilla?

The name will be Mozilla (or Mozilla Addons something?) for ones we sign, which is going to be all of them for some time. If we figure out a way to safely issue certificates to developers the occasional addon will have that name in it. We can't safely use the AMO account name because there's no verification of those and they could be misleading.
Flags: needinfo?(dveditz)
(Reporter)

Updated

2 years ago
Depends on: 1148543
Depends on: 1149329
What will be placed after addonConfirmInstall.message?
(Assignee)

Comment 38

2 years ago
(In reply to Stefan Plewako [:stef] from comment #37)
> What will be placed after addonConfirmInstall.message?

A list of the add-on(s) that are about to be installed.
Updated UI-Flow to reflect blocking uncertified add-ons:
attachment 8586113 [details]
150331_All-Doorhanger-Add-On-Install.jpg
https://bugzilla.mozilla.org/attachment.cgi?id=8586113
(In reply to Dão Gottwald [:dao] from comment #38)
> A list of the add-on(s) that are about to be installed.

Localization note wouldn't hurt (esp since on mockups this looks different) and placeholder and localizable separator too…
(Assignee)

Comment 41

2 years ago
(In reply to Stefan Plewako [:stef] from comment #40)
> (In reply to Dão Gottwald [:dao] from comment #38)
> > A list of the add-on(s) that are about to be installed.
> 
> Localization note wouldn't hurt (esp since on mockups this looks different)

I thought this would be obvious from the en-US string.

> and placeholder and localizable separator too…

What do you mean by that?
(In reply to Dão Gottwald [:dao] from comment #41)
> > and placeholder and localizable separator too…
> 
> What do you mean by that?

An ability to put the list in different place of the sentence and to specify what goes between addon names. Without this, Polish dialog doesn't read really well.
(Assignee)

Comment 43

2 years ago
The message and the list of add-ons are separate UI elements, so this isn't really one continuous sentence. You could also make the message say something like: "This site would like to install the following add-on in Firefox:", or "This site would like to install an add-on in Firefox. Here's its name:", or whatever feels more natural in your language.
(Reporter)

Updated

2 years ago
Blocks: 1149654
Created attachment 8586907 [details]
Screenshot

When I click twice to install two different extensions from single page to test plural form of addonConfirmInstall.message I see something like in the attachment.

How can I test plrural addonConfirmInstall.message and should I report misaligned close button separately?
(Assignee)

Comment 45

2 years ago
(In reply to Stefan Plewako [:stef] from comment #44)
> Created attachment 8586907 [details]
> Screenshot
> 
> When I click twice to install two different extensions from single page to
> test plural form of addonConfirmInstall.message I see something like in the
> attachment.
> 
> How can I test plrural addonConfirmInstall.message

I don't think you can do this manually, but the site itself needs to trigger two or more installations at the same time. I don't know of any site doing that, though, nor do I know of a test page :/

> and should I report misaligned close button separately?

yes
(Reporter)

Comment 46

2 years ago
(In reply to Dão Gottwald [:dao] from comment #45)
> (In reply to Stefan Plewako [:stef] from comment #44)
> > Created attachment 8586907 [details]
> > Screenshot
> > 
> > When I click twice to install two different extensions from single page to
> > test plural form of addonConfirmInstall.message I see something like in the
> > attachment.
> > 
> > How can I test plrural addonConfirmInstall.message
> 
> I don't think you can do this manually, but the site itself needs to trigger
> two or more installations at the same time. I don't know of any site doing
> that, though, nor do I know of a test page :/

This is an old example: https://addons.mozilla.org/en-US/firefox/addon/clickreader/. That attempts to install three add-ons one of which is incompatible so you get a failure and an offer to install two add-ons. That might be a bug but at least you can see the multiple-install message from it.
(In reply to Dave Townsend [:mossop] from comment #46)
> https://addons.mozilla.org/en-US/firefox/addon/clickreader/. That attempts
> to install three add-ons one of which is incompatible so you get a failure
> and an offer to install two add-ons. That might be a bug but at least you
> can see the multiple-install message from it.

Works for me, thx. Close button issue filled as bug 1150218.

Updated

2 years ago
Blocks: 1047239
Depends on: 1147808, 1147812
(Reporter)

Updated

2 years ago
Depends on: 1160562
Depends on: 1154945
(Reporter)

Updated

2 years ago
Depends on: 1163973
Created attachment 8611149 [details]
install.gif

I tested the first pieces of the all-doorhanger install flow on Firefox 39 Beta 1 and this is what I have noticed:
- the xpinstall.customConfirmationUI is set to false which means that for the installing approval is displayed the old prompt window

If this is the expected behavior, I encountered a potential issue:
- 2 install buttons are displayed in 2 different places, one in download doorhanger and the other one in the install prompt window. This behavior can be a bit confusig for the users.
I am attaching a video recorded on Windows where I captured this issue.

Dave, can you explain me what behavior should beta cycle follow?
Flags: needinfo?(dtownsend)
(Reporter)

Comment 49

2 years ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #48)
> Created attachment 8611149 [details]
> install.gif
> 
> I tested the first pieces of the all-doorhanger install flow on Firefox 39
> Beta 1 and this is what I have noticed:
> - the xpinstall.customConfirmationUI is set to false which means that for
> the installing approval is displayed the old prompt window
> 
> If this is the expected behavior, I encountered a potential issue:
> - 2 install buttons are displayed in 2 different places, one in download
> doorhanger and the other one in the install prompt window. This behavior can
> be a bit confusig for the users.
> I am attaching a video recorded on Windows where I captured this issue.
> 
> Dave, can you explain me what behavior should beta cycle follow?

We shouldn't be showing the install button on the progress doorhanger, I've filed bug 1168954
Flags: needinfo?(dtownsend)
(Reporter)

Updated

2 years ago
Depends on: 1168954
Verified this issue on Firefox 39 Beta 2 (20150601171003), Firefox 40.0a2 (2015-06-03) and Firefox 41.0a1 (2015-06-03) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.

Marking tracking flag as verified according with my testing since the other issues are filed separately.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
You need to log in before you can comment on or make changes to this bug.