Closed Bug 1629761 Opened 4 months ago Closed 4 months ago

Custom element lifecycle callback ordering breaking change

Categories

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

74 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- wontfix
firefox76 --- fixed
firefox77 --- fixed

People

(Reporter: rajsite, Assigned: edgar)

References

(Blocks 2 open bugs, Regression, )

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.163 Safari/537.36

Steps to reproduce:

In Firefox 74 we have found that web applications created with our framework are no longer running correctly due to errors during load of the page. Upon investigation it was found that the errors were due to the connectedCallback of a custom element firing during the execution of the constructor instead of the behavior of previous versions of Firefox where the connectedCallback fires after the constructor has finished execution.

We narrowed it down to a code path that can result in setAttribute being called in the constructor of a custom element. It appears that as of Firefox 74, if setAttribute is called in the constructor of a custom element it will synchronously (before the callstack of the constructor completes) call the connectedCallback.

Actual results:

Extract the attached example or see the following hosted example: https://bug-firefox-74-ce-lifecycle.glitch.me/ and open the browser console.

console output in Firefox 74:

constructor - before setAttribute
connected
attributeChanged
constructor - after setAttribute

And the following for Firefox < 74 (68.7.0esr) and Chrome 80.0.3987.163:

constructor - before setAttribute
constructor - after setAttribute
connected

Note that in previous versions of Firefox and in Chrome that the connected callback runs after the constructor has finished and that the invalid setAttribute call during the constructor does not trigger attributeChanged during the constructor execution.

Also note that neither script prints a warning to the console about using setAttribute during execution of the constructor. This appears to be due to the scripts being defer loaded. If the scripts are loaded as classic scripts you see the following messages:

  • Chrome gives: Uncaught DOMException: Failed to construct 'CustomElement': The result must not have attributes
  • Firefox gives: NotSupportedError: Operation is not supported

For a complete demonstration of the issue in a live application you can refer to the following: https://rawcdn.githack.com/rajsite/webvi-experiments/51bcc4cf42b7b237f0e75c0354bb656b24540c82/Snake/index.html

Note that in Firefox 74 and Firefox 75 that an error message appears in the browser console and the contents of graph do not load.

Expected results:

While setting attributes in a constructor is not a supported behavior of custom elements (The element must not gain any attributes or children), I would expect the behavior to be consistent between browsers whether that was to error, avoid the attributeChangedCallback, etc.

The behavior of triggering the connectedCallback before the execution of the constructor completed was surprising and the lack of error messages or warnings in defer loaded scripts made it a difficult issue to identify.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

Thanks a lot Alice :)

Edgar, can you take a look at this regression? It may or may not be a bug in the spec I guess.

Flags: needinfo?(echen)
Status: UNCONFIRMED → NEW
Ever confirmed: true

The original assumption of "custom-element-definition is only available on the
element whose custom-element-state is customized" is no longer true after
bug 1610054 along with the spec changings in https://github.com/whatwg/html/pull/5126.

Priority: -- → P2
Attachment #9140749 - Attachment is obsolete: true
Flags: needinfo?(echen)
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8129abc4c2d
Should not invoke attributeChangedCallback for the attribute that is changed during upgrading; r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/22972 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

This following behavior was also in the description:

Also note that neither script prints a warning to the console about using setAttribute during execution of the constructor. This appears to be due to the scripts being defer loaded. If the scripts are loaded as classic scripts you see the following messages:

Chrome gives: Uncaught DOMException: Failed to construct 'CustomElement': The result must not have attributes
Firefox gives: NotSupportedError: Operation is not supported

Does that also sound like behavior worth addressing? Should I create a separate bug if it is?

Upstream PR merged by moz-wptsync-bot

(In reply to Milan from comment #11)

This following behavior was also in the description:

Also note that neither script prints a warning to the console about using setAttribute during execution of the constructor. This appears to be due to the scripts being defer loaded. If the scripts are loaded as classic scripts you see the following messages:

Chrome gives: Uncaught DOMException: Failed to construct 'CustomElement': The result must not have attributes
Firefox gives: NotSupportedError: Operation is not supported

Does that also sound like behavior worth addressing? Should I create a separate bug if it is?

This is an expected behaviour which is defined in spec, see https://dom.spec.whatwg.org/#concept-create-element.
If the custom element definition is registered first then running elemenet creation and the creation is from parser, it falls into step 6.1 where throws NotSupportedError DOMException if attribute list is not empty after running constructor, i.e. step 6.1.5.

Thanks for finding the spec discussion of the error behavior Edgar! :)

Comment on attachment 9140729 [details]
Bug 1629761 - Should not invoke attributeChangedCallback for the attribute that is changed during upgrading;

Beta/Release Uplift Approval Request

  • User impact if declined: This is a regression of bug 1610054 which introduces an unexpected changes in custom element callbacks and might possible break the page that uses customElement feature. Though I haven't noticed other breakage reports, but it will be good if we could also have this fix in 76.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Should be low, the changes match with the spec, and could pass existing tests.
  • String changes made/needed: None
Flags: needinfo?(echen)
Attachment #9140729 - Flags: approval-mozilla-beta?

Comment on attachment 9140729 [details]
Bug 1629761 - Should not invoke attributeChangedCallback for the attribute that is changed during upgrading;

Improves spec compliance with custom elements. Approved for 76.0b6.

Attachment #9140729 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.