1373642 - Theme name is not bolded in installation pop-up

Status: RESOLVED DUPLICATE of bug 1431242

(WebExtensions :: Frontend, defect, P5)

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: 2017-06-16_1618.png

[Affected versions]:
Firefox 56.0a1 (2017-06-15)
Firefox 55.0b2 (20170615063713)
 
[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit
 
[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Navigate to https://addons.allizom.org/en-US/firefox/addon/little-flowers/
3.Click on “+ Add to Firefox” green button.
 
[Expected Results]:
Theme name is bolded.
 
 
[Actual Results]:
- Theme name is not not bolded just like in installation confirmation doorhanger: https://www.screencast.com/t/jJKVmScbsQ
- See attached screenshot.

Comment 1

[Andy McKay]

I can't get this to occur on addons.mozilla.org production site, so dropping priority based on that.

Comment 2

[Mark Striemer [:mstriemer]]

It looks bold in the screenshot and I see it bolded from addons.mozilla.org on Nightly.

Am I missing something? This looks invalid to me.

Comment 3

[Andy McKay]

Try it on addons.allizom.org, the link in comment 0.

Comment 4

[Mark Striemer [:mstriemer]]

Ah, there were two screenshots. Oops.

So it's the prompt to give permissions to install add-ons.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Reassigning since there hasn't been any recent activity.

Comment 6

[Mark Striemer [:mstriemer]]

There were some changes in bug 1369482 recently that should make this easier to implement. Take not that in its current state it would introduce an unwanted space before the question mark, however.

Comment 7

[Connor Masini]

Attachment: Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8945024 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

https://reviewboard.mozilla.org/r/215226/#review220774


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_bug553455.js:781
(Diff revision 1)
>    is(installs.length, 1, "Should be one pending install");
>    installs[0].cancel();
>  
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await removeTab();
> +  Assert.equals(document.getElementById("addon-webext-name"), null)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/general/browser_bug553455.js:822
(Diff revision 1)
>      AddonManager.getAddonByID("[e-mail]", function(result) {
>        resolve(result);
>      });
>    });
>    isnot(addon, null, "Test theme will have been installed");
> -  addon.uninstall();
> +  addon.uninstall();//addon-webext-perm-header

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 9

[Connor Masini]

Attachment: Bug 1373642: Theme name is not bolded in installation pop-up

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

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8945024 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

https://reviewboard.mozilla.org/r/215226/#review220782


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_bug553455.js:781
(Diff revision 1)
>    is(installs.length, 1, "Should be one pending install");
>    installs[0].cancel();
>  
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await removeTab();
> +  Assert.equals(document.getElementById("addon-webext-name"), null)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/general/browser_bug553455.js:822
(Diff revision 1)
>      AddonManager.getAddonByID("[e-mail]", function(result) {
>        resolve(result);
>      });
>    });
>    isnot(addon, null, "Test theme will have been installed");
> -  addon.uninstall();
> +  addon.uninstall();//addon-webext-perm-header

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8945024 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

https://reviewboard.mozilla.org/r/215226/#review220792

Nice, thanks!

::: browser/base/content/browser-addons.js:648
(Diff revision 1)
>        this._install(data, notify);
>        return;
>      }
>  
>      let strings = {
> -      header: gNavigatorBundle.getFormattedString("webextPerms.header", [data.name]),
> +      header: gNavigatorBundle.getFormattedString("webextPerms.header", [`<span class="addon-webext-name">${data.name}</span>`]),

Can you move `<span class="addon-webext-name">${data.name}</span>` in a separate addonName variable like it's done in other places ?

https://dxr.mozilla.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#252

::: browser/base/content/test/general/browser_bug380960.js
(Diff revision 1)
>    tab = BrowserTestUtils.addTab(gBrowser, "about:blank", { skipAnimation: true });
>    gBrowser.removeTab(tab, { animate: true });
>    gBrowser.removeTab(tab);
>    is(tab.parentNode, null, "tab removed immediately when calling removeTab again after the animation was kicked off");
>  }
> -

nit: please undo this change

::: browser/base/content/test/general/browser_bug553455.js:781
(Diff revision 1)
>    is(installs.length, 1, "Should be one pending install");
>    installs[0].cancel();
>  
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await removeTab();
> +  Assert.equals(document.getElementById("addon-webext-name"), null)

Use `ok` instead of Assert.equals

ok(document.getElementById("addon-webext-name"), "Add-on name should be wrapped in a bold label");

