1119233 - Move inline event handlers and styles from search.xul into other files

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[:Gijs (he/him)]

As per summary. These were (mostly?) added in bug 1088660 - the event handlers should be in JS instead, and the styles should be in CSS.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: js (apply first), v1

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: css, v1

Successful Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed64a9f87416

Comment 3

[:Gijs (he/him)]

Comment on attachment 8546494 [details] [diff] [review]
js (apply first), v1

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

Instead of adding the listeners as functions, please consider adding gSearchPane, and implementing a handleEvent method instead, with a switch() over the event type and then checks on the target IDs. This will avoid having to use anonymous functions to keep |this| in line (as |this| will be gSearchPane anyway in those cases).

::: browser/components/preferences/in-content/search.js
@@ +15,5 @@
>    {
> +    function setEventListener(aId, aEventType, aCallback)
> +    {
> +      document.getElementById(aId)
> +              .addEventListener(aEventType, aCallback.bind(gSearchPane));

Nit:

let node = document.getElementById(aId);
node.addEventListener...

::: browser/components/preferences/in-content/search.xul
@@ +54,5 @@
>        <hbox>
>          <button id="restoreDefaultSearchEngines"
>                  label="&restoreDefaultSearchEngines.label;"
>                  accesskey="&restoreDefaultSearchEngines.accesskey;"
> +        />

Nit: line up with "accesskey", please. :-)

@@ +60,5 @@
>          <button id="removeEngineButton"
>                  label="&removeEngine.label;"
>                  accesskey="&removeEngine.accesskey;"
>                  disabled="true"
> +        />

Nit: same.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8546495 [details] [diff] [review]
css, v1

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

::: browser/components/preferences/in-content/search.xul
@@ -24,5 @@
>        <label class="header-name">&paneSearch.title;</label>
>      </hbox>
>  
>      <!-- Default Search Engine -->
> -    <groupbox id="defaultEngineGroup" align="start" data-category="paneSearch">

I'm not sure if it's necessary to remove align="start", but it's certainly a good idea in principle. However...

@@ +65,5 @@
>        </hbox>
>  
>        <separator class="thin"/>
>  
> +      <hbox id="addEnginesBox" pack="start">

... but then you can also remove pack="start" (the equivalent is -moz-box-pack: start).

The same goes for flex (-moz-box-flex).

I think with those attributes (XUL flexbox model) we should be consistent and either move all of them to CSS or keep all of them in XUL. Considering precedent and requirements for this bug, I guess right now we should keep them in XUL, but we should open a discussion on firefox-dev. Personally, I suspect moving away from the attributes and towards using CSS for these is a good idea, but we should get feedback about it and then implement it more widely. :-)

::: browser/themes/shared/incontentprefs/search.css
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#defaultEngineGroup {
> +  text-align: start;

the equivalent of align="start" in XUL is -moz-box-align: start, not text-align: start.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: js (apply first), v2

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: css, v2

Comment 7

[:Gijs (he/him)]

Comment on attachment 8546671 [details] [diff] [review]
js (apply first), v2

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

You're not actually adding gSearchPane as an event listener anywhere... did you not test this? Or did you forget to qref?

::: browser/components/preferences/in-content/search.js
@@ +63,5 @@
>      });
>    },
>  
> +  handleEvent: function(aEvent) {
> +    switch (aEvent.type) {

Nit: please indent all the case statements 2 spaces, too.

@@ +65,5 @@
>  
> +  handleEvent: function(aEvent) {
> +    switch (aEvent.type) {
> +    case "click":
> +      if (aEvent.target.id == "addEngines" && event.button == 0)

Nit: braces around the bits inside the if, please. :-)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: js (apply first), v3

> You're not actually adding gSearchPane as an event listener anywhere... did
> you not test this? Or did you forget to qref?
Forgot -purgecaches.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8546785 [details] [diff] [review]
js (apply first), v3

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

LGTM!

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: js (apply first), v4, r=Gijs

Successful Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8135d3ffaaea

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: css, v3, r=Gijs

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/18d17da8d569
https://hg.mozilla.org/integration/fx-team/rev/5a902d6fc538

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/18d17da8d569
https://hg.mozilla.org/mozilla-central/rev/5a902d6fc538