Closed Bug 1431242 Opened 6 years ago Closed 6 years ago

Use popupnotification's built-in bolding for webextension prompts

Categories

(WebExtensions :: Frontend, defect, P3)

defect

Tracking

(firefox59+ verified, firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox59 + verified
firefox60 --- verified

People

(Reporter: mstriemer, Assigned: prathiksha)

References

Details

Attachments

(1 file)

In bug 1369482 popupnotification got support for a host portion in the middle to be bolded.

We don't set the host in our install dialogs but instead bold the extension name. This is a nicer solution than setting the innerHTML directly so we should move to the new format.

This will unfortunately require a string change but will remove our use of innerHTML.

You can see an example of this in ExtensionsUI.jsm [1].

[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/browser/modules/ExtensionsUI.jsm#285
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Summary: Use popupnotification's built-in bolding for notifications → Use popupnotification's built-in bolding for webextension prompts
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review222946

This looks really good already, I can't wait for the final version. You should probably flag aswan for additional review once you're ready :)

THANK YOU!

::: browser/base/content/popup-notifications.inc:83
(Diff revision 2)
>          <label id="addon-webext-perm-intro" class="addon-webext-perm-text"/>
>          <html:ul id="addon-webext-perm-list" class="addon-webext-perm-list"/>
>        </popupnotificationcontent>
>      </popupnotification>
>  
>      <popupnotification id="addon-webext-defaultsearch-notification" hidden="true">

Ah, I now understand what you asked me yesterday. Yes, I think this entire popupnotification element can be removed.

::: browser/modules/ExtensionsUI.jsm:295
(Diff revision 2)
> -        doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> +        let header = strings.header;
> +        header = header.split("<>");
> +        message.start = header[0];
> +        // Use the host element to display addon name in addon permission prompts.
> +        message.host = strings.addonName;
> +        message.end = header[1];

It's slightly weird to populate the message contents inside the eventCallback. I guess the popup content is filled after the "showing" event, but that could change. Is there any reason why we can't populate message at the top?

::: browser/modules/ExtensionsUI.jsm:299
(Diff revision 2)
> +        message.host = strings.addonName;
> +        message.end = header[1];
> +
>          let textEl = doc.getElementById("addon-webext-perm-text");
>          // eslint-disable-next-line no-unsanitized/property
>          textEl.innerHTML = strings.text;

This one is still TODO, though I guess you're already tracking that :)

::: browser/modules/ExtensionsUI.jsm:362
(Diff revision 2)
>                                    action, secondaryActions, popupOptions);
>      });
>    },
>  
>    showDefaultSearchPrompt(browser, strings, icon) {
> +    let message = {};

Nit: why not define message inside the promise?

::: browser/modules/ExtensionsUI.jsm:378
(Diff revision 2)
> -            // eslint-disable-next-line no-unsanitized/property
> -            doc.getElementById("addon-webext-defaultsearch-text").innerHTML = strings.text;
> +            let header = strings.text;
> +            header = header.split("<>");
> +            message.start = header[0];
> +            // Use the host element to display addon name in addon notification prompts.
> +            message.host = strings.addonName;
> +            message.end = header[1];

See above, I'd say this can move outside the eventCallback :)

::: browser/modules/ExtensionsUI.jsm:390
(Diff revision 2)
>        let action = {
>          label: strings.acceptText,
>          accessKey: strings.acceptKey,
>          disableHighlight: true,
>          callback: () => {
>  //          this.histogram.add(kDefaultSearchHistKey + "Accepted");

What's that dead code doing here? :(

You can either remove it in this bug, if you like, or we should add a good first bug for removing it...

::: browser/modules/ExtensionsUI.jsm:455
(Diff revision 2)
> -        // eslint-disable-next-line no-unsanitized/property
> +            // eslint-disable-next-line no-unsanitized/property
> -            doc.getElementById("addon-installed-notification-header")
> -               .unsafeSetInnerHTML(msg1);
> -            // eslint-disable-next-line no-unsanitized/property
>              doc.getElementById("addon-installed-notification-message")
>                 .unsafeSetInnerHTML(msg2);

Would you be up for solving these images in this patch or a separate patch in this bug (ie. maybe not in a separate bug)?
Attachment #8945111 - Flags: review?(jhofmann)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

feedback++
Attachment #8945111 - Flags: feedback+
Attachment #8945111 - Flags: review?(aswan)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223104

Thank you for doing this, this is a huge step in the right direction!  I'd like to handle updates consistently with the other types of prompts though, which I think should make things simpler overall.

::: browser/modules/ExtensionsUI.jsm:286
(Diff revision 3)
> +    let message = {};
> +    // Create the notification header element.
> +    let header = strings.header;
> +    header = header.split("<>");
> +    message.start = header[0];
> +    // Use the host element to display addon name in addon permission prompts.
> +    message.host = strings.addonName;
> +    message.end = header[1];

We have 3 copies of basically the same code here, can it move into a helper function?

::: browser/modules/ExtensionsUI.jsm:300
(Diff revision 3)
>        if (topic == "showing") {
> -        // eslint-disable-next-line no-unsanitized/property
> -        doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
>          let textEl = doc.getElementById("addon-webext-perm-text");
>          // eslint-disable-next-line no-unsanitized/property
>          textEl.innerHTML = strings.text;

Can we cover this instance too?  I think it would simplify some of the special handling of different types (eg update) at the same time.
Attachment #8945111 - Flags: review?(aswan) → review-
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223180

::: browser/modules/ExtensionsUI.jsm:368
(Diff revision 3)
> +    // Create the notification header element.
> +    let header = strings.text;
> +    header = header.split("<>");
> +    message.start = header[0];
> +    // Use the host element to display addon name in addon notification prompts.
> +    message.host = strings.addonName;

Nit: Can we rename this from `host` to something like `name` (in a separate patch) please? In most of its uses, now, it's not a host anymore, and keeping that naming is just confusing.
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223104

> We have 3 copies of basically the same code here, can it move into a helper function?

There are copies of the same code in PermissionsUI.jsm and webrtcUI.jsm also. I'll fix it in Bug 1435160.

> Can we cover this instance too?  I think it would simplify some of the special handling of different types (eg update) at the same time.

This instance and other similar instances in ExtensionsUI.jsm are being tracked in Bug 1434976. I'll be pushing a patch there soon. This patch only makes the addon names bold in webextension prompts.
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223180

> Nit: Can we rename this from `host` to something like `name` (in a separate patch) please? In most of its uses, now, it's not a host anymore, and keeping that naming is just confusing.

Yes, we can change that. It does sound confusing now.
Attachment #8945111 - Flags: review?(aswan)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223496

::: browser/locales/en-US/chrome/browser/browser.dtd:999
(Diff revision 5)
>       display a tooltip for accessibility indicator in toolbar/tabbar. It is also
>       used as a textual label for the indicator used by assistive technology
>       users. -->
>  <!ENTITY accessibilityIndicator.tooltip "Accessibility Features Enabled">
> +
> +<!ENTITY addonPostInstallMessage.label "Manage your add-ons by clicking <image id='addon-addon-icon'/> in the <image id='addon-toolbar-icon'/> menu.">

This looks like the best solution to me, but it would mean we could not uplift the patch to beta. In this case solving this separately (the way that you're doing it here, just in a different bug or patch) might indeed be a better option.
Attachment #8945111 - Flags: review?(jhofmann)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223498

::: browser/modules/ExtensionsUI.jsm:297
(Diff revision 5)
> +    message.host = strings.addonName;
> +    message.end = header[1];
> +
> +    let doc = browser.ownerDocument;
> -        let textEl = doc.getElementById("addon-webext-perm-text");
> +    let textEl = doc.getElementById("addon-webext-perm-text");
> -        // eslint-disable-next-line no-unsanitized/property
> +    textEl.value = strings.text;

This only works on my machine if you use textEl.setAttribute("value", ...)

It would be easy to have tests for that, maybe in a follow-up...
Attachment #8945111 - Flags: review?(aswan)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review223902

::: browser/modules/ExtensionsUI.jsm:276
(Diff revision 6)
>  
>      let brandBundle = Services.strings.createBundle(BRAND_PROPERTIES);
>      let appName = brandBundle.GetStringFromName("brandShortName");
> +    let info2;
> +    if (info.type == "update") {
> -    let addonName = `<span class="addon-webext-name">${this._sanitizeName(info.addon.name)}</span>`;
> +      let addonName = `<span class="addon-webext-name">${this._sanitizeName(info.addon.name)}</span>`;

If I'm not mistaken the update prompt just ends up calling showPermissionPrompt() here:

https://searchfox.org/mozilla-central/source/browser/modules/ExtensionsUI.jsm#111

and it formats addonName here:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1004

So I don't think you should keep on using HTML in this case.

It also looks like we need to fix up all the addonName usages in ExtensionData.formatPermissionStrings to use info.addon.name instead of relying on callers to set info.addonName.

info.addonName should be equal to info.addon.name at all times now, right? I think it would be beneficial to completely remove the former then.

What do you think?
(In reply to Johann Hofmann [:johannh] from comment #18)

> info.addonName should be equal to info.addon.name at all times now, right? I
> think it would be beneficial to completely remove the former then.
> 
> What do you think?

Yes, they should be equal. Thanks. I'll update the patch. :)
(In reply to Johann Hofmann [:johannh] from comment #18)

> It also looks like we need to fix up all the addonName usages in
> ExtensionData.formatPermissionStrings to use info.addon.name instead of
> relying on callers to set info.addonName.

I have fixed all the addonName usages in ExtensionData.formatPermissionStrings in this version of my patch but I think we should rely on callers to set info.addonName. It looks like setting info.addon.name in the formatPermissionStrings function means we can't split the string into three parts(start, name, endlabel) after fetching it.


Tagging :aswan for review again. Sorry for the confusion I caused in the second part of Comment 11. It took me a while to understand how the update prompt is called. This version of my patch should fix the handling of the update and the sideload prompts. Thanks. :)
Attachment #8945111 - Flags: review?(aswan)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review224012

We're almost there! I don't think this needs any more major changes, just a couple of notes:

::: browser/modules/ExtensionsUI.jsm:130
(Diff revision 7)
>      let strings = this._buildStrings({
> -      addon,
>        permissions: addon.userPermissions,
>        type: "sideload",
>      });
> +    strings.addonName = addon.name;

Is there ever a case where you call this._buildStrings without setting strings.addonName = addon.name afterwards? If not, you should probably just move it into the _buildStrings function.

::: browser/modules/ExtensionsUI.jsm:263
(Diff revision 7)
>      }
>    },
>  
>    // Escape &, <, and > characters in a string so that it may be
>    // injected as part of raw markup.
>    _sanitizeName(name) {

You can remove this function now.

::: browser/modules/ExtensionsUI.jsm:290
(Diff revision 7)
> -        doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> +    message.start = header[0];
> +    // Use the host element to display addon name in addon permission prompts.
> +    message.host = strings.addonName;
> +    message.end = header[1];
> +
> +    let doc = browser.ownerDocument;

Hmmm, are you sure that you can just entirely remove the "showing" event in this case?

What I suggested previously is to just construct the message object outside of the event callback but still populate the custom DOM nodes inside the eventCallback.

I'm pretty sure they do it on "showing" for a reason, aswan might know more.
Attachment #8945111 - Flags: review?(jhofmann) → review-
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review224076

This is getting close, thank you!
I agree with Johann's comments.  In addition, the update prompts still need a little more work.  Those prompts currently use the `text` property in the strings object instead of `header` like everything else.  But those prompts don't have a separate header, so I think that just moving that string to `result.header` in `Extension.formatPermissionStrings()` should do the trick (though I haven't looked closely at that code to see if there's some other problem with that, I can take a closer look tomorrow).
Also, the `<description>` being used for the header isn't wrapping which leads to a very wide popup for the update, even without the name present due to the above issue!

::: browser/modules/ExtensionsUI.jsm:290
(Diff revision 7)
> -        doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> +    message.start = header[0];
> +    // Use the host element to display addon name in addon permission prompts.
> +    message.host = strings.addonName;
> +    message.end = header[1];
> +
> +    let doc = browser.ownerDocument;

I'm actually not sure, I was just following the pattern that other users of PopupNotifications use.  Sorry for the redirect but perhaps florian or MattN can say if there's a reason not to do it this way?

::: toolkit/components/extensions/Extension.jsm:886
(Diff revision 7)
>     * @returns {object} An object with properties containing localized strings
>     *                   for various elements of a permission dialog.

Can you add a comment here noting that the `header` property (and maybe `text`?) in the returned object has the string `"<>"` as a placeholder for the addon name.
Attachment #8945111 - Flags: review?(aswan) → review-
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review225630

This looks good to me, thank you! Please make sure to do a try run with your latest patch and take a look at my comment below.

Thank you for your work!

::: toolkit/modules/PopupNotifications.jsm:782
(Diff revision 9)
> -        }
> -        if (n.message.host) {
> -          popupnotification.setAttribute("hostname", n.message.host);
> +        popupnotification.setAttribute("hostname", n.message.host);
> -        }
> -        if (n.message.end) {
> -          popupnotification.setAttribute("endlabel", n.message.end);
> +        popupnotification.setAttribute("endlabel", n.message.end);

I haven't tested this, but are you sure that passing null or undefined here actually sets the label to "" instead of stringifying to "null" or "undefined"?

It would be great if you could check this and pass n.message.end || "" instead, if necessary.
Attachment #8945111 - Flags: review?(jhofmann) → review+
> I haven't tested this, but are you sure that passing null or undefined here
> actually sets the label to "" instead of stringifying to "null" or
> "undefined"?
> 
> It would be great if you could check this and pass n.message.end || ""
> instead, if necessary.

We currently don't pass "null" or "undefined" anywhere but I changed the code anyway to avoid problems in the future.
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

https://reviewboard.mozilla.org/r/215334/#review225844

Nice, just one more nit, but r=me with that addressed.  Thank you very much for doing this work!

::: toolkit/components/extensions/Extension.jsm
(Diff revisions 7 - 10)
>          result.acceptKey = bundle.GetStringFromName("webextPerms.sideloadEnable.accessKey");
>          result.cancelKey = bundle.GetStringFromName("webextPerms.sideloadCancel.accessKey");
>        }
>      } else if (info.type == "update") {
> -      result.header = "";
> +      result.header = bundle.formatStringFromName("webextPerms.updateText", ["<>"], 1);
> -      result.text = bundle.formatStringFromName("webextPerms.updateText", ["<>"], 1);

Please include `result.text = "";` here
Attachment #8945111 - Flags: review?(aswan) → review+
Keywords: checkin-needed
We will need QA verification that all the WebExtension installation and permission popups still work as expected.
Flags: qe-verify+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47453e1bdac1
Use popupnotification's built-in bolding for webextension prompts. r=aswan,johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47453e1bdac1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Awesome!! Thank you so much, Prathiksha! Your contribution has been added to the add-ons contributor recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#February_2018

\o/
We perform a series of tests on Latest Nightly (2018-02-15). We didn't find any performance or UI issues. You can see the tests that we ran and the results by accessing this link: https://docs.google.com/document/d/1lDarNVXEvwqczxbhXD45f70gPVsqwAUSwKwp7CYjfDA/edit#heading=h.x34pnbva4z62.
Flags: qe-verify+
Can you request uplift to beta? Thanks!
Flags: needinfo?(prathikshaprasadsuman)
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1432966
[User impact if declined]: Bug 1432966 broke bold text in some add-on prompts. This patch restores it.
[Is this code covered by automated tests?]: Most of the prompts, yes.
[Has the fix been verified in Nightly?]: Yes, see comment 36.
[Needs manual test from QE? If yes, steps to reproduce]: Not beyond what already happened.
[List of other uplifts needed for the feature/fix]: None AFAIK
[Is the change risky?]: Medium risk, but under control.
[Why is the change risky/not risky?]: Without proper testing this might have broken some text in add-on installation prompts, but I'm fairly certain QA covered all cases.
[String changes made/needed]: None
Flags: needinfo?(prathikshaprasadsuman)
Attachment #8945111 - Flags: approval-mozilla-beta?
Comment on attachment 8945111 [details]
Bug 1431242 - Use popupnotification's built-in bolding for webextension prompts.

regression fix for extension prompts, beta59+
Attachment #8945111 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I verify the fix on beta 59.0b13 by performing a series of tests. I didn't find any performance or UI issues. You can see the tests and the results by accessing this link: https://docs.google.com/document/d/1lDarNVXEvwqczxbhXD45f70gPVsqwAUSwKwp7CYjfDA/
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.