::: browser/base/content/test/general/browser_bug553455.js:822
(Diff revision 1)
>      AddonManager.getAddonByID("[e-mail]", function(result) {
>        resolve(result);
>      });
>    });
>    isnot(addon, null, "Test theme will have been installed");
> -  addon.uninstall();
> +  addon.uninstall();//addon-webext-perm-header

Why did you add this comment ?

::: browser/locales/en-US/chrome/browser/browser.properties:37
(Diff revision 1)
>  xpinstallDisabledMessage=Software installation is currently disabled. Click Enable and try again.
>  xpinstallDisabledButton=Enable
>  xpinstallDisabledButton.accesskey=n
>  
>  # LOCALIZATION NOTE (webextPerms.header)
> -# This string is used as a header in the webextension permissions dialog,
> +# This string is https://bug1308309.bmoattachments.org/attachment.cgi?id=8814612used as a header in the webextension permissions dialog,

nit: please undo this change

::: moz.configure:17
(Diff revision 1)
>  # - Fennec-specific options and rules should go in
>  #   mobile/android/moz.configure.
>  # - Spidermonkey-specific options and rules should go in js/moz.configure.
>  # - etc.
>  
> +

Please also undo this one :)

Comment 12

[Connor Masini]

Attachment: Bug 1373642: Theme name is not bolded in installation pop-up - Review Fixes

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

Comment 13

[Connor Masini]

I found a good test case to add to in order to test whether or not the theme name was bolded and removed the previous assert statement completely.  All other recommendations should be fixed.  Thanks for the feedback!

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8945024 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

https://reviewboard.mozilla.org/r/215226/#review220926


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_bug553455.js:781
(Diff revision 1)
>    is(installs.length, 1, "Should be one pending install");
>    installs[0].cancel();
>  
>    Services.perms.remove(makeURI("http://example.com/"), "install");
>    await removeTab();
> +  Assert.equals(document.getElementById("addon-webext-name"), null)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/general/browser_bug553455.js:822
(Diff revision 1)
>      AddonManager.getAddonByID("[e-mail]", function(result) {
>        resolve(result);
>      });
>    });
>    isnot(addon, null, "Test theme will have been installed");
> -  addon.uninstall();
> +  addon.uninstall();//addon-webext-perm-header

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 15

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

Thanks for the patches, Connor! This is in my queue, and I'll have a response shortly.

In the meantime, I'm going to have you work on bug 1418605. I'll let you know once I have feedback on this.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8945117 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Review Fixes

Forwarding review to mconley who is taking this one when he gets time.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8945024 [details]
Bug 1373642: Theme name is not bolded in installation pop-up - Fixed

Clearing review on this since mconley will be taking it.

Comment 18

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

Hm. If I'm reading this correctly, I suspect that bug 1431242 is going to independently fix this... prathiksha, can you confirm?

Comment 19

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

I've cleared the review requests until the patches are folded. I also do want to hear back from prathiksha before I review this to see if we're about to kill two birds with one stone.

Comment 20

[:prathiksha]

Hi Mike, I'd be ok with this issue getting resolved here seeing as it's Connor's first patch but I think we should leave it off to Bug 1431242 as it uses popupnotification's built-in bolding which was put in place to fix this issue and similar ones.

If Connor would like to explore more and come up with a patch that uses popupnotification's built-in bolding to fix this, I wouldn't mind excluding this part (the theme installation popup) from Bug 1431242 but at the same time, I wouldn't call that exercise a good-first-bug. :)

Comment 21

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

(In reply to :prathiksha from comment #20)
> If Connor would like to explore more and come up with a patch that uses
> popupnotification's built-in bolding to fix this, I wouldn't mind excluding
> this part (the theme installation popup) from Bug 1431242 but at the same
> time, I wouldn't call that exercise a good-first-bug. :)

Okay, thanks prathiksha. There's plenty of other work we can have Connor doing, so I think we're okay here.

Hey Connor,

So what happened here was a minor collision - another engineer was working on a bug, which (when fixed), will also fix your bug. It's not exactly duplicate work, but it's in the ballpark.

prathiksha's solution solves the bolding case for _all_ WebExtension permission dialogs, instead of spot-fixing just one of them. I think we should go that route.

Sorry about that! I know it's kind of disappointing for a bug to turn out this way, but there's plenty of other bugs for you to work on, so many chances for you to get a patch landed. :)

So for now, I'm going to unassign Connor from this bug, and mark it as blocked by bug 1431242.

Comment 22

[Kris Maglione [:kmag]]

We may as well just call this a dupe at this point. It might be worth making sure that this prompt is in the STR for QA in bug 1431242, though.