Skip to content
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

Many aurelia libraries are broken in iOS 16 #1003

Closed
jded76 opened this issue Oct 18, 2022 · 47 comments
Closed

Many aurelia libraries are broken in iOS 16 #1003

jded76 opened this issue Oct 18, 2022 · 47 comments

Comments

@jded76
Copy link

jded76 commented Oct 18, 2022

I'm submitting a bug report

  • Library Version:
    aurelia-bootstrapper: 2.3.3

Please tell us about your environment:

  • Operating System:
    OSX 16

  • Node Version:
    14.16.0

  • NPM Version:
    6.14.11
  • Aurelia CLI OR JSPM OR Webpack AND Version
    CLI 1.3.1 | webpack 4.46.0
  • Browser:
    iOS 16 Safari

  • Language:
    all

Current behavior:
After the new IOS 16, Safari has a weird behavior in Aurelia, causing many libraries to fail at the detached event.
After some searching I found that the ownerdocument of the element is an empty one (about:blank) and many libraries that do cleanup at the detached event are getting errors. More details can be found here .

There is also a repro on the issue 808 of aurelia-kendoui-bridge
aurelia-ui-toolkits/aurelia-kendoui-bridge#808

Expected/desired behavior:

Be compatible with Safari.

  • What is the motivation / use case for changing the behavior?
    Can we do something to be compatible again with the new Safari?
@bigopon
Copy link
Member

bigopon commented Oct 18, 2022

I'm not able to guess what has started to fail. ownerDocument suddenly becomes null seems to be an error on Safari part more than Aurelia. This probably is not specific to Aurelia, many others will start to fail as well. Does this happen only when you start navigating away from a page?

@jded76
Copy link
Author

jded76 commented Oct 19, 2022

I thought the same (that this is not specific to Aurelia) and I tried to find any issues with Safari 16 on other frameworks or whatever, but I couldn't find anything.
I just tested with a component (with if.bind) and it is not happening in the detached event.
So it seems you are right, it only happens when you are navigating to another page.

You can see this repo if you like to test anything.
I have installed Playwright, so you can test it without IOS.

@Tamu
Copy link

Tamu commented Oct 20, 2022

I also have issues with Safari 16 too, but I'm not sure it's the same reason.

Here is my repo (close to jded76 repo but with Fast)

and the bug in a nice animation :
https://user-images.githubusercontent.com/5020311/196905608-6971e194-df3c-4ef8-b0b2-fd86cc00b975.mp4

How can I help to find a solution???

@bigopon
Copy link
Member

bigopon commented Oct 20, 2022

Thanks @Tamu, nice 🙂
I suspect this is the prevent default call that is causing the issue. Can you try to returm true from inside the emailChanged?

@Tamu
Copy link

Tamu commented Oct 20, 2022

Awesome @bigopon, if I add if.bind=true in the component it works !

<fast-text-field if.bind="true" type="text" value.bind="email" change.trigger="emailChanged()"></fast-text-field>

On the other hand if I add return true in emailChanded it does not change anything

  emailChanged() {
    return true;
  }

image

@bigopon
Copy link
Member

bigopon commented Oct 20, 2022

@Tamu can you help check the console to see if there is any errors for the one without if? Please paste the whole trace if possible.

@Tamu
Copy link

Tamu commented Oct 20, 2022

here is all the console log :

[Info] [webpack-dev-server] Server started: Hot Module Replacement disabled, Live Reloading enabled, Progress disabled, Overlay enabled. (vendors-node_modules_webpack-dev-server_client_index_js_protocol_ws_3A_hostname_0_0_0_0_port_-a5d60f.2649ddd3284f15ce65eb.bundle.js, line 963)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-templating-binding." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-templating-binding." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-templating-resources." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-templating-resources." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-event-aggregator." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-event-aggregator." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-history-browser." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-history-browser." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-templating-router." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-templating-router." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin resources/index." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin resources/index." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia1-fast-adapter." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia1-fast-adapter." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Loading plugin aurelia-testing." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [aurelia] – "Configured plugin aurelia-testing." (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [templating] – "importing resources for aurelia-templating-resources" – [] (0) (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Debug] DEBUG [templating] – "importing resources for aurelia-templating-router" – [] (0) (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Info] INFO [aurelia] – "Aurelia Started" (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2576)
[Debug] DEBUG [templating] – "importing resources for app.html" – ["app.css"] (1) (vendors-node_modules_aurelia-bootstrapper_dist_native-modules_aurelia-bootstrapper_js-node_mo-047062.2649ddd3284f15ce65eb.bundle.js, line 2566)
[Log] fast-text-field : changing – "te" (app.2649ddd3284f15ce65eb.bundle.js, line 74)
[Log] fast-text-field : changing – "test" (app.2649ddd3284f15ce65eb.bundle.js, line 74, x3)
[Log] fast-text-field : changing – "test" (app.2649ddd3284f15ce65eb.bundle.js, line 74)
[Log] fast-text-field : changing – "test1" (app.2649ddd3284f15ce65eb.bundle.js, line 74)
[Log] fast-text-field : changing – "test1" (app.2649ddd3284f15ce65eb.bundle.js, line 74)
[Log] fast-text-field – "test1" (app.2649ddd3284f15ce65eb.bundle.js, line 70)

