Closed
Bug 1436047
Opened 6 years ago
Closed 6 years ago
Remove use of innerHTML from aboutTelemetry.js
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: chutten, Assigned: prathiksha, Mentored)
References
Details
(Whiteboard: [good next bug][lang=js])
Attachments
(1 file)
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.
Comment 1•6 years ago
|
||
I will take this up. Please assign it to me.
Updated•6 years ago
|
Assignee: nobody → venkateshprabhu2
Updated•6 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•6 years ago
|
||
How are things coming along with this bug? Anything I can help with?
Flags: needinfo?(venkateshprabhu2)
Comment 3•6 years ago
|
||
(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)
Updated•6 years ago
|
Assignee: venkateshprabhu2 → nobody
Status: ASSIGNED → NEW
Updated•6 years ago
|
Assignee: nobody → aakanksha.jain8
Comment 4•6 years ago
|
||
(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. )
Comment 5•6 years ago
|
||
(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?
Comment 6•6 years ago
|
||
(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.
Reporter | ||
Comment 7•6 years ago
|
||
(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)?
Comment 8•6 years ago
|
||
Hi, I'd like to give it a try. If I'm not able to fix this, I'd inform Venkatesh. :)
Comment 9•6 years ago
|
||
(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)
Comment 10•6 years ago
|
||
(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.
Updated•6 years ago
|
Flags: needinfo?(prathikshaprasadsuman)
Reporter | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(prathikshaprasadsuman)
Reporter | ||
Comment 12•6 years ago
|
||
Have you successfully tried it out? Are you stuck?
Flags: needinfo?(aakanksha.jain8)
Comment 13•6 years ago
|
||
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)
Reporter | ||
Comment 14•6 years ago
|
||
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)
Comment 15•6 years ago
|
||
(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
Reporter | ||
Comment 16•6 years ago
|
||
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");
Comment 17•6 years ago
|
||
Hi Aakanksha, are you still working on this? :)
Flags: needinfo?(aakanksha.jain8)
Comment 18•6 years ago
|
||
Hi, really sorry for delay. I'm working in this & will complete within 2-3 days!
Flags: needinfo?(aakanksha.jain8)
Updated•6 years ago
|
Assignee: aakanksha.jain8 → nobody
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 21•6 years ago
|
||
mozreview-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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 23•6 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Comment 24•6 years ago
|
||
Pushed by prathikshaprasadsuman@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c074adee208b Remove innerHTML from aboutTelemetry.js. r=johannh
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(prathikshaprasadsuman)
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c074adee208b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•