Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($compile): work around Firefox DocumentFragment bug #16615

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Jun 26, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Browser bug work-around.

What is the current behavior? (You can also link to an open issue here)
DOM nodes passed to compilationGenerator() will eventually be wrapped in jqLite, when the compilation actually happens. In Firefox 60+, there seems to be a DocumentFragment-related bug that sometimes causes the childNodes to be empty at the time the compilation happens.

What is the new behavior (if this is a feature change)?
This commit works around this bug by eagerly wrapping childNodes in jqLite.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:
Fixes #16607.

NOTE:
The wrapped nodes have references to their DocumentFragment container. This is "by design", since we want to be able to traverse the nodes via nextSibling (in order to correctly handle multi-element directives).

Once the nodes are compiled, they will be either moved to a new container element or the jqLite wrapper is release making them eligible for garbage collection. In both cases, the original DocumentFragment container should be eligible for garbage collection too.

Sorry, something went wrong.

@jbedard
Copy link
Collaborator

jbedard commented Jun 26, 2018

Did you find any FF bug to reference?

@gkalpak
Copy link
Member Author

gkalpak commented Jun 26, 2018

No, I couldn't find anything 😞

Copy link
Contributor

@Narretz Narretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except that this is a fix instead of a refactor.

DOM nodes passed to `compilationGenerator()` will eventually be wrapped
in `jqLite`, when the compilation actually happens. In Firefox 60+,
there seems to be a `DocumentFragment`-related bug that sometimes causes
the `childNodes` to be empty at the time the compilation happens.

This commit works around this bug by eagerly wrapping `childNodes` in
`jqLite`.

NOTE:
The wrapped nodes have references to their `DocumentFragment` container.
This is "by design", since we want to be able to traverse the nodes via
`nextSibling` (in order to correctly handle multi-element directives).

Once the nodes are compiled, they will be either moved to a new
container element or the `jqLite` wrapper is release making them
eligible for garbage collection. In both cases, the original
`DocumentFragment` container should be eligible for garbage collection
too.

Fixes angular#16607
@gkalpak gkalpak force-pushed the fix-compile-ff-DocumentFragment-bug branch from add51b2 to 0a943b8 Compare July 4, 2018 11:29
@gkalpak gkalpak changed the title refactor($compile): work around Firefox DocumentFragment bug fix($compile): work around Firefox DocumentFragment bug Jul 4, 2018
@gkalpak
Copy link
Member Author

gkalpak commented Jul 4, 2018

Updated the commit message.

@Narretz
Copy link
Contributor

Narretz commented Jul 4, 2018

Just thought about this more ... should there be a // Support: Firefox 60 comment in the code? Ofc it's unlikely that we will touch this again even if the bug is fixed, but you never know.

@gkalpak
Copy link
Member Author

gkalpak commented Jul 4, 2018

I thought about this. I decided not to include a comment, since we are really not doing something special (in the sense that compile will eventually wrap the nodes in jqLite(); we are just doing it sooner, hence the original refactor type). But I was on the fence - happy to include a comment if you think it is more appropriate.

@Narretz
Copy link
Contributor

Narretz commented Jul 9, 2018

Ok, let's jsut merge as is

@gkalpak gkalpak closed this in bfdc917 Jul 9, 2018
gkalpak added a commit that referenced this pull request Jul 9, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
DOM nodes passed to `compilationGenerator()` will eventually be wrapped
in `jqLite`, when the compilation actually happens. In Firefox 60+,
there seems to be a `DocumentFragment`-related bug that sometimes causes
the `childNodes` to be empty at the time the compilation happens.

This commit works around this bug by eagerly wrapping `childNodes` in
`jqLite`.

NOTE:
The wrapped nodes have references to their `DocumentFragment` container.
This is "by design", since we want to be able to traverse the nodes via
`nextSibling` (in order to correctly handle multi-element directives).

Once the nodes are compiled, they will be either moved to a new
container element or the `jqLite` wrapper is release making them
eligible for garbage collection. In both cases, the original
`DocumentFragment` container should be eligible for garbage collection
too.

Fixes #16607

Closes #16615
@gkalpak gkalpak deleted the fix-compile-ff-DocumentFragment-bug branch July 9, 2018 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloneLinkingFn sometimes gets an empty clone with multiple slots
4 participants