Remove unsafeSetInnerHTML in browser_reservedkey.js

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
a year ago
8 days ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

a year ago
We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.js test file. While that in itself is not dangerous, we're aiming to remove unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the test.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/permissions/browser_reservedkey.js#13
(Assignee)

Updated

a year ago
Assignee: nobody → guptatrisha97
(Reporter)

Updated

a year ago
Mentor: jhofmann
Priority: -- → P3
(Assignee)

Comment 1

a year ago
(In reply to Johann Hofmann [:johannh] from comment #0)
> We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.js
> test file. While that in itself is not dangerous, we're aiming to remove
> unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the
> test.
> 
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/
> permissions/browser_reservedkey.js#13

Does this mean I will have to use the SetAttribute method to solve the issue? Can you give me a bit more background information on this, please? Thanks in advance :)
(Reporter)

Comment 2

a year ago
(In reply to Trisha from comment #1)
> (In reply to Johann Hofmann [:johannh] from comment #0)
> > We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.js
> > test file. While that in itself is not dangerous, we're aiming to remove
> > unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the
> > test.
> > 
> > https://searchfox.org/mozilla-central/rev/
> > 588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/
> > permissions/browser_reservedkey.js#13
> 
> Does this mean I will have to use the SetAttribute method to solve the
> issue? Can you give me a bit more background information on this, please?
> Thanks in advance :)

You need to construct the elements that are currently in the HTML string using document.createElement (https://developer.mozilla.org/en-US/docs/Web/API/Document/createElement), so <keyset> would become document.createElement("keyset") and so on. You probably need to use other DOM APIs such as appendChild https://developer.mozilla.org/en-US/docs/Web/API/Node/appendChild and setAttribute (https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute), too.

The point is that we can remove the unsafeSetInnerHTML function there, so we need to get rid of that HTML string :)
(Assignee)

Comment 3

a year ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
I have created a work-in-progress patch to receive some feedback from you. Can you please look at it and tell me if it looks okay?
Attachment #8962986 - Flags: feedback?(jhofmann)
(Reporter)

Comment 4

a year ago
Comment on attachment 8962986 [details] [diff] [review]
bug1444441.patch

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

This looks great, just a few things. I assume the test is still passing? :)

