Enable customized built-in element in XUL

RESOLVED FIXED in Firefox 62

Status

()

P2
normal
RESOLVED FIXED
11 months ago
4 months ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

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
Priority: -- → P2
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)
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.
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)
Comment hidden (mozreview-request)

Comment 10

10 months 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-
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

10 months 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

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87368ba3ff8b
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1441935
You need to log in before you can comment on or make changes to this bug.