Closed Bug 1460962 Opened 6 years ago Closed 6 years ago

Enable customized built-in element in XUL

Categories

(Core :: DOM: Core & HTML, task, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Bug 1460962 - Support customized built-in element in XUL
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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.
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.
Attachment #8975190 - Flags: review?(bugs)
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/#review251142
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 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+
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87368ba3ff8b
Support customized built-in element in XUL r=smaug
https://hg.mozilla.org/mozilla-central/rev/87368ba3ff8b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Type: enhancement → task
See Also: → 1596530
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: