ungap / custom-elements

All inclusive customElements polyfill for every browser
ISC License
240 stars 14 forks source link

Safari TP 172 seems to break polyfill #18

Closed bakura10 closed 1 year ago

bakura10 commented 1 year ago

Hi,

I just upgraded Safari TP to 172 released yesterday, and unfortunately all our themes stopped working (we have thousands of installs, so we hope this is just a bug from Safari that can be fixed).

I'm currently on holiday and can't recreate a reduced test case but you can see the issue here: https://prestige-theme-allure.myshopify.com/products/le-gina-petrol-liege

When you click on "Add to cart" it should open the modal (we extend the form element with a built-in custom element) but starting from Safari TP172, the following error appear instead:

image

It seems it points to this code:

image

Thanks.

EDIT: it seems to only apply to our custom element extending a form. Other custom elements extending different built-in does not seem to exhibit the issue.

WebReflection commented 1 year ago

it seems to only apply to our custom element extending a form

then either file a bug in safari or give me a minimal example that fails where the fault is the polyfill one, otherwise there's nothing I can do.

Closing until the latter suggestion happens.

bakura10 commented 1 year ago

Sorry about that. You can find a minimum test case here: https://codepen.io/bakura10/pen/mdQERdX

The connectedCallback is properly called on Safari (stable), Chrome and Firefox. Starting from TP172 (or maybe 171) the connectedCallback is no longer called, and the following error is triggered:

Unhandled Promise Rejection: TypeError: Unable to delete property.

I am not familiar enough with the polyfill internal to know if it is a polyfill issue or Safari issue.

Thanks.

WebReflection commented 1 year ago

@bakura10 I don't have Safari TP but could you please use https://unpkg.com/@ungap/custom-elements@1.2.0/index.js as src instead and tell me more about exact line or issue you are facing?

bakura10 commented 1 year ago

Sure!

Safari (stable): no issue Safari (TP172) tracelog:

image

The issue is happening on line 271:

image
WebReflection commented 1 year ago

@bakura10 awesome ... do you mind adding a debugger around that line and tell me what key[i] is there? I am assuming it's the is attribute itself and this seems easy to fix from my side but I wouldn't know for sure, thanks!

bakura10 commented 1 year ago

I tried but it seems to be... 0 ! Safari is always full of surprises...

image
WebReflection commented 1 year ago

@bakura10 can you please try with https://unpkg.com/@ungap/custom-elements@1.2.1/index.js and tell me how it goes?

bakura10 commented 1 year ago

I just tried and this seems to fix it!

Do you have any idea on what could have caused this regression? We are building Shopify themes, this means that thousands of stores will suddenly break (which we will have to handle at support manually). If this is a regression caused by Apple, I would be more than happy to try to do a report but I am not sure what could be happening here :/.

WebReflection commented 1 year ago

Do you have any idea on what could have caused this regression?

No, and honestly I don't care, as much as they don't care testing my polyfill ... they just gotta support builtin extend as it's one of the most demanding features out there they keep ignoring.

The fix looks robust enough for further shenanigans on own properties on native elements though, I think it'll last for a while.

bakura10 commented 1 year ago

Can't agree more on that... Thanks in all cases to provide a quick fix, I will write some internal note for our support team.

WebReflection commented 1 year ago

P.S. 3 repositories involved so I wouldn't exclude they're just messing around here to prove nobody should interfere with builtin elements ... how lovely!

WebReflection commented 1 year ago

for history sake, if anyone is interested in what was the fix, here we go: https://github.com/WebReflection/custom-elements-upgrade/commit/d680cf08e6d265c9cd46845c55b5e9485f361cb9

karlcow commented 1 year ago

This is also tracked on https://bugs.webkit.org/show_bug.cgi?id=258282

bakura10 commented 1 year ago

@karlcow , for instance Safari TP180 is once again breaking the polyfill (please see #19 ). I hope a similar solution can also be introduced to ensure that websites do not break.

karlcow commented 1 year ago

@bakura10 see https://github.com/ungap/custom-elements/issues/19#issuecomment-1749356690