Closed Bug 1478139 Opened 2 years ago Closed 2 years ago

Migrate <editor> to a Custom Element

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

The editor element is used in some tests. It looks like a relatively easy conversion code-wise, although there seems to be an issue with extending XULFrameElement in the Custom Element class that we'd need to figure out.
Neil, if I have MozEditor extend MozXULElement (and thus XULElement) then it properly constructs and upgrades the element, but  then `this.webNavigation` is undefined. If I directly try to extend XULFrameElement then I get "TypeError: Illegal constructor." when creating the element.

I'm testing with this command in the Browser Console, btw:

  var e = document.createElement("editor");
  document.documentElement.append(e);
  console.log(e, e.webNavigation)

Any ideas on what to do? I expect we'll get the same issue when creating the browser Custom Elements.
Flags: needinfo?(enndeakin)
See Also: → 1437638
See Also: → 1441935
I think Tim would be better to answer this.
Flags: needinfo?(enndeakin) → needinfo?(timdream)
Priority: -- → P3
Forwarding ni? to Mossop as discussed earlier
Flags: needinfo?(timdream) → needinfo?(dtownsend)
Flags: needinfo?(timdream)
The basic problem is that you're trying to extend something that is more specialized than XULElement (XULFrameElement in this case). The proper way to do this would be to do this as a customized built-in element, so define a special instance of editor as:

    customElements.define("ed", MozEditor, { extends: "editor" });

In this case we'd end up with <editor> being a plain XULFrameElement with no additional behaviour and then having <ed> being where we define all the specialized behaviour we actually want.

That's a bit weird though and given that we can make some tweaks for XUL only we can make it possible to do this directly. The patch I've attached does that. It has the drawback that currently the element tag names are hardcoded. That's likely fixable but a lot more work and I don't know if we really care about being able to define arbitrary tags that extends from XULFrameElement etc.
Flags: needinfo?(dtownsend)
Comment on attachment 8995214 [details]
Bug 1478139: Allow writing custom element definitions for the special XUL elements.

I've only tested this as far as the code example in comment 2 and that works but you probably want to exercise this a bit more to see if things actually work.
Attachment #8995214 - Flags: feedback?(bgrinstead)
(In reply to Dave Townsend [:mossop] from comment #6)
> That's a bit weird though and given that we can make some tweaks for XUL
> only we can make it possible to do this directly. The patch I've attached
> does that. It has the drawback that currently the element tag names are
> hardcoded. That's likely fixable but a lot more work and I don't know if we
> really care about being able to define arbitrary tags that extends from
> XULFrameElement etc.

I think having the hardcoded list of tags that extend XULFrameElement etc is totally fine - we need that list anyway for non-CE subclass construction. I don't think we need to be able to do this from JS only.

(In reply to Dave Townsend [:mossop] from comment #7)
> Comment on attachment 8995214 [details]
> Bug 1478139: Allow writing custom element definitions for the special XUL
> elements.
> 
> I've only tested this as far as the code example in comment 2 and that works
> but you probably want to exercise this a bit more to see if things actually
> work.

Thanks! I'll test it out.
It seems to generally work (editor related tests pass!), but there's a number of errors/crashes with the patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5138fd918181977ee3d01b419afe3eb505aa6272
Comment on attachment 8995214 [details]
Bug 1478139: Allow writing custom element definitions for the special XUL elements.

`./mach mochitest editor/` passes locally for me with this applied, and as per Comment 8 I think it's fine for the tags to be hardcoded.

The remaining issue is the crashes and test failures (Comment 9).
Attachment #8995214 - Flags: feedback?(bgrinstead) → feedback+
An uninitialized variable is causing the crashes. Let's see how this does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1675dafc69b3b8e17797262447ff542090c70f4
Attachment #8995214 - Attachment is obsolete: true
Paolo, I'm going to be away until Wed next week - but this should be landable assuming Bug 1480195 sticks and you are happy with the changes - up to you. Here's a try push (eslint issue has been fixed in latest mozreview patch): https://treeherder.mozilla.org/#/jobs?repo=try&revision=535c01821e1501ce960a4b68fec823a31714fd5e
Clearing ni? since Mossop fixed this in Bug 1480195
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Jorg, could you confirm that any <editor> instances in TB still work with this patch applied? It's a pretty straightforward conversion so I expect it will - although if you have any inside of XBL anon content we may get hit by Bug 1470242.
Flags: needinfo?(jorgk)
As far as I can see, the only <editor> we have in TB is in mail/components/compose/content/messengercompose.xul:
https://searchfox.org/comm-central/search?q=%3Ceditor&case=false&regexp=false&path=

I think the other two in editor/ui/composer/content/editor.xul may be for SM's HTML editor (but I could be wrong).

I tried out message compose in HTML and plaintext modes and it still seems to work. To be sure:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f7124d9da9cefcc581814c78c85070d664018282
which is based on a try push of your patch:
https://hg.mozilla.org/try/rev/bc1ffd09fd7dde43b612e7bf74f452532a92651e
Flags: needinfo?(jorgk)
The try run didn't show any problems.
Comment on attachment 8994656 [details]
Bug 1478139 - Migrate <editor> to a Custom Element;

https://reviewboard.mozilla.org/r/259160/#review268528

This looks good, just needed rebasing. I'll land the rebased version.
Attachment #8994656 - Flags: review?(paolo.mozmail) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9101a4dc38
Migrate <editor> to a Custom Element. r=paolo
https://hg.mozilla.org/mozilla-central/rev/0e9101a4dc38
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.