1493536 - Convert search-one-offs binding to custom element

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Tim Nguyen :ntim]

Could be useful for bug 1491248.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1493536 - Convert search-one-offs binding to custom element. r=dao

Comment 2

[Tim Nguyen :ntim]

In case someone wants to take over this, here's what left to do:
- The search panel should persist when right clicking a one off button
- Possibly other test and linting issues

Comment 3

[Brian Grinstead [:bgrins]]

Try push showing remaining tests to be fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccc5127e356736e22ac9da6e2a131387568a550e

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Try push showing remaining tests to be fixed:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ccc5127e356736e22ac9da6e2a131387568a550e

Thanks for putting this up. Scanning these failures, it looks like this patch is almost ready to go. It's the fourth biggest binding remaining, so it'd be great to get it landed. Are you planning to finish this or looking for someone else to take it?

Comment 5

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Try push showing remaining tests to be fixed:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ccc5127e356736e22ac9da6e2a131387568a550e
> 
> Thanks for putting this up. Scanning these failures, it looks like this
> patch is almost ready to go. It's the fourth biggest binding remaining, so
> it'd be great to get it landed. Are you planning to finish this or looking
> for someone else to take it?

I've just fixed most of the test failures and pushed to Phabricator. The only issue left is the context menu issue that I mentioned in comment 2. I'm not sure how to solve it, and I don't really have time to investigate.

Comment 6

[Dão Gottwald [:dao]]

Copy-pasting my comment from bug 1491248:

A custom element is likely overkill here. We just need a class that takes a container and does stuff with it, and then we can use that in two places.

Comment 7

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #6)
> Copy-pasting my comment from bug 1491248:
> 
> We just need a class that takes a container and does stuff with it, and then we can use that in two places.

I tend to agree a class would be better, but converting to custom elements more straightforward than refactoring the binding to be a class, and updating existing usages. Converting to custom elements is literally running the binding through a script and fixing issues caught by tests.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Copy-pasting my comment from bug 1491248:
> > 
> > We just need a class that takes a container and does stuff with it, and then we can use that in two places.
> 
> I tend to agree a class would be better, but converting to custom elements
> more straightforward than refactoring the binding to be a class, and
> updating existing usages. Converting to custom elements is literally running
> the binding through a script and fixing issues caught by tests.

Yeah, I agree with that. As we've seen before, it's a lot easier to refactor things around once it's in JS and out of XBL.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: context-menu-open.patch

I don't know exactly why context="_child" doesn't prevent the outer popup from closing (surely something to do with it previously being in anon content). But, this seems to do the trick and doesn't seem too bad.

Comment 10

[Brian Grinstead [:bgrins]]

Let's see what try says with this patch on top of yours: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b41b6ad6f8f5604d1d3b6a74c406deabedf56c6f.

