Closed
Bug 1460962
Opened 7 years ago
Closed 7 years ago
Enable customized built-in element in XUL
Categories
(Core :: DOM: Core & HTML, task, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: timdream, Assigned: timdream)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
With the assumption that all XUL elements are "built-in" elements, we should be able to extend their behavior, while keeping the same tag name, much like we do with XBL.
The alternative would be to create a "super" class that morph and load different customized behaviors. That's not good in terms of API affordance and might result in performance impact.
Yet another alternative would be to always define XUL elements as autonomous custom elements. Sadly it is clear that we won't be able to extend them because of [1]. It would be a huge work to change XUL tag names in the tree too.
[1] https://github.com/w3c/webcomponents/issues/750#issuecomment-384663104
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
The patch attached is what I have right now. It can create a customed built-in XUL element but none of the life-cycle callbacks are called. I will need to look into why.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
I think I get it working; let's wait for the Try results.
There are still assumption around the upgrade etc that needs some test cases, given our approach in bug 1446247 made every XUL tag name a custom element name, and bug 1460815 where we might be upgrading elements after-the-fact.
Assignee | ||
Comment 6•7 years ago
|
||
Try looks OK I guess?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=080e5d35c58e510299037edd9ee4497c8306bb12&selectedJob=178869279
I am updating the test script to include a few more tests on upgrading existing elements. So far the patch failed at upgrading the element created by the parser. Somewhere in nsXULElement::CreateFromPrototype() has to aware of the existence of |is| attribute and pass that to the NS_NewXULElement() accordingly.
Will update the patch when I do that.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8975190 -
Flags: review?(bugs)
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8975190 [details]
Bug 1460962 - Support customized built-in element in XUL
https://reviewboard.mozilla.org/r/243534/#review250858
Some minor tweaks and then prototype handling needs to be fixed for subclasses of XULElement
::: dom/base/CustomElementRegistry.cpp:768
(Diff revision 6)
> - tag == eHTMLTag_multicol) {
> + tag == eHTMLTag_multicol) {
> - aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> + aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> - return;
> + return;
> - }
> + }
> + } else { // kNameSpaceID_XUL
> + if (!nsContentUtils::IsCustomElementName(nameAtom, kNameSpaceID_XHTML)) {
Please add a comment why we use kNameSpaceID_XHTML here. any name is CE name for xul, but here we reuse HTML CE name.
::: dom/base/Element.cpp:4278
(Diff revision 6)
> MOZ_ASSERT(!slots->mCustomElementData, "Custom element data may not be changed once set.");
> #if DEBUG
> nsAtom* name = NodeInfo()->NameAtom();
> nsAtom* type = aData->GetCustomElementType();
> + if (NodeInfo()->NamespaceID() == kNameSpaceID_XHTML) {
> - if (nsContentUtils::IsCustomElementName(name, NodeInfo()->NamespaceID())) {
> + if (nsContentUtils::IsCustomElementName(name, NodeInfo()->NamespaceID())) {
Perhaps just pass html namespace explicitly. that is faster than re-lookup the namespace from nodeInfo
::: dom/base/Element.cpp:4284
(Diff revision 6)
> - MOZ_ASSERT(type == name);
> + MOZ_ASSERT(type == name);
> - } else {
> + } else {
> - MOZ_ASSERT(type != name);
> + MOZ_ASSERT(type != name);
> - }
> + }
> + } else { // kNameSpaceID_XUL
> + if (nsContentUtils::IsCustomElementName(name, kNameSpaceID_XHTML)) {
Please add a comment why we pass html namespace to IsCustomElementName
::: dom/base/nsContentUtils.cpp:9899
(Diff revision 6)
> if (nodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
> tag = nsHTMLTags::CaseSensitiveAtomTagToId(name);
> isCustomElementName = (tag == eHTMLTag_userdefined &&
> nsContentUtils::IsCustomElementName(name, kNameSpaceID_XHTML));
> } else {
> - isCustomElementName = nsContentUtils::IsCustomElementName(name, kNameSpaceID_XUL);
> + // Consider the tag name a built-in XUL element name, if we are extending it with [is].
Ok, this is confusing stuff. I hope we have tests for all the possible cases, <foo is="bar"> <foo-foo is=bar> <foo is="bar-bar"> etc.
::: dom/bindings/BindingUtils.cpp:3808
(Diff revision 6)
>
> - // If the definition is for a customized built-in element, the active
> + // If the definition is for a customized built-in element, the active
> - // function should be the localname's element interface.
> + // function should be the localname's element interface.
> - constructorGetterCallback cb = sConstructorGetterCallback[tag];
> + cb = sConstructorGetterCallback[tag];
> + } else { // kNameSpaceID_XUL
> + cb = XULElementBinding::GetConstructorObject;
I doubt this works with the new XULElement subclasses.
::: dom/tests/mochitest/webcomponents/test_xul_custom_element.xul:226
(Diff revision 6)
> <body xmlns="http://www.w3.org/1999/xhtml">
> <p id="display"></p>
> <div id="content" style="display: none">
> <test-custom-element id="element4" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/>
> <testwithoutdash id="element5" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/>
> + <axulelement id="element6" is="test-built-in-element" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/>
Could you add more tests here for parsing.
<a-xulelement is="foo">, <a-xulelement is="a-xulelement">,<axulelement is="axulelement"> and such.
Or if we want to MOZ_ASSERT in wrong cases, can we have crashtests with expected to fail (on debug) or something?
Attachment #8975190 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8975190 [details]
Bug 1460962 - Support customized built-in element in XUL
https://reviewboard.mozilla.org/r/243534/#review251142
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8975190 [details]
Bug 1460962 - Support customized built-in element in XUL
https://reviewboard.mozilla.org/r/243534/#review250858
> Ok, this is confusing stuff. I hope we have tests for all the possible cases, <foo is="bar"> <foo-foo is=bar> <foo is="bar-bar"> etc.
I don't want to crash the browser if the parser or script attempt to create elements with non-confirming tag-is pairs. That's not what HTML does either.
I have created `nonCustomElementCreate()` in the test to make sure we construct plain XULElements in these cases.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8975190 [details]
Bug 1460962 - Support customized built-in element in XUL
https://reviewboard.mozilla.org/r/243534/#review251180
::: dom/base/nsContentUtils.h:710
(Diff revision 8)
> nsIURI* aBaseURI);
>
> /**
> + * Returns true if |aName| is a name with dashes.
> + */
> + static bool IsNameHasDash(nsAtom* aName);
Perhaps call this IsNameWithDash
Attachment #8975190 -
Flags: review?(bugs) → review+
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87368ba3ff8b
Support customized built-in element in XUL r=smaug
Comment 18•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•5 years ago
|
Type: enhancement → task
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•