@ben-girardet
Copy link

Hi @bigopon and @Tamu

I confirm that there is a new issue with iOS 16 when observing properties in Aurelia 1. The aurelia1-fast-adapter that you helped me write a long time ago doesn't work anymore (only in iOS 16).

In the file aurelia-fast-adapter.ts, in iOS 16 the getObserver() on line 140 is never called.

Any help on this matter is very appreciated.

@jded76
Copy link
Author

jded76 commented Oct 21, 2022

I searched further on the problem, and if I'm right (because I don't know the internals of aurelia) the problem is not at the detached event, but at the creation of the View.

I had a breakpoint at this this line (aurelia-templating - ViewFactory - create), hit refresh on the browser and at the first hit of the breakpoint I saw that on Chrome the template has a valid document but on Safari it has an about:blank document (see attached pictures).

@bigopon do you have any idea? Am I on the right track?
Thank you

Chrome
chrome

Safari
Safari

@bigopon
Copy link
Member

bigopon commented Oct 21, 2022

@jded76 this.template can have null for ownerDocument, but after the document.adoptNode call, it should have the right ownerDocument. Can you check further after that?

@jded76
Copy link
Author

jded76 commented Oct 21, 2022

I saw that all the views have on fragment property the about:blank on Safari, even before the detached event. That led me there. I wanted to find why we have different behavior between Safari and Chrome. As you can see on Chrome has already the right document.

You can see here, after DOM.adoptNode the content is about:blank document
AfterCom adoptNode

@bigopon
Copy link
Member

bigopon commented Oct 21, 2022

@jded76 right, thanks. I guess you have replicated the bug. Safari 16 document.adoptNode when called on DocumentFragment stopped working the way it did before. Maybe it's time to file a bug with Safari/webkit team. I'm not sure where the place is.

@bigopon
Copy link
Member

bigopon commented Oct 21, 2022

A test can be like this I think

const div = document.createElement('div')
div.innerHTML = '<template><button>';

const fragment = div.firstChild.content;
console.assert(fragment.ownerDocument === null)

document.adoptNode(fragment);
console.assert(fragment.ownerDocument === document);

With Safari 16, it'll fail while it'll succeed with Safari < 16

@jded76
Copy link
Author

jded76 commented Oct 21, 2022

I just found that the source has the right document, but the source.content has the wrong one at this line.

@bigopon
Copy link
Member

bigopon commented Oct 21, 2022

That is expected, before .adoptNode call, it'll likely have a different owner document. If it still does after the .adoptNode call then that's the change/bug.

@jded76
Copy link
Author

jded76 commented Oct 21, 2022

Sorry this is the same behavior as in Chrome. Ignore my previous comment.
The problem is that after content = DOM.adoptNode(source.content); in chrome has the right ownerDocument but in Safari the wrong one.

@jded76
Copy link
Author

jded76 commented Oct 21, 2022

A test can be like this I think

const div = document.createElement('div')
div.innerHTML = '<template><button>';

const fragment = div.firstChild.content;
console.assert(fragment.ownerDocument === null)

document.adoptNode(fragment);
console.assert(fragment.ownerDocument === document);

With Safari 16, it'll fail while it'll succeed with Safari < 16

Actually the ownerDocument is not null, but an empty document (about:blank).
I created a fiddler here.
In Chrome the console says "about:blank" and then "https://fiddle.jshell.net/j6duwkbt/show/?editor_console="
In Safari 16 the console says "about:blank" in both cases.
In Firefox the console says ""https://fiddle.jshell.net/_display/?editor_console=true"" in both cases.
Unfortunatelly I don't have an older version of Safari to try right now.

What can we do about that from here?

@3cp
Copy link
Member

3cp commented Oct 22, 2022

Few investigations:

https://github.com/aurelia/templating/blob/aaf3a00cb28af59021421f99e179dc9f60ce2d28/src/view-slot.ts#L335

