Closed
Bug 1478139
Opened 6 years ago
Closed 6 years ago
Migrate <editor> to a Custom Element
Categories
(Toolkit :: XUL Widgets, task, P3)
Toolkit
XUL Widgets
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years 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•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
I think Tim would be better to answer this.
Flags: needinfo?(enndeakin) → needinfo?(timdream)
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•6 years ago
|
||
Forwarding ni? to Mossop as discussed earlier
Flags: needinfo?(timdream) → needinfo?(dtownsend)
Updated•6 years ago
|
Flags: needinfo?(timdream)
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
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•6 years 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•6 years 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•6 years 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+
Comment 11•6 years ago
|
||
An uninitialized variable is causing the crashes. Let's see how this does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1675dafc69b3b8e17797262447ff542090c70f4
Updated•6 years ago
|
Attachment #8995214 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years 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•6 years ago
|
||
Clearing ni? since Mossop fixed this in Bug 1480195
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Assignee | ||
Comment 16•6 years 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•6 years 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®exp=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•6 years ago
|
||
The try run didn't show any problems.
Comment 19•6 years 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 20•6 years ago
|
||
Comment 21•6 years ago
|
||
Comment 22•6 years 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•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•