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

nimble-table data doesn't render in Safari #984

Closed
jattasNI opened this issue Jan 19, 2023 · 3 comments · Fixed by #998
Closed

nimble-table data doesn't render in Safari #984

jattasNI opened this issue Jan 19, 2023 · 3 comments · Fixed by #998
Assignees
Labels
bug Something isn't working

Comments

@jattasNI
Copy link
Contributor

🐛 Bug Report

When you open a demo page that contains a nimble-table in Safari, the table shows empty cells and there are errors in the dev tools console.

💻 Repro or Code Sample

Visit https://nimble.ni.dev/storybook/example-client-app/#/customapp in Safari and scroll to the table example

🤔 Expected Behavior

Table renders data like it does in other browsers.

😯 Current Behavior

image

Repeated console errors:

[Error] TypeError: undefined is not an object (evaluating 'n.appendChild')
	runGuarded (polyfills.54ab400a48eba0a6.js:1:2165)
	appendChild
	appendTo (main.a4d212b3f0e17485.js:1:264961)
	render (main.a4d212b3f0e17485.js:1:268254)
	renderTemplate (main.a4d212b3f0e17485.js:1:244195)
	finishInitialization (main.a4d212b3f0e17485.js:1:243793)
	onConnectedCallback (main.a4d212b3f0e17485.js:1:242794)
	connectedCallback (main.a4d212b3f0e17485.js:1:244616)
	connectedCallback (main.a4d212b3f0e17485.js:1:257647)
	runGuarded (polyfills.54ab400a48eba0a6.js:1:2092)
	insertBefore
	insertBefore (main.a4d212b3f0e17485.js:1:265051)
	d$ (main.a4d212b3f0e17485.js:1:261462)
	handleChange (main.a4d212b3f0e17485.js:1:263178)
	notify (main.a4d212b3f0e17485.js:1:231601)
	call (main.a4d212b3f0e17485.js:1:233925)
	i (main.a4d212b3f0e17485.js:1:229445)
	o (main.a4d212b3f0e17485.js:1:229526)
	(anonymous function) (polyfills.54ab400a48eba0a6.js:1:28207)
	runTask (polyfills.54ab400a48eba0a6.js:1:2590)
	invokeTask (polyfills.54ab400a48eba0a6.js:1:8243)

💁 Possible Solution

Not a solution, but some initial debugging showed:

  • In Firefox, cellTemplateChanged() (in cell/index.ts) is called many times but this.isConnected is false so it exits early.
  • In Safari, cellTemplateChanged() is being called many times but this.isConnected is true. However this.cellContentContainer is undefined. This results in a call within FAST code to view.appendTo(undefined) and thus undefined.appendChild().

🔦 Context

First noticed when testing virtualization in different browsers: #966 (comment)

When this is fixed we should re-validate that scrolling performance is good in Safari (no flickering).

🌍 Your Environment

  • OS & Device: macOS 13.1
  • Browser: Safari
  • Version: 16.2 (18614.3.7.1.5)

@msmithNI also reproduced in the Windows WebKit build from Playwright: https://playwright.azureedge.net/builds/webkit/1751/webkit-win64.zip

@jattasNI jattasNI added bug Something isn't working triage New issue that needs to be reviewed labels Jan 19, 2023
@jattasNI jattasNI mentioned this issue Jan 19, 2023
1 task
@jattasNI jattasNI self-assigned this Jan 24, 2023
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Jan 24, 2023
@jattasNI
Copy link
Contributor Author

Possibly related FAST bug: microsoft/fast#6537

It links to a webkit bug which is closed as "expected behavior" due to a recent web components spec change and discussion about whether the spec change is a good idea.

@msmithNI
Copy link
Contributor

@rajsite noticed that the table cell has an isConnected call that should be $fastController.isConnected. I tried that locally and it fixed the issue in the Playwright Webkit at least. Can you try it out in the real Webkit / on your Mac?

@jattasNI
Copy link
Contributor Author

Sweet! It fixes it for me!

jattasNI added a commit that referenced this issue Jan 26, 2023
# Pull Request

## 🤨 Rationale

Fixes #984 

## 👩‍💻 Implementation

As @rajsite and @msmithNI suggested in the linked issue, use
`this.$fastController.isConnected` instead of `this.isConnected` in the
table cell's `cellTemplateChanged()` method.

## 🧪 Testing

I manually tested in Safari in Storybook and the Angular example app.
Malcolm tested in Playwright's Safari.

Auto tests would be covered by #990.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

3 participants