For reference, I am able to bypass this issue if I manually modify the ViewSlot removeAll (this issue only used removeAll code path) method, to do detached() callback before child.removeNodes().

In other words, I tested calling detached() callback before the view is removed from DOM. So it's kind of "detaching" callback like au2, and it worked. That confirmed Safari 16 has issue in dealing with detached element.

  1. I was unable to find a way to reproduce this bug using simple DOM api (out of aurelia app). ownerDocument seems correct for detached element. I need to reproduce in order to raise a bug to webkit. Just noticed the code sample above :-)

  2. confirmed this issue still exists in unreleased Safari 16.1 beta, and also Safari Technology Preview. Also could not find a bug raised in webkit.

@3cp
Copy link
Member

3cp commented Oct 22, 2022

@bigopon
Copy link
Member

bigopon commented Oct 22, 2022

Thanks @3cp

@jded76
Copy link
Author

jded76 commented Oct 22, 2022

I'm still thinking about this.
I found this relevant commit in webkit on July and if I followed the discussion correctly, they did this to be compatible with the standard (see the 3rd rule here)

If node is a DocumentFragment node whose host is non-null, then return..

Safari is the first major browser that supports that rule.

It look's like that this is not a bug of Safari, but a feature and we must adapt to overcome the problems.

On the other hand I remember that I tested the detach event of aurelia-kendoui-bridge without navigating to other page and it didn't have the same problem. I'm still searching on this...

@jded76
Copy link
Author

jded76 commented Oct 22, 2022

Finally I'm getting somewhere for aurelia-kendoui-bridge, maybe for other libraries too that using jquery.

I changed this line from
var adown = a.nodeType === 9 ? a.documentElement : a, bup = b && b.parentNode;
to
var adown = a.nodeType === 9 && a.documentElement ? a.documentElement : a, bup = b && b.parentNode;
and I'm not getting any errors.
I will file an issue to jquery for this to see if they can make this change to be compatible with safari again.

Edit:
The difference between navigating to another page and if.bind was that somewhere in jquery they compare the current document with the ownerdocument of the element and in the second case were the same, so it didn't get to "contains function" that has the problem with a.documentElement being null.

@3cp
Copy link
Member

3cp commented Oct 22, 2022

One not-so-great way to partially ignore the error. Only works for route component.

<router-view swap-order="before"></router-view>

This swap order loads new route component before unloading old one. The exception still happens on the old component, error log still shows up in console, but it would not stop the new route from loading up.

@3cp
Copy link
Member

3cp commented Oct 22, 2022

@bigopon, since @jded76 found out Safari is likely following the updated spec. There is no much we can do about it.

https://github.com/aurelia/templating/blob/aaf3a00cb28af59021421f99e179dc9f60ce2d28/src/view-compiler.ts#L104

I tested replace the adoptNode with importNode, it works.

//content = DOM.adoptNode(source.content);
content = document.importNode(source.content, true);

Obvious the deep clone has performance penalty, but it seems necessary to be compatible with updated spec. The other option is not using document fragment at all, but I don't think that's possible for au1 since au1 uses native browser html parser.

Regarding the spec, I have trouble to follow "a DocumentFragment node whose host is non-null", any idea what's the api to inspect the host of a fragment?

@jded76
Copy link
Author

jded76 commented Oct 23, 2022

I'm not sure that aurelia will have a problem with the change of the spec.
I made this repository so you can test with vanilla js.

You can see (with Safari 16+) that the template and the template.content have an empty ownerDocument, and I think this is right because they don't belong to the main document.
But after you add the template to main document, the span has the right document. And if you remove it, it reverts to an empty document.

The only issue I can see it's the lack of the "detaching" event.

Regarding the spec, I have trouble to follow "a DocumentFragment node whose host is non-null", any idea what's the api to inspect the host of a fragment?

From the commit of Safari, they check if the node is type of TemplateContentDocumentFragment.

@3cp
Copy link
Member

3cp commented Oct 23, 2022

@bigopon I recall you have some benchmark setup? if importNode didn't hurt performance too much, it might be a good enough fix.

@jded76
Copy link
Author

jded76 commented Oct 23, 2022

If this is right, then if we change to importNode, we must change the remove process too.
As it is now (and if I understand it correctly), it's adding the removed elements back to template fragment again. If 'importNode' is not removing them from the beginning then we must change the remove process too.

@jded76
Copy link
Author

jded76 commented Oct 23, 2022

