Skip to content

Commit

Permalink
🐛 CEv1: Fix innerHTML patch in Safari <= 9 and maybe Yandex (#28086)
Browse files Browse the repository at this point in the history
* CEv1: Fix innerHTML patch in Safari <= 9

Old Safari has an interesting definition for `Element.p.innerHTML`: the descriptor is non-configurable and the `set` setter is undefined. In the old code, trying to patch this setter throws an error.

This leaves the document in a broken state. The first CE to be defined will not properly upgrade the elements that existed were already parsed on the page. Additionally, `innerHTML` will not properly sync-upgrade CEs that are parsed from the string, but they will async-upgrade via the `MutationObserver`.

Skipping this `innerHTML` patch shouldn't hurt, since we'll still get the async-upgrade. Safari 9 is sooo old, all we really need here is a best-effort "it kinda works".

* Add potential fix for Yandex not having a innerHTML descriptor

* Add tests for innerHTML patch

* Add browser names to test cases

* Fix test skipping
  • Loading branch information
jridgewell authored Apr 29, 2020
1 parent c6e3be8 commit 67815dc
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 12 deletions.
22 changes: 12 additions & 10 deletions src/polyfills/custom-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -684,16 +684,18 @@ function installPatches(win, registry) {
'innerHTML'
);
}
const innerHTMLSetter = innerHTMLDesc.set;
innerHTMLDesc.set = function (html) {
innerHTMLSetter.call(this, html);
registry.upgrade(this);
};
Object.defineProperty(
/** @type {!Object} */ (innerHTMLProto),
'innerHTML',
innerHTMLDesc
);
if (innerHTMLDesc && innerHTMLDesc.configurable) {
const innerHTMLSetter = innerHTMLDesc.set;
innerHTMLDesc.set = function (html) {
innerHTMLSetter.call(this, html);
registry.upgrade(this);
};
Object.defineProperty(
/** @type {!Object} */ (innerHTMLProto),
'innerHTML',
innerHTMLDesc
);
}
}

/**
Expand Down
84 changes: 83 additions & 1 deletion test/unit/test-polyfill-custom-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,89 @@
* limitations under the License.
*/

import {copyProperties} from '../../src/polyfills/custom-elements';
import {copyProperties, install} from '../../src/polyfills/custom-elements';

describes.realWin(
'install patches',
{skipCustomElementsPolyfill: true},
(env) => {
let win;
let innerHTMLProto;

function getInnerHtmlProto(win) {
let innerHTMLProto = win.Element.prototype;
if (!Object.getOwnPropertyDescriptor(innerHTMLProto, 'innerHTML')) {
innerHTMLProto = win.HTMLElement.prototype;
if (!Object.getOwnPropertyDescriptor(innerHTMLProto, 'innerHTML')) {
return null;
}
}
return innerHTMLProto;
}

before(function () {
if (!getInnerHtmlProto(window)) {
this.skipTest();
}
});

beforeEach(function () {
win = env.win;
innerHTMLProto = getInnerHtmlProto(win);

// We want to test the full polyfill.
delete win.Reflect;
delete win.customElements;
});

it('handles non-configurable innerHTML accessor (Safari 9)', () => {
// Use strict is important, as strict mode code will throw an error when
// trying to redefine a non-configurable property.
'use strict';

Object.defineProperty(innerHTMLProto, 'innerHTML', {configurable: false});
install(win, function () {});

class Test extends win.HTMLElement {}

expect(() => {
win.customElements.define('x-test', Test);
}).not.to.throw();
});

it('handles missing innerHTML descriptor (Yandex)', () => {
// Use strict is important, as strict mode code will throw an error when
// trying to redefine a non-configurable property.
'use strict';

delete innerHTMLProto.innerHTML;
install(win, function () {});

class Test extends win.HTMLElement {}

expect(() => {
win.customElements.define('x-test', Test);
}).not.to.throw();
});

it('handles innerHTML descriptor on HTMLElement (IE11)', () => {
// Use strict is important, as strict mode code will throw an error when
// trying to redefine a non-configurable property.
'use strict';

const desc = Object.getOwnPropertyDescriptor(innerHTMLProto, 'innerHTML');
delete innerHTMLProto.innerHTML;
Object.defineProperty(win.HTMLElement.prototype, 'innerHTML', desc);
install(win, function () {});

class Test extends win.HTMLElement {}

expect(() => {
win.customElements.define('x-test', Test);
}).not.to.throw();
});
}
);

describes.fakeWin('copyProperties', {}, () => {
it('copies own properties from proto object', () => {
Expand Down
3 changes: 2 additions & 1 deletion testing/describes.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ export const fakeWin = describeEnv((spec) => [
* @param {string} name
* @param {{
* fakeRegisterElement: (boolean|undefined),
* skipCustomElementsPolyfill: (boolean|undefined),
* amp: (boolean|!AmpTestSpec|undefined),
* }} spec
* @param {function({
Expand Down Expand Up @@ -648,7 +649,7 @@ class RealWinFixture {
Object.defineProperty(win, 'customElements', {
get: () => customElements,
});
} else {
} else if (!spec.skipCustomElementsPolyfill) {
// The anonymous class parameter allows us to detect native classes
// vs transpiled classes.
installCustomElements(win, class {});
Expand Down

0 comments on commit 67815dc

Please sign in to comment.