Closed Bug 1493536 Opened 6 years ago Closed 6 years ago

Convert search-one-offs binding to custom element

Categories

(Firefox :: Search, task, P3)

task

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: ntim, Assigned: bgrins)

References

Details

Attachments

(1 file, 2 obsolete files)

Could be useful for bug 1491248.
Blocks: war-on-xbl
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
See Also: → 1460982
Priority: -- → P3
(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?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs) → needinfo?(bgrinstead)
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.
(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.
(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.
Attached patch context-menu-open.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bgrinstead
Flags: needinfo?(bgrinstead)
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.
(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.
Flags: needinfo?(ntim.bugs)
Depends on: 1491724
Flags: needinfo?(ntim.bugs)
Thanks for looking into the issue! Will fold the patch then re-submit.
Attachment #9011298 - Attachment description: Bug 1493536 - Convert search-one-offs binding to custom element. → Bug 1493536 - Convert search-one-offs binding to custom element. r=florian
Assignee: bgrinstead → ntim.bugs
Attachment #9013026 - Attachment is obsolete: true
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?
Flags: needinfo?(bgrinstead)
(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)
(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?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(dao+bmo)
Flags: needinfo?(ntim.bugs)
Attachment #9011298 - Attachment description: Bug 1493536 - Convert search-one-offs binding to custom element. r=florian → Bug 1493536 - Convert search-one-offs binding to custom element. r=dao
Assignee: ntim.bugs → nobody
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #9011298 - Attachment is obsolete: true
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec90e4f6f2a2
Convert search-one-offs binding to custom element;r=dao
Blocks: 1502151
https://hg.mozilla.org/mozilla-central/rev/ec90e4f6f2a2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Blocks: 1506237
Blocks: 1506261
Depends on: 1515939
Type: defect → task
You need to log in before you can comment on or make changes to this bug.