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?
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.
<rdar://problem/111008826>
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.
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.
(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.
> 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.
(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.
(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.
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/
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.
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
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.
Functional regressions are P1.
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.
(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.
(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.
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…
This polyfill will register a custom element by the name of 'extends-li' so maybe we could use that as a signal.
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
Pull request: https://github.com/WebKit/WebKit/pull/15128
(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.
(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).
(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.
(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.
(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.
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.
Committed 265403@main (d8e02e3e3f41): <https://commits.webkit.org/265403@main> Reviewed commits have been landed. Closing PR #15128 and removing active labels.
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.
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.
(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.
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