1443781 - Eliminate innerHTML usage in contentSearchUI.js

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Comment 1

[:prathiksha]

Attachment: Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

Review commit: https://reviewboard.mozilla.org/r/228614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228614/

Comment 2

[Francesco Lodolo [:flod]]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

https://reviewboard.mozilla.org/r/228614/#review234462

::: browser/locales/en-US/chrome/browser/search.properties:39
(Diff revision 1)
>  cmd_addFoundEngineMenu=Add search engine
>  
>  # LOCALIZATION NOTE (searchForSomethingWith):
>  # This string is used to build the header above the list of one-click
>  # search providers:  "Search for <user-typed string> with:"
> -# NB: please leave the <span> and its class exactly as it is in English.
> +searchForSomethingWith=Search for <> with:

A few issues here:
* Changing existing strings: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
* The "<>" is beyond brittle. You should use a standard variable, like `%S` (and account for `%1$S`), at least we get warnings if it's missing in localizations.

You should also take a look at bug 1431428

Comment 3

[:prathiksha]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228614/diff/1-2/

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

https://reviewboard.mozilla.org/r/228614/#review234904

Thank you for coming up with a nice and short solution to this, lack of getLocalizedFragment was making this one quite tricky!

(In reply to Francesco Lodolo [:flod] from comment #2)
> A few issues here:
> * Changing existing strings:
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Changing_existing_strings
> * The "<>" is beyond brittle. You should use a standard variable, like `%S`
> (and account for `%1$S`), at least we get warnings if it's missing in
> localizations.

I agree that using %S (and normalizing the "%1$S" variant) sounds like a better idea.
 
> You should also take a look at bug 1431428

contentSearchUI.js looks content privileged to me, which is probably the reason why she's not using BU.getLocalizedFragment.

::: browser/base/content/contentSearchUI.js:637
(Diff revision 2)
> -      // eslint-disable-next-line no-unsanitized/property
> -      searchWithHeader.innerHTML = this._strings.searchForSomethingWith;
> -      searchWithHeader.querySelector(".contentSearchSearchWithHeaderSearchText").textContent = this.input.value;
> +      let header = this._strings.searchForSomethingWith2.split("%S");
> +      labels[0].textContent = header[0];
> +      labels[1].textContent = this.input.value;
> +      labels[2].textContent = header[1];
>      } else {
> -      searchWithHeader.textContent = this._strings.searchWithHeader;
> +      labels[0].textContent = this._strings.searchWithHeader;

You probably need to clear the other labels in this case.

::: browser/base/content/contentSearchUI.js:796
(Diff revision 2)
>      headerRow.setAttribute("class", "contentSearchHeaderRow");
>      header.setAttribute("class", "contentSearchHeader");
>      headerRow.appendChild(header);
>      header.id = "contentSearchSearchWithHeader";
> +    let start = document.createElement("label");
> +    let inputString = document.createElement("label");

nit: that variable name is a bit weird, the thing you're creating is not a string, it's a label. Maybe inputLabel or searchText?

Comment 5

[:prathiksha]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228614/diff/2-3/

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

https://reviewboard.mozilla.org/r/228614/#review235074

::: browser/base/content/contentSearchUI.js:633
(Diff revision 3)
>      }
>      let searchWithHeader = document.getElementById("contentSearchSearchWithHeader");
> +    let labels = searchWithHeader.querySelectorAll("label");
>      if (this.input.value) {
> -      // eslint-disable-next-line no-unsanitized/property
> -      searchWithHeader.innerHTML = this._strings.searchForSomethingWith;
> +      let header = this._strings.searchForSomethingWith2;
> +      header = header.replace("%S", "<>").replace("%1$S", "<>").split("<>");

Probably cleaner like this? But wait for Johannh to confirm if that makes any sense ;-)

let headerParts = this._strings.searchForSomethingWith2.split(/%(?:1\$)?S/);

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

https://reviewboard.mozilla.org/r/228614/#review235074

> Probably cleaner like this? But wait for Johannh to confirm if that makes any sense ;-)
> 
> let headerParts = this._strings.searchForSomethingWith2.split(/%(?:1\$)?S/);

I'd just replace "%1$S" with "%S" and split on "%S", maybe? Also please add a comment why we're doing this!

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

https://reviewboard.mozilla.org/r/228614/#review235752

r=me with nits fixed :)

::: browser/base/content/contentSearchUI.js:639
(Diff revision 3)
> -      searchWithHeader.querySelector(".contentSearchSearchWithHeaderSearchText").textContent = this.input.value;
> +      labels[0].textContent = header[0];
> +      labels[1].textContent = this.input.value;
> +      labels[2].textContent = header[1];
>      } else {
> -      searchWithHeader.textContent = this._strings.searchWithHeader;
> +      labels[0].textContent = this._strings.searchWithHeader;
> +      labels[1].textContent = labels[2].textContent = "";

Nit: Please assign this in two separate lines :)

Comment 9

[:prathiksha]

Comment on attachment 8959852 [details]
Bug 1443781 - Eliminate innerHTML usage in contentSearchUI.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228614/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a531cf7b4a54
Eliminate innerHTML usage in contentSearchUI.js. r=johannh

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a531cf7b4a54