Skip to content

Selector:Manipulation: Fix DOM manip within template contents #5158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

mgol
Copy link
Member

@mgol mgol commented Nov 14, 2022

Summary

The <template/> element contents property is a document fragment that may
have a null documentElement. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in jQuery.contains. As it turns out, we don't need
to query the supposed container documentElement if it has the
Node.DOCUMENT_NODE (9) nodeType; we can call .contains() directly on
the document. That avoids the crash.

Fixes gh-5147

-18 bytes

Checklist

mgol added 3 commits November 14, 2022 17:57
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains`. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

Fixes jquerygh-5147
@mgol mgol added Manipulation Selector Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Nov 14, 2022
@mgol mgol added this to the 4.0.0 milestone Nov 14, 2022
@mgol mgol self-assigned this Nov 14, 2022
@mgol mgol removed Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Nov 14, 2022
@mgol mgol merged commit 10a1d54 into jquery:main Nov 14, 2022
@mgol mgol deleted the template-manip-fixes branch November 14, 2022 17:37
mgol added a commit that referenced this pull request Nov 14, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains`. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

Fixes gh-5147
Closes gh-5158
mgol added a commit to mgol/sizzle that referenced this pull request Nov 15, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The above behavior made `Sizzle.contains` crash when run on certain elements
within the `<template/>` element. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

However, we still need to fall back to `documentElement` if one is
defined as IE <9 have a broken `.contains()` on the document.

Fixes jquery/jquery#5147
Ref jquery/jquery#5158
@mgol
Copy link
Member Author

mgol commented Nov 15, 2022

Landed on main in 3299236. The 3.x-stable part of the fix needs to first happen in Sizzle.

mgol added a commit to mgol/jquery that referenced this pull request Nov 15, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains` which is an alias for
`Sizzle.contains` in jQuery 3.x.

The Sizzle fix is at jquery/sizzle#490. This PR pulls the new Sizzle
version into jQuery's `3.x-stable` branch and backports one test from
jquerygh-5158.

Fixes jquerygh-5147
Ref jquery/sizzle/pull/490
Ref jquerygh-5158
mgol added a commit to jquery/sizzle that referenced this pull request Nov 16, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The above behavior made `Sizzle.contains` crash when run on certain elements
within the `<template/>` element. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

However, we still need to fall back to `documentElement` if one is
defined as IE <9 have a broken `.contains()` on the document.

Fixes jquery/jquery#5147
Closes gh-490
Ref jquery/jquery#5158
mgol added a commit to mgol/jquery that referenced this pull request Nov 16, 2022
mgol added a commit to mgol/jquery that referenced this pull request Nov 16, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains` which is an alias for
`Sizzle.contains` in jQuery 3.x.

The Sizzle fix is at jquery/sizzle#490. This PR pulls the new Sizzle
version into jQuery's `3.x-stable` branch and backports one test from
jquerygh-5158.

Fixes jquerygh-5147
Closes jquerygh-5159
Ref jquery/sizzle#490
Ref jquerygh-5158
mgol added a commit to mgol/jquery that referenced this pull request Nov 16, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains` which is an alias for
`Sizzle.contains` in jQuery 3.x.

The Sizzle fix is at jquery/sizzle#490, released in Sizzle `2.3.8`. This
version of Sizzle is included in the parent commit.

Fixes jquerygh-5147
Closes jquerygh-5159
Ref jquery/sizzle#490
Ref jquerygh-5158
mgol added a commit to mgol/jquery that referenced this pull request Nov 16, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains` which is an alias for
`Sizzle.contains` in jQuery 3.x.

The Sizzle fix is at jquery/sizzle#490, released in Sizzle `2.3.8`. This
version of Sizzle is included in the parent commit.

A fix similar to the one from jquerygh-5158 has also been applied here to the
`selector-native` version.

Fixes jquerygh-5147
Closes jquerygh-5159
Ref jquery/sizzle#490
Ref jquerygh-5158
mgol added a commit that referenced this pull request Nov 16, 2022
mgol added a commit that referenced this pull request Nov 16, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains` which is an alias for
`Sizzle.contains` in jQuery 3.x.

The Sizzle fix is at jquery/sizzle#490, released in Sizzle `2.3.8`. This
version of Sizzle is included in the parent commit.

A fix similar to the one from gh-5158 has also been applied here to the
`selector-native` version.

Fixes gh-5147
Closes gh-5159
Ref jquery/sizzle#490
Ref gh-5158
mgol added a commit to mgol/jquery that referenced this pull request Nov 17, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains`. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

Fixes jquerygh-5147
Closes jquerygh-5158

(cherry picked from commit 3299236)
mgol added a commit to mgol/jquery that referenced this pull request Dec 1, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains`. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

Fixes jquerygh-5147
Closes jquerygh-5158

(cherry picked from commit 3299236)
mgol added a commit to mgol/jquery that referenced this pull request Dec 13, 2022
The `<template/>` element `contents` property is a document fragment that may
have a `null` `documentElement`. In Safari 16 this happens in more cases due
to recent spec changes - in particular, even if that document fragment is
explicitly adopted into an outer document. We're testing both of those cases
now.

The crash used to happen in `jQuery.contains`. As it turns out, we don't need
to query the supposed container `documentElement` if it has the
`Node.DOCUMENT_NODE` (9) `nodeType`; we can call `.contains()` directly on
the `document`. That avoids the crash.

Fixes jquerygh-5147
Closes jquerygh-5158

(cherry picked from commit 3299236)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Safari 16 crashes on in-template DOM manipulation
3 participants