Also, we may want to lazify the script loading by using `customElements.setElementCreationCallback` with the tag name instead of pulling it in eagerly (we have to do that with <searchbar> because it's in the markup, but I don't think that's the case here). I'm guessing that would clean up the medium confidence tpaint regression at https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=febeb326e1d86606536fb6e9bf207296f4f4145a&newProject=try&newRevision=78142fa3e0ca016a6c91f8873696129a78fb4541&framework=1&showOnlyImportant=1.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Let's see what try says with this patch on top of yours:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b41b6ad6f8f5604d1d3b6a74c406deabedf56c6f.

This looks good! Would you be able to fold together and get this up for review? FYI it's a lot easier for the reviewer to see the  search-one-offs.js changes while ignoring whitespace i.e. https://phabricator.services.mozilla.com/D6576?whitespace=ignore-all.

Comment 12

[Brian Grinstead [:bgrins]]

May need to rebuild based on top of Bug 1491724 if it's not already: https://hg.mozilla.org/mozilla-central/log/tip/browser/components/search/content/search.xml.

Comment 13

[Tim Nguyen :ntim]

Thanks for looking into the issue! Will fold the patch then re-submit.

Comment 14

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f01af7a7cd10a919c92792ff7823beeb4229716

Comment 15

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9be90d215f15f3eafec12144b6c0bb02c220183

Comment 16

[Dão Gottwald [:dao]]

bgrins, instead of all event listeners living in the constructor, which seems a bit unstructured and not ideal to maintain, what do you think about adopting a pattern like the one I used in UrlbarInput.jsm? E.g.:

https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/UrlbarInput.jsm#104

https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/UrlbarInput.jsm#171-174

https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/UrlbarInput.jsm#336

Would it be feasible to let your script do that?

Comment 17

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #16)
> Would it be feasible to let your script do that?

That would conflict with bindings that already have a handleEvent function (search-one-offs is one of them)

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #16)
> bgrins, instead of all event listeners living in the constructor, which
> seems a bit unstructured and not ideal to maintain, what do you think about
> adopting a pattern like the one I used in UrlbarInput.jsm? E.g.:
> 
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/
> UrlbarInput.jsm#104
> 
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/
> UrlbarInput.jsm#171-174
> 
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/browser/components/urlbar/
> UrlbarInput.jsm#336
> 
> Would it be feasible to let your script do that?

As Tim points out there could be some conflicts depending on how bindings currently work. But maybe we could make a separate object with `handleEvent` and hook it up in the constructor. For example:


```
MozXULElement {
  constructor() {
    super();
    this.eventHandler = {
      handleEvent(event) {
        let methodName = "_on_" + event.type;
        if (methodName in this) {
          this[methodName](event);
        } else {
          throw "Unrecognized urlbar event: " + event.type;
        }
      }
    }
  }
}

MyElement extends MozXULElement {
  constructor() {
    super();
    this.addEventListener("blur", this.eventHandler);
  }
  _on_blur() {
    foo();
  }
}
```

Something like that would be doable to hook up with the converter script. We'd have to be careful about re-using the same event on multiple targets, i.e. `this.addEventListener("blur", this.eventHandler); this.inputField.addEventListener("blur", this.eventHandler);` would both call the same _on_blur function. But, at least when converting <handler>s we'd only ever be adding the addEventListener to `this`.

Alternatively, we could do this more directly using a naming convention, like:

```
MyElement extends MozXULElement {
  constructor() {
    super();
    this.addEventListener("blur", this._on_blur.bind(this));
  }
  _on_blur() {
    foo();
  }
}
```

Which could be easily wired up with the converter script.

What do you think?

Comment 19

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #17)
> (In reply to Dão Gottwald [::dao] from comment #16)
> > Would it be feasible to let your script do that?
> 
> That would conflict with bindings that already have a handleEvent function
> (search-one-offs is one of them)

Well, they should eventually be merged, which may require some manual work. Maybe the script could at least do it automatically when there's no handleEvent.

(In reply to Brian Grinstead [:bgrins] from comment #18)
> As Tim points out there could be some conflicts depending on how bindings
> currently work. But maybe we could make a separate object with `handleEvent`
> and hook it up in the constructor. For example:
> 
> 
> ```
> MozXULElement {
>   constructor() {
>     super();
>     this.eventHandler = {
>       handleEvent(event) {
>         let methodName = "_on_" + event.type;
>         if (methodName in this) {
>           this[methodName](event);
>         } else {
>           throw "Unrecognized urlbar event: " + event.type;
>         }
>       }
>     }
>   }
> }
> 
> MyElement extends MozXULElement {
>   constructor() {
>     super();
>     this.addEventListener("blur", this.eventHandler);
>   }
>   _on_blur() {
>     foo();
>   }
> }
> ```
> 
> Something like that would be doable to hook up with the converter script.

This seems unnecessarily convoluted... Let's back up a bit and ask how we'd ideally structure this code, leaving aside what the script alone can do.

> We'd have to be careful about re-using the same event on multiple targets,
> i.e. `this.addEventListener("blur", this.eventHandler);
> this.inputField.addEventListener("blur", this.eventHandler);` would both
> call the same _on_blur function. But, at least when converting <handler>s
> we'd only ever be adding the addEventListener to `this`.

Yes, need to be careful there, but the pattern itself is sane. Handlers should check the event target when they can get the same event from different sources.

> Alternatively, we could do this more directly using a naming convention,
> like:
> 
> ```
> MyElement extends MozXULElement {
>   constructor() {
>     super();
>     this.addEventListener("blur", this._on_blur.bind(this));
>   }
>   _on_blur() {
>     foo();
>   }
> }
> ```
> 
> Which could be easily wired up with the converter script.

It's impossible to later remove that listener. You'd need to keep a reference to the bound method. Being able to just do this.removeEventListener("blur", this) seems preferable.

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: Bug 1493536 - Convert search-one-offs binding to custom element;r=dao

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec90e4f6f2a2
Convert search-one-offs binding to custom element;r=dao

Comment 22

[Cristina Coroiu [:ccoroiu]]

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