fix: patch additional methods so React doesnt break with template elements#9385
fix: patch additional methods so React doesnt break with template elements#9385snowystinger merged 5 commits intomainfrom
Conversation
|
Build successful! 🎉 |
|
Is a test possible for this one? |
|
I tried to make one but couldn't get it to fail in the SSR tests without the change unfortunately |
reidbarber
left a comment
reidbarber
left a comment
There was a problem hiding this comment.
I still see the issue if I navigate to another page first, then the Picker page. Doesn't happen when just loading the Picker page first though.
|
oh thats bizzare, didn't expect that flow to break, I'll dig |
|
Build successful! 🎉 |
| enumerable: true, | ||
| value: function (node, child) { | ||
| if (this.dataset.reactAriaHidden) { | ||
| // child might not exist in this.content for some reason (stale?), add to end instead |
There was a problem hiding this comment.
???
In this case, what is the child and it's parent?
There was a problem hiding this comment.
the child is the bottom most div, at the time of the error this.content is a document fragment with no children hence why it breaks trying to call insertBefore
There was a problem hiding this comment.
oh, this seems wrong. <template> doesn't support direct children, they should be within the document fragment. That's what these overridden methods should be doing (since react doesn't handle that). So maybe we're missing another one and that's how those dives are getting there?
There was a problem hiding this comment.
ok I did some poking around and it seems like the monkey patches about didn't trigger their reactAriaHidden route when switching pages from some arbitrary page to the Picker docs page, hence causing the original error to appear when attempting to add a description via the example controls since we had stray direct children as you noticed above. I'm not 100% sure how that happens since the aria hidden data attribute should always be there...
On the plus side, this actually fixed a couple of tests that actually inadvertently noticed that those direct children were still appearing outside the document fragment.
There was a problem hiding this comment.
did some more digging to verify what is happening and got this:
The above is from adding a debugger to the patch of appendChild where we call originalAppendChild.call(this, node); aka the else statement when we detect that the template element in question doesn't have data-react-aria-hidden. However, as can be seen above the element has said attribute as part of its React props, but not in its attributes hence why we are appending the elements as direct children to the template element rather than sending them to the document fragment.
Is there a reason we have a data-react-aria-hidden check anyways? Can't we have these patches always run on any template element since direct children aren't allowed anyways?
|
Build successful! 🎉 |
1 similar comment
|
Build successful! 🎉 |
|
Build successful! 🎉 |
reidbarber
left a comment
reidbarber
left a comment
There was a problem hiding this comment.
Not seeing any issues, would be good to get this in for further testing.
|
Build successful! 🎉 |
| return; | ||
| } | ||
| while (el.childNodes.length > 0) { | ||
| el.content.appendChild(el.childNodes[0]); |
There was a problem hiding this comment.
do we need to remove the Child node from the original location as well? does this result in it in two places?
github-project-automation
bot
moved this from 👀 In Review
to ✅ Done
in RSP Component Milestones
5 participants
Closes #9376
✅ Pull Request Checklist:
📝 Test Instructions:
Go to S2 Picker docs. Type any description in the first example's description control, it should apply without crashing
🧢 Your Project:
RSP