Thanks!

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +1,2 @@
>  add_task(async function test_reserved_shortcuts() {
>    /* eslint-disable no-unsanitized/property */

You should be able to remove this now.

@@ +6,5 @@
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "this.setAttribute("count", Number(this.getAttribute("count")) + 1));

Can you convert this to an addEventListener call?

key1.addEventListener("command", () => ... )

Please do the same for the other "oncommand" attributes below.

@@ +8,5 @@
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "this.setAttribute("count", Number(this.getAttribute("count")) + 1));
> +  keyset.appendChild(key1);
> +  key1.append("\n                  ");

I don't think it's important to add whitespace here. Can you remove this line and the other ones that add whitespace?

@@ +34,2 @@
>    document.documentElement.appendChild(container);
>    /* eslint-enable no-unsanitized/property */

Please remove this, too. I don't think these linter comments were actually needed...
Attachment #8962986 - Flags: feedback?(jhofmann) → feedback+
(Assignee)

Comment 5

a year ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
Attachment #8962986 - Attachment is obsolete: true
Attachment #8963508 - Flags: review?(jhofmann)
(Reporter)

Comment 6

a year ago
Comment on attachment 8963508 [details] [diff] [review]
bug1444441.patch

I'm a bit overloaded on reviews right now, Prathiksha, can you take a look at this?

Thanks!
Attachment #8963508 - Flags: review?(jhofmann) → review?(prathikshaprasadsuman)
(Assignee)

Comment 7

a year ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
Attachment #8963508 - Attachment is obsolete: true
Attachment #8963508 - Flags: review?(prathikshaprasadsuman)
Attachment #8964372 - Flags: review?(prathikshaprasadsuman)
(Assignee)

Comment 8

a year ago
Please let me know if this looks okay, thanks!
(Reporter)

Updated

a year ago
Status: NEW → ASSIGNED

Comment 9

a year ago
Comment on attachment 8964372 [details] [diff] [review]
bug1444441.patch

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

Hi Trisha,

Weren't you trying to submit this patch using mozReview? I know that you've already configured mercurial to use mozReview. You can submit your patch on mozReview by first selecting your changeset(hg up <changeset_ID>) and pushing it for review(hg push review). If you're still stuck, feel free to ping me or Johann on IRC.
Attachment #8964372 - Flags: review?(prathikshaprasadsuman)
(Assignee)

Comment 10

a year ago
(In reply to :prathiksha from comment #9)
> Comment on attachment 8964372 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8964372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Trisha,
> 
> Weren't you trying to submit this patch using mozReview? I know that you've
> already configured mercurial to use mozReview. You can submit your patch on
> mozReview by first selecting your changeset(hg up <changeset_ID>) and
> pushing it for review(hg push review). If you're still stuck, feel free to
> ping me or Johann on IRC.

I had few doubts which I think are best to be solved on IRC. I've left you a message! Thanks
Comment hidden (mozreview-request)
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review239388


Code analysis found 8 defects in this patch:
 - 8 defects found by mozlint

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


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


::: browser/base/content/test/permissions/browser_reservedkey.js:10
(Diff revision 1)
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:21
(Diff revision 1)
> +  key2.setAttribute("modifiers", "shift");
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:31
(Diff revision 1)
> +  key3.setAttribute("id", "kt_reserveddefault");
> +  key3.setAttribute("modifiers", "shift");
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:39
(Diff revision 1)
> +  key3.append("\n                ");
>    let container = document.createElement("box");
> -  container.unsafeSetInnerHTML(keyset);
> +  container.appendChild(keyset);
> +  keyset.append("\n                  ");
>    document.documentElement.appendChild(container);
> -  /* eslint-enable no-unsanitized/property */
> +         

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:21
(Diff revision 1)
>    let str3 = TOOLBOX_L10N.getStr("toolbox.defaultTitle");
>    ok(str1 && str2 && str3, "If this failed, strings should be updated in the test");
>  
>    info("Create the test markup");
>    let div = document.createElement("div");
> -  div.unsafeSetInnerHTML(
> +  div.setAttribute("data-localization-bundle", "devtools/client/locales/startup.properties");

Error: Line 21 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:33
(Diff revision 1)
> -     <div id="d3" data-localization="content=inspector.label;
> -                                     title=inspector.accesskey"></div>
> -     <div id="d4" data-localization="aria-label=inspector.label">Some content</div>
> -     <div data-localization-bundle="devtools/client/locales/toolbox.properties">
> -       <div id="d5" data-localization="content=toolbox.defaultTitle"></div>
> -     </div>
> +  d1.setAttribute("data-localization", "content=inspector.label");
> +  div.appendChild(d1);
> +  d1.append("Text will disappear");
> +  let d2 = document.createElement("div");
> +  d2.setAttribute("id", "d2");
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");

Error: Line 33 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:37
(Diff revision 1)
> -       <div id="d5" data-localization="content=toolbox.defaultTitle"></div>
> -     </div>
> -   </div>
> -  `);
> -
> +  d2.setAttribute("id", "d2");
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");
> +  div.appendChild(d2);
> +  let d3 = document.createElement("div");
> +  d3.setAttribute("id", "d3");
> +  d3.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");

Error: Line 37 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:45
(Diff revision 1)
> +  d4.setAttribute("id", "d4");
> +  d4.setAttribute("data-localization", "aria-label=inspector.label");
> +  div.appendChild(d4);
> +  d4.append("Some content");
> +  let div0 = document.createElement("div");
> +  div0.setAttribute("data-localization-bundle", "devtools/client/locales/toolbox.properties");

Error: Line 45 exceeds the maximum line length of 90. [eslint: max-len]
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8964372 - Attachment is obsolete: true

Comment 14

a year ago
mozreview-review
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review239622

Thank you for working on this!

I'm holding off on fully reviewing this patch for now. Please upload an updated patch with fixes after you run this mochitest locally. It's a good idea to always check that your changes to test file(s) are not breaking the tests. Here's how you can run a mochitest:

./mach mochitest <path_to_file/path_to_folder>

Feel free to ping me or Johann on IRC if you have any questions or if you get stuck. :)

::: browser/base/content/test/permissions/browser_reservedkey.js:11
(Diff revision 2)
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> -
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });

Please add a new line here

::: browser/base/content/test/permissions/browser_reservedkey.js:21
(Diff revision 2)
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });

Add a new line here also

::: browser/base/content/test/permissions/browser_reservedkey.js:31
(Diff revision 2)
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +  key2.appendChild(key3);

Please append key1, key2 and key3 to keyset here
Attachment #8965008 - Flags: review?(prathikshaprasadsuman) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 16

11 months ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
I have added a work-in-progress patch for feedback because I made the changes you asked me to in the previous review, but couldn't understand some stuff.

Firstly I was appending key1 to keyset as soon as I set its attributes, and same with key2, key3. But since your review said I should append all keys towards the end, I want to know which one is the preferred method? 

Also, I ran mochitest using both the above options yet got 3 failed tests. Can you give me a hint where I'm going wrong?

Thanks for your feedback on my work-in-progress patch in advance! :)
Attachment #8965008 - Attachment is obsolete: true
Attachment #8965008 - Flags: review?(prathikshaprasadsuman)
Attachment #8966831 - Flags: feedback?(prathikshaprasadsuman)

Comment 17

11 months ago
mozreview-review
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review241488

I assume the test is still failing. I recommend getting help on IRC to resolve it. Feel free to ping me or Johann for any questions you might have. Thanks for working on this!

::: browser/base/content/test/permissions/browser_reservedkey.js:13
(Diff revision 3)
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +
> +  keyset.appendChild(key1);

You're appending key1 to keyset twice in this patch (here and in line 35)

::: browser/base/content/test/permissions/browser_reservedkey.js:20
(Diff revision 3)
> +  key2.setAttribute("id", "kt_notreserved");
> +  key2.setAttribute("modifiers", "shift");
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {

This should be "command", not "oncommand"

::: browser/base/content/test/permissions/browser_reservedkey.js:24
(Diff revision 3)
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
>  
> +  key1.appendChild(key2);

Please remove this line

::: browser/base/content/test/permissions/browser_reservedkey.js:34
(Diff revision 3)
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +
> +  key2.appendChild(key3);

Please remove this line also
Attachment #8965008 - Flags: review-

Comment 18

11 months ago
(In reply to Trisha from comment #16)
> Created attachment 8966831 [details] [diff] [review]
> bug1444441.patch
> 
> I have added a work-in-progress patch for feedback because I made the
> changes you asked me to in the previous review, but couldn't understand some
> stuff.
> 
> Firstly I was appending key1 to keyset as soon as I set its attributes, and
> same with key2, key3. But since your review said I should append all keys
> towards the end, I want to know which one is the preferred method? 
> 
> Also, I ran mochitest using both the above options yet got 3 failed tests.
> Can you give me a hint where I'm going wrong?
> 
> Thanks for your feedback on my work-in-progress patch in advance! :)

I guess the point I was trying to make is that you have to append all keys to the parent keyset element directly.

Updated

11 months ago
Attachment #8966831 - Flags: feedback?(prathikshaprasadsuman) → feedback+
(Assignee)

Comment 19

11 months ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
I debugged my code and came down to one failed test now. I will be happy if you can help me get feedback on why this one case's failing. This is what it says: "FAIL reserved='true' with preference on - Got 1, expected 2" Thanks in advance!
Attachment #8966831 - Attachment is obsolete: true
Attachment #8967983 - Flags: feedback?(prathikshaprasadsuman)
(Reporter)

Comment 20

11 months ago
Comment on attachment 8967983 [details] [diff] [review]
bug1444441.patch

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

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +6,5 @@
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");
> +  let attri1 = key1.getAttribute("count");

Try getting this attribute (and the ones below) inside the event handler callback :)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

11 months ago
(In reply to Johann Hofmann [:johannh] from comment #20)
> Comment on attachment 8967983 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8967983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/permissions/browser_reservedkey.js
> @@ +6,5 @@
> > +  key1.setAttribute("key", "O");
> > +  key1.setAttribute("reserved", "true");
> > +  key1.setAttribute("count", "0");
> > +  key1.setAttribute("oncommand", "//");
> > +  let attri1 = key1.getAttribute("count");
> 
> Try getting this attribute (and the ones below) inside the event handler
> callback :)

Thanks! That worked. Submitted on MozReview for one final review.
(Assignee)

Updated

11 months ago
Attachment #8967983 - Attachment is obsolete: true
Attachment #8967983 - Flags: feedback?(prathikshaprasadsuman)

Comment 23

11 months ago
mozreview-review
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review242670

Looks good to me. Thank you for working on this. :)

::: browser/base/content/test/permissions/browser_reservedkey.js:11
(Diff revision 4)
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");
> +  key1.addEventListener("command", () => {
> +    let attri1 = key1.getAttribute("count");

I would just call this variable "attribute" :)

::: browser/base/content/test/permissions/browser_reservedkey.js:23
(Diff revision 4)
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.setAttribute("oncommand", "//");
> +  key2.addEventListener("command", () => {
> +    let attri2 = key2.getAttribute("count");

this one too

::: browser/base/content/test/permissions/browser_reservedkey.js:34
(Diff revision 4)
> +  key3.setAttribute("modifiers", "shift");
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.setAttribute("oncommand", "//");
> +  key3.addEventListener("command", () => {
> +    let attri3 = key3.getAttribute("count");

and this one!
Attachment #8965008 - Flags: review?(prathikshaprasadsuman) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8965008 - Attachment is obsolete: true
(Assignee)

Comment 25

11 months ago
Posted patch bug1444441.patch (obsolete) — Splinter Review
Changed the variable names. Hope this looks fine now! Thanks
Attachment #8968271 - Flags: review?(prathikshaprasadsuman)

Comment 26

11 months ago
Comment on attachment 8968271 [details] [diff] [review]
bug1444441.patch

Tagging Johann for a final review so we can be sure here.
Attachment #8968271 - Flags: review?(prathikshaprasadsuman) → review?(jhofmann)
(Reporter)

Comment 27

11 months ago
Comment on attachment 8968271 [details] [diff] [review]
bug1444441.patch

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

Looks good, thank you for all the work!

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +5,5 @@
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");

nit: can you add a comment that we need to do this, otherwise the "command" event won't fire?

also in the other 2 occurrences, please.
Attachment #8968271 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8968271 - Attachment is obsolete: true
(Assignee)

Comment 29

11 months ago
(In reply to Johann Hofmann [:johannh] from comment #27)
> Comment on attachment 8968271 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8968271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thank you for all the work!
> 
> ::: browser/base/content/test/permissions/browser_reservedkey.js
> @@ +5,5 @@
> > +  key1.setAttribute("modifiers", "shift");
> > +  key1.setAttribute("key", "O");
> > +  key1.setAttribute("reserved", "true");
> > +  key1.setAttribute("count", "0");
> > +  key1.setAttribute("oncommand", "//");
> 
> nit: can you add a comment that we need to do this, otherwise the "command"
> event won't fire?
> 
> also in the other 2 occurrences, please.

Is this attachment okay?
(Reporter)

Comment 30

11 months ago
mozreview-review
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review243270

That looks good, thank you!

Before you set checkin-needed, you will need to resolve all outstanding issues in mozreview (click on the yellow label with the number and set each individual item to fixed or dropped).
Attachment #8965008 - Flags: review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 31

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/081b471aa819
Remove unsafeSetInnerHTML in browser_reservedkey.js r=johannh,prathiksha
Keywords: checkin-needed

Comment 32

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/081b471aa819
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.