Bug 258282 – REGRESSION (264594@main): Broke custom element built-in polyfill by making form properties non deletable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

Hi,

Apple never wanted to implement the full custom element spec with the built-in element extensions. Devs had to rely on polyfill for that (especially the excellent ungap/@custom-elements: https://github.com/ungap/custom-elements)

This has worked well for ages, but starting from Safari TP172, the polyfill broke. After discussing with polyfill authors it appears that Safari made some properties non deletable (https://github.com/ungap/custom-elements/issues/18).

This shows how important it is for Apple to fully implement the spec here (even if they don't like it), as relying on a polyfill can cause such issues. We have thousands of stores relying on this that will suddenly break on next version of Safari.

Apple refusing to implement the full spec is one thing, but at least Apple should ensure to not break popular polyfills, as thousands of websites are relying on this.

Can something be made to at least revert the change that broke the polyfill, and re-consider again the implementation of customized built-in?

[Alexey Proskuryakov]

Thank you for the report. I expect us to focus on fixing the regression here. I don't know about the history around customized built-in, but it would be confusing and ineffective to track that in the same bug as fixing a recent regression.

[Radar WebKit Bug Importer]

<rdar://problem/111008826>

[Andrea Giammarchi]

Polyfill author here. I am not sure if it's a regression or an intended behavior but the form gets an own `0` property which I believe is the button within the form (I can't test as I'm on WebKit non TP)

Live example here: https://codepen.io/bakura10/pen/mdQERdX

Before I explain the fix: some class might define accessors and when a node gets upgraded, these accessors won't trigger unless removed and re-attached (to trigger accessors at the prototype level) later on.

The current live demo reveals that the form gets an own, enumerable, property as index `0` and when the upgrade dance kicks in, this can't be deleted or an error is thrown.

I believe the polyfill fix was necessary regardless to avoid this kind of situation in the future, even if this never happened before (builtin don't usually get own enumerable properties out of the blue) but it was surprising to find that index `0` attached, although I am sure I shouldn't delete it and re-attach it, if that's not meant to ever happen.

Hopefully this helps clarifying what you should do or decide if it's a regression or not, yet it would be extremely helpful if you folks could test that polyfill when adding new features as part of your tests ... it's been around for years, there are business depending on it, and beside adding a +1 to the request "please just add builtin extends and let's move forward" the polyfill doesn't change much in time, if ever, as it's rock solid and working very well so it shouldn't add particular burden to your flow.

Thank you in advance for eventually considering my hint.

[Anne van Kesteren]

This seems like https://github.com/WebKit/WebKit/commit/12544d418716.

I suspect this polypill relied on a bug in WebKit and broke as a result of WebKit becoming more standards compliant and more interoperable with other browsers.

Perhaps we should consider a quirk for this polyfill.

[Ryosuke Niwa]

(In reply to Andrea Giammarchi from comment #3)
> Polyfill author here. I am not sure if it's a regression or an intended
> behavior but the form gets an own `0` property which I believe is the button
> within the form (I can't test as I'm on WebKit non TP)
> 
> Live example here: https://codepen.io/bakura10/pen/mdQERdX

When I open this URL in STP172, I see "CONNECTED" in console and there is no other error emitted. What exactly is the symptom I should be looking for?

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Andrea Giammarchi from comment #3)
> > Polyfill author here. I am not sure if it's a regression or an intended
> > behavior but the form gets an own `0` property which I believe is the button
> > within the form (I can't test as I'm on WebKit non TP)
> > 
> > Live example here: https://codepen.io/bakura10/pen/mdQERdX
> 
> When I open this URL in STP172, I see "CONNECTED" in console and there is no
> other error emitted. What exactly is the symptom I should be looking for?

This happens because I tried with an updated polyfill that Andrea created. The previous version (the one that was using for years) is the one that broke.

I re-updated the CodePen to use the previous polyfill version. You should now exhibit the issue again.

[Karl Dubost]

> Hopefully this helps clarifying what you should do or decide if it's a regression or not, yet it would be extremely helpful if you folks could test that polyfill when adding new features as part of your tests

Just as a note. That is partly the goal of STP. 
Being able to test things and discover what eventually breaks so it can be fixed for the release versions. 

It's not perfect but that helps to avoid pretty hard falls.

[Andrea Giammarchi]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Andrea Giammarchi from comment #3)
> > Polyfill author here. I am not sure if it's a regression or an intended
> > behavior but the form gets an own `0` property which I believe is the button
> > within the form (I can't test as I'm on WebKit non TP)
> > 
> > Live example here: https://codepen.io/bakura10/pen/mdQERdX
> 
> When I open this URL in STP172, I see "CONNECTED" in console and there is no
> other error emitted. What exactly is the symptom I should be looking for?

The polyfill was fixed within the same day so you're seeing the already fixed version. Change the polyfill version to its previous patch and see an error is thrown.

[Andrea Giammarchi]

(In reply to Anne van Kesteren from comment #4)
> This seems like https://github.com/WebKit/WebKit/commit/12544d418716.
> 
> I suspect this polypill relied on a bug in WebKit and broke as a result of
> WebKit becoming more standards compliant and more interoperable with other
> browsers.
> 
> Perhaps we should consider a quirk for this polyfill.

That's very possible and indeed I believe my fix was needed regardless.

(In reply to Karl Dubost from comment #7)
> > Hopefully this helps clarifying what you should do or decide if it's a regression or not, yet it would be extremely helpful if you folks could test that polyfill when adding new features as part of your tests
> 
> Just as a note. That is partly the goal of STP. 
> Being able to test things and discover what eventually breaks so it can be
> fixed for the release versions. 
> 
> It's not perfect but that helps to avoid pretty hard falls.

It would be close to perfect if I had a way to test STP on Linux, my daily OS, otherwise I need to rely in people being able to catch these kind of errors on their Macs ... luckily this was the case, but the bug was after a <form> element, I hope no other issues will come out of any other builtin element in the future, which is why in an ideal world Safari would just ship builtin extends so that nobody cna possibly ever get hurt by these kind of quirks hard to test or debug from my side.

I am going to check if WebKit TP is available somewhere but we all know Safari is not necessarily 1:1 with what I can test on GNOME.

[Andrea Giammarchi]

FYI I've just realized there's an Epiphany.Devel flatpak file here [1] so *maybe* after all I can test at least what's commonly shared across WebKit and Safari and builtin / DOM stuff is hopefully the same ... or at least I could indeed verify the bug which has been fixed in latest patch of the polyfill.

OK then, rant over, I'll try to keep the file updated but I couldn't find it in flathub ... maybe an official entry in there would help others too.

Regards.

[1] https://webkit.org/downloads/

[Ryosuke Niwa]

codepen is stil loading
<script src="https://unpkg.com/@ungap/custom-elements@1.2.0/index.js"></script>
and the output is the same. Just "CONNECTED".

Could you create a new one that actually reproduces the issue? Better yet, create a HTML File and attach on Bugzilla.

[Andrea Giammarchi]

with 1.2.0 I can reproduce the issue in Epiphany TP so I am not sure what you are testing ... the error is:

[Error] Unhandled Promise Rejection: TypeError: Unable to delete property.
	expando (index.js:271)
	_handle (index.js:418)
	notifier (index.js:213)
	parse (index.js:227)
	(anonymous function) (index.js:541)

and that's what everyone else witnessed before 1.2.1

[Andrea Giammarchi]

to whom it might concern, this was the polyfill fix and it makes sense to me after reading this discussion:

https://github.com/WebReflection/custom-elements-upgrade/commit/d680cf08e6d265c9cd46845c55b5e9485f361cb9

I should not have tried to re-attach any own property, that's on me, but it was unexpected from any live element to date to expose own properties that could backfire.

[Ryosuke Niwa]

Functional regressions are P1.

[Chris Dumez]

I wrote this very reduced test case which tries to delete property 0 of a form:
https://codepen.io/cdumez/pen/JjeKgLa

Safari before 264594@main: Able to delete the property
Safari after 264594@main: Not able to delete the property
Chrome 114: Not able to delete the property
Firefox 114: Not able to delete the property

So it seems like 264594@main fixed a bug and aligned our behavior with Chrome & Firefox. Unfortunately, it also seems that the custom-elements polyfill was relying on said bug.

At this point, I think the best we could do is detect the policy and maybe quirk our behavior. I think this would also mean detecting the polyfill version so that we can have our new/standard behavior when using the fixed version of the polyfill.

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> I wrote this very reduced test case which tries to delete property 0 of a
> form:
> https://codepen.io/cdumez/pen/JjeKgLa
> 
> Safari before 264594@main: Able to delete the property
> Safari after 264594@main: Not able to delete the property
> Chrome 114: Not able to delete the property
> Firefox 114: Not able to delete the property
> 
> So it seems like 264594@main fixed a bug and aligned our behavior with
> Chrome & Firefox. Unfortunately, it also seems that the custom-elements
> polyfill was relying on said bug.
> 
> At this point, I think the best we could do is detect the policy and maybe
> quirk our behavior. I think this would also mean detecting the polyfill
> version so that we can have our new/standard behavior when using the fixed
> version of the polyfill.

Also, for the record, I believe that the part of 264594@main that broke the polyfill was dropping this logic in the generated bindings:
https://github.com/WebKit/WebKit/commit/12544d4187162da11dad0451bceece73a83c4837#diff-635979f17a2f56f0ef3529de78480ad862a207ce15c096072b4e025a1e6b7428L1484

We used to mark indexed properties as configurable, but it didn't match Blink or Gecko.

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Chris Dumez from comment #15)
> > I wrote this very reduced test case which tries to delete property 0 of a
> > form:
> > https://codepen.io/cdumez/pen/JjeKgLa
> > 
> > Safari before 264594@main: Able to delete the property
> > Safari after 264594@main: Not able to delete the property
> > Chrome 114: Not able to delete the property
> > Firefox 114: Not able to delete the property
> > 
> > So it seems like 264594@main fixed a bug and aligned our behavior with
> > Chrome & Firefox. Unfortunately, it also seems that the custom-elements
> > polyfill was relying on said bug.
> > 
> > At this point, I think the best we could do is detect the policy and maybe
> > quirk our behavior. I think this would also mean detecting the polyfill
> > version so that we can have our new/standard behavior when using the fixed
> > version of the polyfill.
> 
> Also, for the record, I believe that the part of 264594@main that broke the
> polyfill was dropping this logic in the generated bindings:
> https://github.com/WebKit/WebKit/commit/
> 12544d4187162da11dad0451bceece73a83c4837#diff-
> 635979f17a2f56f0ef3529de78480ad862a207ce15c096072b4e025a1e6b7428L1484
> 
> We used to mark indexed properties as configurable, but it didn't match
> Blink or Gecko.

Doing a quirk to restore pre-264594@main when the custom-element polyfill version < 1.2.1  is used sounds tricky. We usually do quirks based on site domains. Here the quirk would change the behavior of our bindings based on whether of not a script element with a particular URL is on the page. I'll see what I can do.

[Chris Dumez]

How important is a quirk here? Presumably, sites that include the script like so will get the new fixed version of the polyfill already:
```
<script src="//unpkg.com/@ungap/custom-elements"></script>
```

How concerned are we about pages including an explicit version of the polyfill that wouldn't have the fix?

What would be the best way to detect such pages?

In our situation we bundle the polyfill as part of a vendor file so you won’t be able to detect it.

To explain our use case, we are selling themes. Our themes are currently installed on over 5000 e-commerce store that will all break on new safari. The platform we’re working on (Shopify) does not automatically upgrade themes. So of course fixing it for new installs with new polyfill version is easy, but I am already afraid of the apocalypse it will be for our support team when new Safari will be here, unfortunately…

[Ryosuke Niwa]

This polyfill will register a custom element by the name of 'extends-li' so maybe we could use that as a signal.

[Ryosuke Niwa]

Andrea Giammarchi: would it be possible to change the name of element you're creating to detect whether customized built-in is supported or not in a new version? e.g. extends-li-2

[Chris Dumez]

Pull request: https://github.com/WebKit/WebKit/pull/15128

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> Pull request: https://github.com/WebKit/WebKit/pull/15128

I uploaded a proof-of-concept quirk in this PR. It relies on a custom element named
"extends-li" getting created, which is not ideal. As Ryosuke mentions, this would match versions of the polyfill that are fixed so not ideal. The quirk is also a bit intrusive so I'd like not to keep it for too long. Ideally, we'd give enough time for sites to upgrade to v1.2.1+ and drop it.

I have verified locally that it seems to fix this demo case (https://codepen.io/bakura10/pen/mdQERdX). I see no more error logging (about either deleting a property or setting a property). Not sure if I should be watching for anything else.

Things to do still:
- It would be nice to have a fuller test case with things to watch out for to make sure functionality is fully restored with the quirk.
- It would be good to have a way to detect fixed version of the polyfill to disable the quirk for these.

[Chris Dumez]

(In reply to Chris Dumez from comment #23)
> (In reply to Chris Dumez from comment #22)
> > Pull request: https://github.com/WebKit/WebKit/pull/15128
> 
> I uploaded a proof-of-concept quirk in this PR. It relies on a custom
> element named
> "extends-li" getting created, which is not ideal. As Ryosuke mentions, this
> would match versions of the polyfill that are fixed so not ideal. The quirk
> is also a bit intrusive so I'd like not to keep it for too long. Ideally,
> we'd give enough time for sites to upgrade to v1.2.1+ and drop it.
> 
> I have verified locally that it seems to fix this demo case
> (https://codepen.io/bakura10/pen/mdQERdX). I see no more error logging
> (about either deleting a property or setting a property). Not sure if I
> should be watching for anything else.
> 
> Things to do still:
> - It would be nice to have a fuller test case with things to watch out for
> to make sure functionality is fully restored with the quirk.
> - It would be good to have a way to detect fixed version of the polyfill to
> disable the quirk for these.

Other open question:
- Can we restrict the quirk to HTMLFormElement or does this issue impact other types in practice? It would be nice if we could limit the scope of the quirk as much as possible.

Thanks a lot for considering doing a quirk, this is highly appreciated.

I would love this to stay at least for 2 years (in our theme business the upgrade rate is very, very slow as merchants don't upgrade their theme on Shopify once they start customized it).

[Andrea Giammarchi]

(In reply to Ryosuke Niwa from comment #21)
> Andrea Giammarchi: would it be possible to change the name of element you're
> creating to detect whether customized built-in is supported or not in a new
> version? e.g. extends-li-2

I've published 1.3.0 as I can't drop 1.2.1 from npm (AFAIK).

The new patched polyfill uses `extends-br` for `HTMLBRElement` (had to find a terser name and I think I've no other options) and it works without issues in current GNOME Web.

The fix is the same, just the test to apply the polyfill might be different ... and I hope people won't use 2 polyfills at the same time because the registry check for `extends-li` is not there anymore (or maybe it should but I felt like that'd be too hacky to me).

In any case, the try/catch around the attempt to create a builtin extend should guarantee no double patch ever happens or is ever needed, so I think we should all be good with the current fix and the minor change as the test is different and I wouldn't know if anyone would use that registry `extends-li` info/detail for anything in particular but I can't control hacks around hacks ^_^;;

Thanks for the fix and the hints.

[Chris Dumez]

(In reply to Andrea Giammarchi from comment #26)
> (In reply to Ryosuke Niwa from comment #21)
> > Andrea Giammarchi: would it be possible to change the name of element you're
> > creating to detect whether customized built-in is supported or not in a new
> > version? e.g. extends-li-2
> 
> I've published 1.3.0 as I can't drop 1.2.1 from npm (AFAIK).
> 
> The new patched polyfill uses `extends-br` for `HTMLBRElement` (had to find
> a terser name and I think I've no other options) and it works without issues
> in current GNOME Web.
> 
> The fix is the same, just the test to apply the polyfill might be different
> ... and I hope people won't use 2 polyfills at the same time because the
> registry check for `extends-li` is not there anymore (or maybe it should but
> I felt like that'd be too hacky to me).
> 
> In any case, the try/catch around the attempt to create a builtin extend
> should guarantee no double patch ever happens or is ever needed, so I think
> we should all be good with the current fix and the minor change as the test
> is different and I wouldn't know if anyone would use that registry
> `extends-li` info/detail for anything in particular but I can't control
> hacks around hacks ^_^;;
> 
> Thanks for the fix and the hints.

Is there a better test case than https://codepen.io/bakura10/pen/mdQERdX to validate the quirk? I no longer see any console logging about exceptions when deleting or writing properties but is there anything else I should look for?

It will take a while for the quirk to get into STP and for you to be able to test so it would be best if I could do as much of the validation as possible by myself.

[Chris Dumez]

(In reply to Chris Dumez from comment #27)
> (In reply to Andrea Giammarchi from comment #26)
> > (In reply to Ryosuke Niwa from comment #21)
> > > Andrea Giammarchi: would it be possible to change the name of element you're
> > > creating to detect whether customized built-in is supported or not in a new
> > > version? e.g. extends-li-2
> > 
> > I've published 1.3.0 as I can't drop 1.2.1 from npm (AFAIK).
> > 
> > The new patched polyfill uses `extends-br` for `HTMLBRElement` (had to find
> > a terser name and I think I've no other options) and it works without issues
> > in current GNOME Web.
> > 
> > The fix is the same, just the test to apply the polyfill might be different
> > ... and I hope people won't use 2 polyfills at the same time because the
> > registry check for `extends-li` is not there anymore (or maybe it should but
> > I felt like that'd be too hacky to me).
> > 
> > In any case, the try/catch around the attempt to create a builtin extend
> > should guarantee no double patch ever happens or is ever needed, so I think
> > we should all be good with the current fix and the minor change as the test
> > is different and I wouldn't know if anyone would use that registry
> > `extends-li` info/detail for anything in particular but I can't control
> > hacks around hacks ^_^;;
> > 
> > Thanks for the fix and the hints.
> 
> Is there a better test case than https://codepen.io/bakura10/pen/mdQERdX to
> validate the quirk? I no longer see any console logging about exceptions
> when deleting or writing properties but is there anything else I should look
> for?
> 
> It will take a while for the quirk to get into STP and for you to be able to
> test so it would be best if I could do as much of the validation as possible
> by myself.

I would also appreciate if you could help me reduce the scope of the quirk. Would it suffice to apply the quirk to HTMLFormElement? (Do you only try to delete and add indexed properties on forms?) Or maybe only to Node subclasses? At the moment, the scope of the quirk is larger than I'd like.

[Andrea Giammarchi]

every single builtin extends passes through the same code ... fixed in 1.2.1 and in 1.3.x where the try/catch is around extends-br instead of extends-li so the answer I think is no: if any other builtin extend got any non configurable property attached in STP 1.2.0 or earlier would fail the same.

If Form is the only one that aligned with the rest of the vendors though, then yes, you can apply the quirk for the form case only but I think the previous idea to enable quirks on `extends-li` is better, because I believe nobody to date would've used that name to extend anything or it would've thrown out of the box the attempt as already present in the registry.

[EWS]

Committed 265403@main (d8e02e3e3f41): <https://commits.webkit.org/265403@main>

Reviewed commits have been landed. Closing PR #15128 and removing active labels.

[Andrea Giammarchi]

here we go again https://github.com/ungap/custom-elements/issues/19

apparently TP180 breaks again what it shouldn't break despite this previous episode, collaboration, and fixes from both sides.

[Andrea Giammarchi]

digging a bit, I am not sure this commit https://github.com/WebKit/WebKit/commit/23551a03b824a73d7f0b63db00a35706701324f5#diff-635979f17a2f56f0ef3529de78480ad862a207ce15c096072b4e025a1e6b7428 or this one caused possible issues https://github.com/WebKit/WebKit/commit/0756854fafd4312b8f0126de111792e39db106ef#diff-635979f17a2f56f0ef3529de78480ad862a207ce15c096072b4e025a1e6b7428 but it's also not super clear what is the error now around the broken polyfill.

The test case works on Epiphany TP 45.0-11-g986625975+ but I can't possibly know if that matches TP180 for Safari.

[Ryosuke Niwa]

(In reply to Andrea Giammarchi from comment #31)
> here we go again https://github.com/ungap/custom-elements/issues/19
> 
> apparently TP180 breaks again what it shouldn't break despite this previous
> episode, collaboration, and fixes from both sides.

It's STP179 which broke this. The culprit is http://commits.webkit.org/267565@main but this change has already been reverted on trunk so the next version of STP (i.e. STP181 when it gets released) should not have this issue.

[Ryosuke Niwa]

Note for myself:
268059@main: Bad
267603@main: Bad
267570@main: Bad
267565@main: Bad
267564@main: Good
267563@main: Good
267560@main: Good
267555@main: Good
267540@main: Good
267475@main: Good
267350@main: Good
267300@main: Good
267113@main: Broken
267000@main: Good
266624@main: Good