Remove use of innerHTML from aboutTelemetry.js

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: chutten, Assigned: prathiksha, Mentored)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 attachment)

toolkit/content/aboutTelemetry.js has two uses of innerHTML, against our eslint policy. We should really get rid of those in favour of building our DOM nodes manually.

Both innerHTML uses follow the pattern of:
1) put this HTML string inside a localized string via bundle.formatStringFromName
2) put that whole string into the DOM via innerHTML.

We now have a nice API to handle this without innerHTML, called BrowserUtils.getLocalizedFragment. It does the same thing, but takes nodes instead of an HTML string and results in a document fragment at the end of step 1. If we use that we can then use appendChild for step 2 instead of innerHTML.
I will take this up. Please assign it to me.
Assignee: nobody → venkateshprabhu2
Status: NEW → ASSIGNED
How are things coming along with this bug? Anything I can help with?
Flags: needinfo?(venkateshprabhu2)
(In reply to Chris H-C :chutten from comment #2)
> How are things coming along with this bug? Anything I can help with?

Hi Chris,

Sorry, I totally forgot about this. Give me a day or two to work on it. I will probably be needing some help. Thank you.
Flags: needinfo?(venkateshprabhu2)
Assignee: venkateshprabhu2 → nobody
Status: ASSIGNED → NEW
Assignee: nobody → aakanksha.jain8
(In reply to Chris H-C :chutten from comment #2)
> How are things coming along with this bug? Anything I can help with?

Hi Chris,

    let explanation = bundle.formatStringFromName("settingsExplanation", parameters, 2);

    // eslint-disable-next-line no-unsanitized/property
    settingsExplanation.innerHTML = explanation;
    this.attachObservers();
  },

  convertStringToLink(string) {
    return "<a href=\"#\" class=\"change-data-choices-link\">" + string + "</a>";
  },


So, according to the steps you have mentioned, I have to rewrite converStringToLink to return a DOM element instead of the HTML string and then replace bundle.formatStringFromName with the api call to BrowserUtils.getLocalizedFragment. 

Is that correct? 

(Thought I could assign it to myself once I submit the patch. I wasn't sure if I could fix this. So, removed myself. )
(In reply to Venkatesh Prabhu :vprabhu from comment #4)
> (In reply to Chris H-C :chutten from comment #2)
> > How are things coming along with this bug? Anything I can help with?
> 
> Hi Chris,
> 
>     let explanation = bundle.formatStringFromName("settingsExplanation",
> parameters, 2);
> 
>     // eslint-disable-next-line no-unsanitized/property
>     settingsExplanation.innerHTML = explanation;
>     this.attachObservers();
>   },
> 
>   convertStringToLink(string) {
>     return "<a href=\"#\" class=\"change-data-choices-link\">" + string +
> "</a>";
>   },
> 
> 
> So, according to the steps you have mentioned, I have to rewrite
> converStringToLink to return a DOM element instead of the HTML string and
> then replace bundle.formatStringFromName with the api call to
> BrowserUtils.getLocalizedFragment. 
> 
> Is that correct? 
> 
> (Thought I could assign it to myself once I submit the patch. I wasn't sure
> if I could fix this. So, removed myself. )

Are you working on this?
(In reply to Aakanksha [:accakks] from comment #5)
> (In reply to Venkatesh Prabhu :vprabhu from comment #4)
> > (In reply to Chris H-C :chutten from comment #2)
> > > How are things coming along with this bug? Anything I can help with?
> > 
> > Hi Chris,
> > 
> >     let explanation = bundle.formatStringFromName("settingsExplanation",
> > parameters, 2);
> > 
> >     // eslint-disable-next-line no-unsanitized/property
> >     settingsExplanation.innerHTML = explanation;
> >     this.attachObservers();
> >   },
> > 
> >   convertStringToLink(string) {
> >     return "<a href=\"#\" class=\"change-data-choices-link\">" + string +
> > "</a>";
> >   },
> > 
> > 
> > So, according to the steps you have mentioned, I have to rewrite
> > converStringToLink to return a DOM element instead of the HTML string and
> > then replace bundle.formatStringFromName with the api call to
> > BrowserUtils.getLocalizedFragment. 
> > 
> > Is that correct? 
> > 
> > (Thought I could assign it to myself once I submit the patch. I wasn't sure
> > if I could fix this. So, removed myself. )
> 
> Are you working on this?

If you are interested, please go ahead. I didn't want to leave it in the middle. I will follow this to see the fix even if you fix it.
(In reply to Venkatesh Prabhu :vprabhu from comment #4)
> (In reply to Chris H-C :chutten from comment #2)
> > How are things coming along with this bug? Anything I can help with?
> 
> Hi Chris,
> 
>     let explanation = bundle.formatStringFromName("settingsExplanation",
> parameters, 2);
> 
>     // eslint-disable-next-line no-unsanitized/property
>     settingsExplanation.innerHTML = explanation;
>     this.attachObservers();
>   },
> 
>   convertStringToLink(string) {
>     return "<a href=\"#\" class=\"change-data-choices-link\">" + string +
> "</a>";
>   },
> 
> 
> So, according to the steps you have mentioned, I have to rewrite
> converStringToLink to return a DOM element instead of the HTML string and
> then replace bundle.formatStringFromName with the api call to
> BrowserUtils.getLocalizedFragment. 
> 
> Is that correct? 

This is correct.

---

In Bugzilla, unassigning yourself from a bug is a sign that you're no longer working on it. Assigning yourself is a sign that you will begin working on it. Near as I can tell this isn't mentioned in bugzilla.mo proper, but it might be mentioned in some of the documentation and etiquette guides we have on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Bugzilla

I'll let the two of you figure out who wants to work on this bug. For the other, may I suggest bug 1430531 which involves C++ and locking (it has a partial solution that might be helpful)? Or bug 1441904 which is in JS and involves testing Android (you probably don't need an Android device for it, but you might benefit from having Tryserver access so you can test on try's android devices without having to ask me to do it for you)?
Hi, I'd like to give it a try. If I'm not able to fix this, I'd inform Venkatesh. :)
(In reply to Chris H-C :chutten from comment #0)
> toolkit/content/aboutTelemetry.js has two uses of innerHTML, against our
> eslint policy. We should really get rid of those in favour of building our
> DOM nodes manually.
> 
> Both innerHTML uses follow the pattern of:
> 1) put this HTML string inside a localized string via
> bundle.formatStringFromName
> 2) put that whole string into the DOM via innerHTML.
> 
> We now have a nice API to handle this without innerHTML, called
> BrowserUtils.getLocalizedFragment. It does the same thing, but takes nodes
> instead of an HTML string and results in a document fragment at the end of
> step 1. If we use that we can then use appendChild for step 2 instead of
> innerHTML.

Hi, how do I test the changes I made are correct, before submitting for review?
Flags: needinfo?(chutten)
(In reply to Aakanksha [:accakks] from comment #9)
> (In reply to Chris H-C :chutten from comment #0)
> > toolkit/content/aboutTelemetry.js has two uses of innerHTML, against our
> > eslint policy. We should really get rid of those in favour of building our
> > DOM nodes manually.
> > 
> > Both innerHTML uses follow the pattern of:
> > 1) put this HTML string inside a localized string via
> > bundle.formatStringFromName
> > 2) put that whole string into the DOM via innerHTML.
> > 
> > We now have a nice API to handle this without innerHTML, called
> > BrowserUtils.getLocalizedFragment. It does the same thing, but takes nodes
> > instead of an HTML string and results in a document fragment at the end of
> > step 1. If we use that we can then use appendChild for step 2 instead of
> > innerHTML.
> 
> Hi, how do I test the changes I made are correct, before submitting for
> review?

I'm using 1. DOMparser(), then BrowserUtils.getLocalizedFragment, and then at last appendChild.
Flags: needinfo?(prathikshaprasadsuman)
The testing for this is manual. Load about:telemetry in the Firefox you built and make sure that it looks like the about:telemetry in the Firefox you have installed. If everything went according to plan, they'll look identical both in appearance and, if you open up the inspector, in document structure.
Flags: needinfo?(chutten)
Flags: needinfo?(prathikshaprasadsuman)
Have you successfully tried it out? Are you stuck?
Flags: needinfo?(aakanksha.jain8)
Hi, as mentioned, I tried it using DOMparser() method, but it returns a reference error on using "DOMparser()". 
Can you suggest whether  DOMparser can be used and there might be an error in my implementation or I should try some other method to convert HTML string to nodes?
Flags: needinfo?(aakanksha.jain8) → needinfo?(chutten)
Without seeing the code I don't think I'll be able to help you out too much.

To me I don't think parsing is needed. document.createElement ought to be all you need to create the anchor element, no?
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #14)
> Without seeing the code I don't think I'll be able to help you out too much.
> 
> To me I don't think parsing is needed. document.createElement ought to be
> all you need to create the anchor element, no?

Okay, noted that. 


I'm getting reference error for BrowserUtils
Ah, that might be because you may need to include BrowserUtils in aboutTelemetry.js

You can import it like so:

 ChromeUtils.import("resource://gre/modules/BrowserUtils.jsm");
Hi Aakanksha, are you still working on this? :)
Flags: needinfo?(aakanksha.jain8)
Hi, really sorry for delay. 
I'm working in this & will complete within 2-3 days!
Flags: needinfo?(aakanksha.jain8)
Assignee: aakanksha.jain8 → nobody
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment on attachment 8970577 [details]
Bug 1436047 - Remove innerHTML from aboutTelemetry.js.

https://reviewboard.mozilla.org/r/239322/#review245672

This looks good, thank you!

::: toolkit/content/aboutTelemetry.js:310
(Diff revision 1)
>      let pings = bundle.GetStringFromName("pingExplanationLink");
> -    let pingLink = "<a href=\"https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/concepts/pings.html\">" + pings + "</a>";
> +    let pingLink = document.createElement("a");
> +    pingLink.setAttribute("href", "https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/concepts/pings.html");
> +    pingLink.textContent = pings;
>      let pingName = this._getSelectedPingName();
> +    let pingNameHtml = document.createElement("span");

nit: I don't think pingNameHTML is still the right name for this. pingNameSpan?

::: toolkit/content/aboutTelemetry.js:317
(Diff revision 1)
>  
>      // Display the type and controls if the ping is not current
>      let pingDate = document.getElementById("ping-date");
>      let pingType = document.getElementById("ping-type");
>      let controls = document.getElementById("controls");
> -    let explanation;
> +    let explanation, fragment;

Why does explanation need to be defined outside of the if condition?
Attachment #8970577 - Flags: review?(jhofmann) → review+
Comment on attachment 8970577 [details]
Bug 1436047 - Remove innerHTML from aboutTelemetry.js.

https://reviewboard.mozilla.org/r/239322/#review245926
Attachment #8970577 - Flags: review?(chutten)
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Pushed by prathikshaprasadsuman@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c074adee208b
Remove innerHTML from aboutTelemetry.js. r=johannh
Flags: needinfo?(prathikshaprasadsuman)
https://hg.mozilla.org/mozilla-central/rev/c074adee208b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.