1478139 - Migrate <editor> to a Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1478139 - Migrate <editor> to a Custom Element;

Review commit: https://reviewboard.mozilla.org/r/259160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259160/

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Neil Deakin]

I think Tim would be better to answer this.

Comment 4

[Brian Grinstead [:bgrins]]

Forwarding ni? to Mossop as discussed earlier

Comment 5

[Dave Townsend [:mossop]]

Attachment: Bug 1478139: Allow writing custom element definitions for the special XUL elements.

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

(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.

Comment 9

[Brian Grinstead [:bgrins]]

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 10

[Brian Grinstead [:bgrins]]

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).

Comment 11

[Dave Townsend [:mossop]]

An uninitialized variable is causing the crashes. Let's see how this does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1675dafc69b3b8e17797262447ff542090c70f4

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8994656 [details]
Bug 1478139 - Migrate <editor> to a Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259160/diff/1-2/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8994656 [details]
Bug 1478139 - Migrate <editor> to a Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259160/diff/2-3/

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Brian Grinstead [:bgrins]]

Clearing ni? since Mossop fixed this in Bug 1480195

Comment 16

[Brian Grinstead [:bgrins]]

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.

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Jorg K (CEST = GMT+2)]

The try run didn't show any problems.

Comment 19

[:Paolo Amadini]

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.

Comment 20

[:Paolo Amadini]

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

Comment 21

[:Paolo Amadini]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9101a4dc38
Migrate <editor> to a Custom Element. r=paolo

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0e9101a4dc38