Closed Bug 1446247 Opened 6 years ago Closed 6 years ago

Allow non-dashed Custom Elements for XUL elements to make migration from XBL bindings easier

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Bug 1446247 - Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;
59 bytes, text/x-review-board-request
smaug
: review+
Details 
Background for this request is here: https://groups.google.com/forum/#!topic/firefox-dev/L1ohKKXh1y0

There are a couple options for migrating existing XBL bindings to Custom Elements that involve temporarily allowing a non-dashed version of the tag name in order to make frontend development easier. That is, we want to be able to land a XBL->CE migration without also rewriting all of the frontend code that uses those elements (and keeping that rebased during development).

Option 1 is to modify NS_NewXULElement to map an existing name (i.e. "deck") to another one (i.e. "moz-deck"). Then we would define the Custom Element as "moz-deck" and also update existing CSS rules to reference the new tag. Then after rewriting the rest of the frontend to actually use <moz-deck> we would convert the mapping into an assertion to make sure we didn't miss anything.

Option 2 is to modify IsCustomElementName to allow for non-dashed names for XUL elements and XUL documents (either unconditionally, with a whitelist of tag names we want to migrate, or for everything except for a blacklist of common tags like 'hbox', 'image', etc). In that case we would define the Custom Element as "deck" for initial landing, and then rewrite the rest of the frontend along with CSS to "moz-deck" in a second bug.

