All users were logged out of Bugzilla on October 13th, 2018

Get rid of innerHTML in Keyboard app

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sergi, Assigned: sergi)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 727663 [details] [review]
Github pull request
This patch takes care of sanitising innerHTML as per issue https://bugzilla.mozilla.org/show_bug.cgi?id=837740. It makes use of textContent and DocumentFragment in some cases, which also improves the speed of these operations. Some other small fixes in code and comments have been made.
Assignee: nobody → sergi.mansilla
Attachment #727663 - Flags: review?(dflanagan)
Comment on attachment 727663 [details] [review]
Github pull request

Thanks for doing this. The patch looks mostly good.  r- because of linter errors (see the Travis log on the github page starting around line 60).  And also because I think I found one or two errors in your patch.  See github for specific comments.
Attachment #727663 - Flags: review?(dflanagan) → review-
Created attachment 728895 [details] [review]
New Pull Request URL
Attachment #727663 - Attachment is obsolete: true
Attachment #728895 - Flags: review?(dflanagan)
Comment on attachment 728895 [details] [review]
New Pull Request URL

r- this time because I don't want want dom.js to go in the shared directory with only one function in it.  I think a small library of shared dom utilities would be worth having.  But not until it has more utility than this.

Also, see the discussion that has continued in https://github.com/mozilla-b2g/gaia/pull/8718 about the fastest method of clearing a DOM element.  I think that Rick's test had an error. In my versions, setting innerHTML to the empty string is actually the fastest technique.

It would be fine with me to just use innerHTML='' in render.js
Attachment #728895 - Flags: review?(dflanagan) → review-
Created attachment 732243 [details] [review]
New Pull Request URL
Attachment #728895 - Attachment is obsolete: true
Attachment #732243 - Flags: review?(dflanagan)
Comment on attachment 732243 [details] [review]
New Pull Request URL

Sergi,

The code looks good, except for one linter error and one indentation error that should be trivial to fix.

Unfortunately, in testing this out, it appears that it breaks the keyboard layout switcher menu.  

To try this out, go to Settings->Keyboard and enable multiple layouts (like "other latin").  Then, when you use the keyboard, you'll see a globe icon in the lower left.  Pressing and holding it brings up a menu of available layouts.

On master, I can move my finger up the menu to select the layout I want. With your patch applied, the menu disappears when I move my finger off the globe button.
Attachment #732243 - Flags: review?(dflanagan) → review-
Created attachment 732748 [details] [review]
Pull Request URL
Attachment #732243 - Attachment is obsolete: true
Attachment #732748 - Flags: review?(dflanagan)
David,

Good catch. I found the problem and it is solved now. I added the newline and gjslint seems happy. I don't believe there is any indentation error left, but let me know if you find any.

Sergi
Comment on attachment 732748 [details] [review]
Pull Request URL

Sergi,

I don't understand why your cloneNode() code didn't work the way the existing code did.  By going back to the old code, you've got innerHTML in there again, but this is just a safe use of it to clone, so I'm okay with that.  I'm pretty sure that this layout switching menu is going to be replaced anyway with a full-screen system menu.

There is still an indentation issue at render.js:130, at least according to they way emacs indents my JavaScript, but that's not worth holding this up for.

r+, but squash your commits before landing anything.  Let me know (by email, irc, or needinfo?) if you'd like me to land it.
Attachment #732748 - Flags: review?(dflanagan) → review+
David,

Yeah, it shouldn't be a problem since innerHTML is set to empty string. I fixed the indentation issue and squashed commits together.

Also, what would be necessary for me to be able to land commits (of course not my own)?

Sergi
Daivid, please land it if it's ok for you.
Flags: needinfo?(dflanagan)
Landed on master: https://github.com/mozilla-b2g/gaia/commit/f4e802d142148106263a56b2b3c9a2b98562c7ef
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: needinfo?(dflanagan)
Resolution: --- → FIXED
(In reply to Sergi Mansilla from comment #11)
> 
> Also, what would be necessary for me to be able to land commits (of course
> not my own)?
> 

I actually don't know what are policy is about that.  Maybe ask on dev-gaia?

Sergi, do you feel that there is a noticeable performance difference with this patch applied? If so, we should consider uplifting it to v1-train.  (And uplifting to v1-train will make it easier to uplift keyboard auto-correction to v1-train, since that will also modify the render.js file.)
Flags: needinfo?(sergi.mansilla)
David,

It feels a bit faster, but it might as well be placebo effect. I didn't make any benchmark test, but I might do that when I have time. I think it is safe enough to go to v1, but it is obviously not my call.
Flags: needinfo?(sergi.mansilla)
If this ever gets uplifted, we also need to uplift 859159 which fixes a regression it caused.

Updated

6 years ago
Blocks: 835404
Attachment mime type: text/plain text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.