Closed Bug 1334051 Opened 4 years ago Closed 3 years ago

Implement list of observed attributes for custom elements' attributeChanged callbacks

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: edgar, Assigned: jessica)

References

Details

(Whiteboard: dom-ce-m3)

Attachments

(2 files, 10 obsolete files)

4.93 KB, patch
jessica
: review+
Details | Diff | Splinter Review
25.36 KB, patch
jessica
: review+
Details | Diff | Splinter Review
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
Duplicate of this bug: 1357730
Assignee: nobody → jjong
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.
(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.
rebased on top of bug 1299363.
Attachment #8892351 - Attachment is obsolete: true
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.
Attachment #8893712 - Flags: feedback?(jdai)
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.
Attachment #8893715 - Flags: feedback?(jdai)
Attachment #8893712 - Flags: feedback?(jdai) → feedback+
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.
Attachment #8893715 - Flags: feedback?(jdai) → feedback+
(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.
rebased.
Attachment #8893712 - Attachment is obsolete: true
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
(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 on attachment 8896894 [details] [diff] [review]
Part 1: add namespace in attributeChangedCallback.

Hi Olli, do you think you can help review this? :)
Attachment #8896894 - Flags: review?(bugs)
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.
Attachment #8897729 - Flags: review?(bugs)
Attachment #8896894 - Flags: review?(bugs) → review+
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
Attachment #8897729 - Flags: review?(bugs) → review-
(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.
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
Attachment #8897729 - Attachment is obsolete: true
Attachment #8898221 - Flags: review?(bugs)
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.
Attachment #8898221 - Flags: review?(bugs) → review+
(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. :)
Attached patch Part 3: optimization. (obsolete) — Splinter Review
Attachment #8900172 - Flags: review?(bugs)
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.
Attachment #8900172 - Flags: review?(bugs) → review-
Attached patch Part 3: optimization, v2. (obsolete) — Splinter Review
Addressed review comment 25.
Attachment #8900172 - Attachment is obsolete: true
Attachment #8900615 - Flags: review?(bugs)
Attachment #8900615 - Flags: review?(bugs) → review+
added reviewer's name.
Attachment #8896894 - Attachment is obsolete: true
Attachment #8901681 - Flags: review+
Combine Part 2 and Part 3 and add reviewer's name.
Attachment #8898221 - Attachment is obsolete: true
Attachment #8900615 - Attachment is obsolete: true
Attachment #8901682 - Flags: review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4481da121c0
https://hg.mozilla.org/mozilla-central/rev/81bbf995e743
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.