One benefit to option 1 is that we could migrate the frontend piecemeal (minus CSS which would need to be rewritten immediately). One benefit to option 2 is that it's a simpler intermediate state (there's no confusion where you see <deck> in the markup and then <moz-deck> in the CSS/Inspector).
The blacklist is pulled from the most frequently created tag names generated during browser-instrumentation runs on treeherder (https://github.com/bgrins/xbl-analysis/blob/gh-pages/cache/instrumentation/2018-03-13/xulsummary.txt). It isn't fully representative given Bug 1443328, but I think it's close enough.

Talos shows only a tp6_amazon regression, which I think is just noise as I've seen it on a bunch of pushes before: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=938e4bbb3d658b46ff4ba7f6972080d380c07f2e&newProject=try&newRevision=1b4a7b47071f4415fcf4fa203d68dd4e1199d3fd&framework=1&showOnlyImportant=1
Blocks: 1411707
Comment on attachment 8959410 [details]
Bug 1446247 - Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;

https://reviewboard.mozilla.org/r/228226/#review234378

Given that XUL requires that most elements be implemented in either XBL or JS, which allows non-dashed names, the conversation in https://github.com/w3c/webcomponents/issues/634 and that XUL is a Mozilla-only technology, I am in support of option 2.

My only concern is that once the removal of XUL is given the green light (see [Problems with XUL](https://mozilla.github.io/firefox-browser-architecture/text/0003-problems-with-xul.html) and [XBL Removal Design Review Packet – Competetive Analysis Alternative 5](https://mozilla.github.io/firefox-browser-architecture/text/0007-xbl-design-review-packet.html#competitive-analysis)), the lack of dashes in the names would require to rename all non-dashed names to have dashes anyway, but this is probably something that we should worry about after XBL removal is complete.

::: dom/base/nsContentUtils.cpp:3334
(Diff revision 2)
> -nsContentUtils::IsCustomElementName(nsAtom* aName)
> +nsContentUtils::IsCustomElementName(nsAtom* aName, uint32_t aNameSpaceID)
>  {
> +  // Allow non-dashed names in XUL for XBL to Custom Element migrations. Skip
> +  // commonly used tags that aren't Custom Elements to avoid extra work for them.
> +  if (aNameSpaceID == kNameSpaceID_XUL) {
> +    return !(

Wouldn’t it be slightly faster to just return true for all custom element names in the XUL namespace instead of checking a blacklist?
Attachment #8959410 - Flags: review+
FWIW, GetExtantDoc may return null. But I assume DOM peer review will be asked at some point.
(In reply to Olli Pettay [:smaug] from comment #6)
> FWIW, GetExtantDoc may return null. But I assume DOM peer review will be
> asked at some point.

Yes, I was going to ask if GetExtantDoc was even the right place to check for the namespace. In what case would the extant doc be null during Define/WhenDefined? Also, what should we do if it is null - can we assume that we are in an html ns in that case or is there a an alternate way to check?
Flags: needinfo?(bugs)
> In what case would the extant doc be null during Define/WhenDefined?

Here's a likely example:

  <iframe id="f"></iframe>
  <script>
    onload = function() {
      var reg = frames[0].customElements;
      document.getElementById("f").remove();
      setTimeout(function() {
        reg.define(whatever-args-here);
      }, 1000); // Give some time for the GC to run, to be extra sure
    }
  </script>

> Also, what should we do if it is null

Assuming HTML is probably as good as anything else.  I don't expect our chrome to ever be hitting this case.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Priority: -- → P3
Comment on attachment 8959410 [details]
Bug 1446247 - Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;

https://reviewboard.mozilla.org/r/228226/#review237956

::: dom/base/CustomElementRegistry.cpp:685
(Diff revision 4)
>  
>    /**
>     * 2. If name is not a valid custom element name, then throw a "SyntaxError"
>     *    DOMException and abort these steps.
>     */
> +  uint32_t nameSpaceID = mWindow->GetExtantDoc() ? mWindow->GetExtantDoc()->GetDefaultNamespaceID() : kNameSpaceID_XHTML;

overlong line.
perhaps
nsIDocument* doc = mWindow->GetExtantDoc();
uint32_t nameSpaceID = doc ? doc->GetDefaultNamespaceID() : kNameSpaceID_XHTML;

::: dom/base/CustomElementRegistry.cpp:969
(Diff revision 4)
>    if (aRv.Failed()) {
>      return nullptr;
>    }
>  
>    RefPtr<nsAtom> nameAtom(NS_Atomize(aName));
> -  if (!nsContentUtils::IsCustomElementName(nameAtom)) {
> +  uint32_t nameSpaceID = mWindow->GetExtantDoc() ? mWindow->GetExtantDoc()->GetDefaultNamespaceID() : kNameSpaceID_XHTML;

overlong line.
perhaps
nsIDocument* doc = mWindow->GetExtantDoc();
uint32_t nameSpaceID = doc ? doc->GetDefaultNamespaceID() : kNameSpaceID_XHTML;

::: dom/base/nsContentUtils.cpp:3200
(Diff revision 4)
>  bool
> -nsContentUtils::IsCustomElementName(nsAtom* aName)
> +nsContentUtils::IsCustomElementName(nsAtom* aName, uint32_t aNameSpaceID)
>  {
> +  // Allow non-dashed names in XUL for XBL to Custom Element migrations. Skip
> +  // commonly used tags that aren't Custom Elements to avoid extra work for them.
> +  if (aNameSpaceID == kNameSpaceID_XUL) {

so, you allow all the element names

::: dom/tests/mochitest/webcomponents/test_xul_custom_element.xul:65
(Diff revision 4)
> +      ok(element5 instanceof TestWithoutDash, "Should be an instance of TestWithoutDash");
> +
> +      try {
> +        // Non-dashed names are supported, except for some built in tags (Bug 1446247)
> +        customElements.define("hbox", TestCustomElement);
> +        ok(false, "defining hbox didn't throw");

so how can this throw
Attachment #8959410 - Flags: review?(bugs) → review-
Depends on: 1449979
Comment on attachment 8959410 [details]
Bug 1446247 - Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;

https://reviewboard.mozilla.org/r/228226/#review238942
Attachment #8959410 - Flags: review?(bugs) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e220521c6ff6
Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;r=e7358d9c+590837,smaug
https://hg.mozilla.org/mozilla-central/rev/e220521c6ff6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451340
Depends on: 1451974
Status: RESOLVED → REOPENED
status-firefox61: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Backout by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/42cfc3e15726
Backed out changeset e220521c6ff6 on request a=backout r=smaug
Since talos didn't catch Bug 1451974 I don't really expect to see anything, but a new talos push only currently shows 'tp6_amazon opt e10s stylo', which is within the range of noise from the last few days on that test: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2a73af3da855e8bb047435def3e3bbaf9f29663a&newProject=try&newRevision=57520af2a78308ef4e4c441bb00f7ba599bf724a&framework=1&showOnlyImportant=1.

Now that Bug 1452074 has been fixed I can do some manual testing with a bunch of tabs to simulate Bug 1451974.
Flags: needinfo?(bgrinstead)
I generated a mach run command with 1500 tabs by running this in the webconsole: `copy("./mach run " + new Array(1500).join("--new-tab --url data:text/html, "))`.

Then I profiled opening and closing the tabs overflow menu:

- Without the patch applied: https://perfht.ml/2qrjfpl
- With the patch applied: https://perfht.ml/2quNA6L

I don't see many hits when filtering the stack on CustomElement - :smaug do you think we can reland this?
Flags: needinfo?(bugs)
I think we can. I do have some ideas to improve performance of unresolved elements even more, but that will require some more memory - so if we could live without that for XUL, great.
Flags: needinfo?(bugs)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13faaffe1c4f
Pass namespace into IsCustomElementName to allow for non-dashed XUL elements;r=e7358d9c+590837,smaug
https://hg.mozilla.org/mozilla-central/rev/13faaffe1c4f
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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: