Migrate <editor> to a Custom Element

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

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

Updated

8 months ago
See Also: → bug 1437638
(Assignee)

Updated

8 months ago
See Also: → bug 1441935
I think Tim would be better to answer this.
Flags: needinfo?(enndeakin) → needinfo?(timdream)
Priority: -- → P3
(Assignee)

Comment 4

8 months ago
Forwarding ni? to Mossop as discussed earlier
Flags: needinfo?(timdream) → needinfo?(dtownsend)
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)
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

8 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

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

Comment 15

8 months ago
Clearing ni? since Mossop fixed this in Bug 1480195
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
(Assignee)

Comment 16

8 months ago
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)

Comment 17

8 months ago
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)

Comment 18

8 months ago
The try run didn't show any problems.

Comment 19

8 months ago
mozreview-review
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+

Comment 22

8 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9101a4dc38
Migrate <editor> to a Custom Element. r=paolo

Comment 23

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e9101a4dc38
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.