And I think if we change the remove process to just remove the elements from the current view (and not adding them back to the template), then we will have the same problem, that the elements don't have an ownerDocument

Or maybe even worse, that the elements don't exist any more on the detached event?

@3cp
Copy link
Member

3cp commented Oct 23, 2022

https://bugs.chromium.org/p/chromium/issues/detail?id=1031811
https://bugzilla.mozilla.org/show_bug.cgi?id=1602200

It seems both Chrome and Firefox are still waiting for some further spec change. whatwg/dom#819

There is new option forceDocumentFragmentAdoption mentioned in the pending change.

@3cp
Copy link
Member

3cp commented Oct 23, 2022

From my limited understanding, au1's remove process only returns view to a cache (which is just a simple JS array to hold those views).

@3cp
Copy link
Member

3cp commented Oct 23, 2022

Also, it's little bit unusual for DOM spec to introduce a breaking change.

@jded76
Copy link
Author

jded76 commented Oct 23, 2022

From my limited understanding, au1's remove process only returns view to a cache (which is just a simple JS array to hold those views).

I'm not sure but when I was searching, I have shown this and I thought that is the removal process.

https://bugs.chromium.org/p/chromium/issues/detail?id=1031811 https://bugzilla.mozilla.org/show_bug.cgi?id=1602200

It seems both Chrome and Firefox are still waiting for some further spec change. whatwg/dom#819

I have shown that too, the request of the change is two years old but I couldn't follow them...

@3cp
Copy link
Member

3cp commented Oct 23, 2022

I'm not sure but when I was searching, I have shown this and I thought that is the removal process.

I think you are right :-) I didn't read these lines.

@3cp
Copy link
Member

3cp commented Oct 24, 2022

webkit team is doing something on that. I don't quite understand the details, but it seems they think the bug I raised is a valid bug.

🤞🏻

@bigopon
Copy link
Member

bigopon commented Oct 24, 2022

Thanks @3cp @jded76 🤞 . We have a way around it, which is around the template parsing time: immediately import the fragment after creating the template from a string. And perf won't be an issue here since we are never going to compile 10000 times per second.
Though I'd really prefer if the DOM wasn't broken this way, we are probably not the only will be affected by this change.

@3cp
Copy link
Member

3cp commented Oct 25, 2022

Too bad, webkit closed the ticket as "intentional behaviour" :-(

@jded76
Copy link
Author

jded76 commented Nov 5, 2022

It looks like that the change of the specs is stuck without any updates and we have to figure out what we have do to be compatible with Safari again...
Personally I changed locally the jquery script and removed the aurelia-resize library and it seems the application is working again with Safari.
But I still don't know if will have other hidden problems elsewhere.
@bigopon do you think that this change affects the core functionality of Aurelia too?
Like, for example, the issue @Tamu and @ben-girardet mention here?

@bigopon
Copy link
Member

bigopon commented Nov 5, 2022

Maybe we can have a snippet to let apps temporary patch it until further action from the spec and Safari? Then apps can use it to verify whether it's a bug from this change or not. I'm a bit afraid of doing any "fix" for this right now, before a resolution in the spec.

@jded76
Copy link
Author

jded76 commented Nov 5, 2022

I'm a bit afraid of doing any "fix" for this right now, before a resolution in the spec.

I agree with that, that's why I asked your opinion if this affects the core functionality.

About the snippet and the patch, i can't follow you. What do you mean?

@slgoodson
Copy link

Has anyone found a temporary work-around for this? Dealing with this issue in production still.

@jded76
Copy link
Author

jded76 commented Jan 12, 2023

We solved our production problems by removing some incompatible libraries and for aurelia-kendoui-bridge we changed something in jquery and it's working again.

@bigopon
Copy link
Member

bigopon commented Mar 3, 2023

good news, it's being revised https://bugs.webkit.org/show_bug.cgi?id=246899

@bigopon
Copy link
Member

bigopon commented Mar 10, 2023

PR is up, so I guess we'll have some good updates for our customers/users soon.
WebKit/WebKit#11347

@3cp
Copy link
Member

3cp commented Aug 30, 2023

@jded76 I don't see this problem anymore on Safari v16.6.
Can you confirm?

@jded76
Copy link
Author

jded76 commented Aug 30, 2023

I think it's working again.
I don't have IOS to test it but I tried with the latest playwright and it's working fine.

@3cp
Copy link
Member

3cp commented Aug 30, 2023

@bigopon pls close it :-)

@bigopon
Copy link
Member

bigopon commented Aug 30, 2023

Thanks @jded76 @3cp

@bigopon bigopon closed this as completed Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants