Convert search-one-offs binding to custom element

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: ntim, Assigned: bgrins)

Tracking

(Blocks 1 bug)

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 months ago
Could be useful for bug 1491248.
(Reporter)

Updated

7 months ago
Blocks: war-on-xbl
(Reporter)

Comment 2

7 months ago
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
(Assignee)

Updated

7 months ago
See Also: → 1460982

Updated

7 months ago
Priority: -- → P3
(Assignee)

Comment 4

7 months ago
(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)
(Reporter)

Comment 5

7 months ago
(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.
(Reporter)

Comment 7

7 months ago
(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.
(Assignee)

Comment 8

7 months ago
(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.
(Assignee)

Comment 9

7 months ago
Posted 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)
(Assignee)

Comment 10

7 months ago
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.
(Assignee)

Comment 11

7 months ago
(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)
(Reporter)

Updated

7 months ago
Depends on: 1491724
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 13

7 months ago
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
(Reporter)

Updated

7 months ago
Assignee: bgrinstead → ntim.bugs
(Assignee)

Updated

7 months ago
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)
(Reporter)

Comment 17

7 months ago
(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)
(Assignee)

Comment 18

6 months ago
(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)
(Reporter)

Updated

6 months ago
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
(Reporter)

Updated

6 months ago
Assignee: ntim.bugs → nobody
(Assignee)

Updated

6 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Attachment #9011298 - Attachment is obsolete: true

Comment 21

6 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec90e4f6f2a2
Convert search-one-offs binding to custom element;r=dao
(Assignee)

Updated

6 months ago
Blocks: 1502151

Comment 22

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec90e4f6f2a2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Assignee)

Updated

6 months ago
Blocks: 1506237
(Assignee)

Updated

6 months ago
Blocks: 1506261

Updated

4 months ago
Depends on: 1515939
You need to log in before you can comment on or make changes to this bug.