1334051 - Implement list of observed attributes for custom elements' attributeChanged callbacks

Status: RESOLVED FIXED

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

Description

[Edgar Chen [:edgar]]

We have implemented attributeChanged callback for old spec. but latest spec introduced a list of observed attributes to filter the attribute, e.g. step 4 of https://html.spec.whatwg.org/multipage/scripting.html#enqueue-a-custom-element-callback-reaction

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: add namespace in attributeChangedCallback.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list.

Comment 4

[Jessica Jong [:jessica] (inactive)]

With these 2 WIPs, we can pass 5 of 10 tests in wpt custom-elements/attribute-changed-callback.html.
And if we comment out the part that it checks for 'constructed', we can pass 9 of 10 tests.

Comment 5

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #4)
> With these 2 WIPs, we can pass 5 of 10 tests in wpt
> custom-elements/attribute-changed-callback.html.
> And if we comment out the part that it checks for 'constructed', we can pass
> 9 of 10 tests.

The 'constructed' case should be fixed after bug 1301024, which calls the constructor when creating a custom element via createElement/createElementNS.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: add namespace in attributeChangedCallback.

rebased on top of bug 1299363.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list.

rebased on top of bug 1299363, and added the part when upgrading an element (https://html.spec.whatwg.org/multipage/custom-elements.html#concept-upgrade-an-element, step 3).

Comment 8

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8893712 [details] [diff] [review]
(WIP) Part 1: add namespace in attributeChangedCallback.

Hi John, can I have your early feedback on this? Thanks.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8893715 [details] [diff] [review]
(WIP) Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list.

Hi John, can I have your early feedback on this? Thanks.

Comment 10

[John Dai[:jdai]]

Comment on attachment 8893715 [details] [diff] [review]
(WIP) Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list.

Review of attachment 8893715 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, it looks good. Thank you!

::: dom/base/CustomElementRegistry.cpp
@@ +582,5 @@
>    }
>  }
>  
> +static bool
> +AppendValueAsString(JSContext* aCx,

This function looks redundant to me, because we only call this function at Define(). Could you please merge into Define()? Thank you.

@@ +799,3 @@
>  
> +        if (!JS_GetProperty(cx, constructor,
> +                           "observedAttributes", &observedAttributesIterable)) {

Nit: add space before ".

@@ +985,5 @@
> +    LifecycleCallbackArgs args = {
> +      name, NullString(), (value.IsEmpty() ? NullString() : value),
> +      (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
> +    };
> +    EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement, &args, aDefinition);

Nit: This line over 80 characters.

::: testing/web-platform/meta/custom-elements/attribute-changed-callback.html.ini
@@ -27,2 @@
>    [attributedChangedCallback must be enqueued for style attribute change by mutating inline style declaration]
>      expected: FAIL

Per comment 4 and comment 5, this test remains fail after bug 1301024 is landed. Please make sure we have a bug for this. Thank you.

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to John Dai[:jdai] from comment #10)
> Comment on attachment 8893715 [details] [diff] [review]
> (WIP) Part 2: Invoke attributeChangedCallback only if attribute is in the
> observed attribute list.
> 
> Review of attachment 8893715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, it looks good. Thank you!

Thanks for the feedback, John.

> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +582,5 @@
> >    }
> >  }
> >  
> > +static bool
> > +AppendValueAsString(JSContext* aCx,
> 
> This function looks redundant to me, because we only call this function at
> Define(). Could you please merge into Define()? Thank you.

Ok, will do.

> 
> @@ +799,3 @@
> >  
> > +        if (!JS_GetProperty(cx, constructor,
> > +                           "observedAttributes", &observedAttributesIterable)) {
> 
> Nit: add space before ".

Will do.

> 
> @@ +985,5 @@
> > +    LifecycleCallbackArgs args = {
> > +      name, NullString(), (value.IsEmpty() ? NullString() : value),
> > +      (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
> > +    };
> > +    EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement, &args, aDefinition);
> 
> Nit: This line over 80 characters.

Will do.

> 
> :::
> testing/web-platform/meta/custom-elements/attribute-changed-callback.html.ini
> @@ -27,2 @@
> >    [attributedChangedCallback must be enqueued for style attribute change by mutating inline style declaration]
> >      expected: FAIL
> 
> Per comment 4 and comment 5, this test remains fail after bug 1301024 is
> landed. Please make sure we have a bug for this. Thank you.

Sure, will file a bug to track this if bug 1301024 is not landed when this patch gets reviewed.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add namespace in attributeChangedCallback.

rebased.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list.

rebased and addressed review comment 10.

Comment 14

[Jessica Jong [:jessica] (inactive)]

This is breaking v0 tests, since in the v0 spec there is no `observedAttributes` list, so attributeChangedCallback is fired on any attribute change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=852e2122a55a7dd4cf08b86189e811737c9928fb

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #14)
> This is breaking v0 tests, since in the v0 spec there is no
> `observedAttributes` list, so attributeChangedCallback is fired on any
> attribute change.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=852e2122a55a7dd4cf08b86189e811737c9928fb

I think I'll disabled these tests first, since fixing the tests will leave them half v0-compatible and half v1-compatible. Instead, I'll file a bug to track these and change the tests to v1-compatible once all v1 callbacks are implemented (bug 1293921).

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v1.

Disabled some mochitests.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8896894 [details] [diff] [review]
Part 1: add namespace in attributeChangedCallback.

Hi Olli, do you think you can help review this? :)

Comment 18

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8897729 [details] [diff] [review]
Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v1.

Review of attachment 8897729 [details] [diff] [review]:
-----------------------------------------------------------------

In this patch, we get the observedAttributes list when custom element is defined, and only fire attributeChangedCallback if the attribute that has been changed is on that list.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8897729 [details] [diff] [review]
Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v1.

># HG changeset patch
># Parent  2569260a38d7afd64741330474d838238f1d6dc1
>Bug 1334051 - Part 2: Invoke attributeChangedCallback only if attribute name is in the observed attribute list. f=jdai
>
>We call attributeChangedCallback in two cases:
>1. When any of the attributes in the observed attribute list has changed,
>   appended, removed, or replaced.
>2. When upgrading an element, for each attribute in element's attribute list
>   that is in the observed attribute list.
>
>MozReview-Commit-ID: LKUY5ibp9RI
>
>diff --git a/dom/base/CustomElementRegistry.cpp b/dom/base/CustomElementRegistry.cpp
>--- a/dom/base/CustomElementRegistry.cpp
>+++ b/dom/base/CustomElementRegistry.cpp
>@@ -447,30 +447,57 @@ CustomElementRegistry::SyncInvokeReactio
> }
> 
> void
> CustomElementRegistry::EnqueueLifecycleCallback(nsIDocument::ElementCallbackType aType,
>                                                 Element* aCustomElement,
>                                                 LifecycleCallbackArgs* aArgs,
>                                                 CustomElementDefinition* aDefinition)
> {
>+  RefPtr<CustomElementData> elementData = aCustomElement->GetCustomElementData();
>+  MOZ_ASSERT(elementData, "CustomElementData should exist");
>+
>+  // Let DEFINITION be ELEMENT's definition
>+  CustomElementDefinition* definition = aDefinition;
>+  if (!definition) {
>+    mozilla::dom::NodeInfo* info = aCustomElement->NodeInfo();
>+
>+    // Make sure we get the correct definition in case the element
>+    // is a extended custom element e.g. <button is="x-button">.
>+    nsCOMPtr<nsIAtom> typeAtom = elementData ?
>+      elementData->mType.get() : info->NameAtom();
>+
>+    definition = mCustomDefinitions.Get(typeAtom);
>+    if (!definition || definition->mLocalName != info->NameAtom()) {
>+      return;
>+    }
>+  }
What has this to do with this bug?
(And are we going to support customized built-in elements? I don't know our plans for that.)

 
>-  // Step 3 and Step 4.
>-  // TODO: Bug 1334051 - Implement list of observed attributes for custom elements' attributeChanged callbacks
>+  // Step 3.
>+  nsDOMAttributeMap* attributes = aElement->Attributes();
>+  for (uint32_t i = 0; i < attributes->Length(); i++) {
>+    Attr* attr = attributes->Item(i);
>+    nsAutoString name, value, namespaceURI;
>+    attr->GetLocalName(name);
>+    attr->GetValue(value);
>+    attr->GetNamespaceURI(namespaceURI);
>+
>+    LifecycleCallbackArgs args = {
>+      name, NullString(), (value.IsEmpty() ? NullString() : value),
>+      (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
>+    };
>+    EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement, &args,
>+                             aDefinition);
>+  }
This code should not use Attributes(), since that is slow because it creates DOMAttributeMap and all the objects for attributes.
You want probably nsIContent::GetAttrCount() and GetAttrInfoAt
 
>+  // The list of attributes that this custom element observes.
>+  nsTArray<nsString> mObservedAttributes;
Hmm, I think we should store an array of nsCOMPtr<nsIAtom> objects.
Comparing atoms is way faster than nsStrings.
We use atoms in MutationObserver code http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/dom/base/nsDOMMutationObserver.h#339

Comment 20

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #19)
> > void
> > CustomElementRegistry::EnqueueLifecycleCallback(nsIDocument::ElementCallbackType aType,
> >                                                 Element* aCustomElement,
> >                                                 LifecycleCallbackArgs* aArgs,
> >                                                 CustomElementDefinition* aDefinition)
> > {
> >+  RefPtr<CustomElementData> elementData = aCustomElement->GetCustomElementData();
> >+  MOZ_ASSERT(elementData, "CustomElementData should exist");
> >+
> >+  // Let DEFINITION be ELEMENT's definition
> >+  CustomElementDefinition* definition = aDefinition;
> >+  if (!definition) {
> >+    mozilla::dom::NodeInfo* info = aCustomElement->NodeInfo();
> >+
> >+    // Make sure we get the correct definition in case the element
> >+    // is a extended custom element e.g. <button is="x-button">.
> >+    nsCOMPtr<nsIAtom> typeAtom = elementData ?
> >+      elementData->mType.get() : info->NameAtom();
> >+
> >+    definition = mCustomDefinitions.Get(typeAtom);
> >+    if (!definition || definition->mLocalName != info->NameAtom()) {
> >+      return;
> >+    }
> >+  }
> What has this to do with this bug?

This is because aDefinition may be null, so we need to get the custom element definition from mDefinitions. Actually, we are planning to add the custom element definition to each element, then we can get it from the element itself.

> (And are we going to support customized built-in elements? I don't know our
> plans for that.)

Yes, we do plan to support customized built-in elements as well.

> 
>  
> >-  // Step 3 and Step 4.
> >-  // TODO: Bug 1334051 - Implement list of observed attributes for custom elements' attributeChanged callbacks
> >+  // Step 3.
> >+  nsDOMAttributeMap* attributes = aElement->Attributes();
> >+  for (uint32_t i = 0; i < attributes->Length(); i++) {
> >+    Attr* attr = attributes->Item(i);
> >+    nsAutoString name, value, namespaceURI;
> >+    attr->GetLocalName(name);
> >+    attr->GetValue(value);
> >+    attr->GetNamespaceURI(namespaceURI);
> >+
> >+    LifecycleCallbackArgs args = {
> >+      name, NullString(), (value.IsEmpty() ? NullString() : value),
> >+      (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
> >+    };
> >+    EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement, &args,
> >+                             aDefinition);
> >+  }
> This code should not use Attributes(), since that is slow because it creates
> DOMAttributeMap and all the objects for attributes.
> You want probably nsIContent::GetAttrCount() and GetAttrInfoAt

Will do.

>  
> >+  // The list of attributes that this custom element observes.
> >+  nsTArray<nsString> mObservedAttributes;
> Hmm, I think we should store an array of nsCOMPtr<nsIAtom> objects.
> Comparing atoms is way faster than nsStrings.
> We use atoms in MutationObserver code
> http://searchfox.org/mozilla-central/rev/
> 13148faaa91a1c823a7d68563d9995480e714979/dom/base/nsDOMMutationObserver.h#339

Sounds good, will use array of nsCOMPtr<nsIAtom> instead.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v2.

Addressed review comment 19:
- Use nsIContent::GetAttrCount() and GetAttrInfoAt instead of Attributes()
- Use nsCOMArray<nsIAtom> instead of nsTArray<nsString>
- Check if web components pref is enabled before doing anything in attribute changed functions

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8898221 [details] [diff] [review]
Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v2.

>+            JSString *attrJSStr = attribute.toString();
* goes with the type


>+  // Step 3.
>+  uint32_t count = aElement->GetAttrCount();
>+  for (uint32_t i = 0; i < count; i++) {
>+    mozilla::dom::BorrowedAttrInfo info = aElement->GetAttrInfoAt(i);
>+
>+    const nsAttrName* name = info.mName;
>+    nsIAtom* attrName = name->LocalName();
>+    int32_t namespaceID = name->NamespaceID();
>+
>+    nsAutoString attrValue, namespaceURI;
>+    info.mValue->ToString(attrValue);
>+    nsContentUtils::NameSpaceManager()->GetNameSpaceURI(namespaceID,
>+                                                        namespaceURI);
>+
>+    LifecycleCallbackArgs args = {
>+      nsDependentAtomString(attrName),
>+      NullString(),
>+      (attrValue.IsEmpty() ? NullString() : attrValue),
>+      (namespaceURI.IsEmpty() ? NullString() : namespaceURI)
>+    };
>+    EnqueueLifecycleCallback(nsIDocument::eAttributeChanged, aElement, &args,
>+                             aDefinition);
>+  }
Hmm, isn't this a bit slow, and useless work is done if definition doesn't have any observed attributes.
Could we possibly get the definition first and if there is such, check whether there are observed attributes, and only if so, 
do all this work.

>+  if (nsContentUtils::IsWebComponentsEnabled()) {
>+    nsIDocument* ownerDoc = OwnerDoc();
>+    if (ownerDoc && GetCustomElementData()) {
>+      nsCOMPtr<nsIAtom> oldValueAtom;
>+      if (oldValue) {
>+        oldValueAtom = oldValue->GetAsAtom();
>+      } else {
>+        // If there is no old value, get the value of the uninitialized
>+        // attribute that was swapped with aParsedValue.
>+        oldValueAtom = aParsedValue.GetAsAtom();
>+      }
>+      nsCOMPtr<nsIAtom> newValueAtom = valueForAfterSetAttr.GetAsAtom();
>+      nsAutoString ns;
>+      nsContentUtils::NameSpaceManager()->GetNameSpaceURI(aNamespaceID, ns);
>+
>+      LifecycleCallbackArgs args = {
>+        nsDependentAtomString(aName),
>+        aModType == nsIDOMMutationEvent::ADDITION ?
>+          NullString() : nsDependentAtomString(oldValueAtom),
>+        nsDependentAtomString(newValueAtom),
>+        (ns.IsEmpty() ? NullString() : ns)
>+      };
>+
>+      nsContentUtils::EnqueueLifecycleCallback(
>+        ownerDoc, nsIDocument::eAttributeChanged, this, &args);
>     }
Also here it would be nice to check first whether we're actually observing the attribute before
doing all the work.

r+, but file a followup to optimize this. Or just re-architect the code a bit in this bug and ask review again. Up to you.

Comment 23

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #22)
> r+, but file a followup to optimize this. Or just re-architect the code a
> bit in this bug and ask review again. Up to you.

Ok, I can do it in this bug, but I'll be away next Monday, so may ask review again after that. :)

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: optimization.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900172 [details] [diff] [review]
Part 3: optimization.

>   if (nsContentUtils::IsWebComponentsEnabled()) {
>     nsIDocument* ownerDoc = OwnerDoc();
>-    if (ownerDoc && GetCustomElementData()) {
>+    if (ownerDoc && GetCustomElementData() &&
Not about this bug, but feel free to drop the null check for ownerDoc. OwnerDoc() never ever returns null.


>+nsContentUtils::IsInObservedAttributeList(Element* aCustomElement,
>+                                          nsIAtom* aAttrName,
>+                                          CustomElementDefinition* aDefinition)
>+{
>+  if (aDefinition) {
>+    return aDefinition->IsInObservedAttributeList(aAttrName);
>+  }
>+
>+  CustomElementData* data = aCustomElement->GetCustomElementData();
>+  MOZ_ASSERT(data);
>+
>+  nsString extType = nsDependentAtomString(data->mType);
>+  NodeInfo *ni = aCustomElement->NodeInfo();
>+
>+  CustomElementDefinition* definition =
>+    LookupCustomElementDefinition(aCustomElement->OwnerDoc(), ni->LocalName(),
>+                                  ni->NamespaceID(),
>+                                  extType.IsEmpty() ? nullptr : &extType);
>+
Hmm, so we do a lookup here and then possibly later in EnqueueLifecycleCallback if we are observing the attribute.
(it is especially bad atm, but even if we put definition to extended slots, it would be still several indirect accesses)
Could the method be something like CustomElementDefinition* GetElementDefinitionIfObservingAttr
and then the return value could be passed to EnqueueLifecycleCallback. If null was returned, attributeChanged wouldn't be queued.

Comment 26

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: optimization, v2.

Addressed review comment 25.

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add namespace in attributeChangedCallback.

added reviewer's name.

Comment 28

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Invoke attributeChangedCallback only if attribute is in the observed attribute list, v3.

Combine Part 2 and Part 3 and add reviewer's name.

Comment 29

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b00a5f56b6af3a105ce6d3a95ba2a70be303c0da&group_state=expanded

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4481da121c0
Part 1: Include namespace in attributeChangedCallback. f=jdai, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/81bbf995e743
Part 2: Invoke attributeChangedCallback only if attribute name is in the observed attribute list. f=jdai, r=smaug

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e4481da121c0
https://hg.mozilla.org/mozilla-central/rev/81bbf995e743