Shady Parts: improved support for mutations, plus small bugfixes #357
Conversation
Also use the native method when setting the "shady-part" attribute, instead of going through ShadyDOM.
|
This might sound greedy of me (thanks for doing this), but do you think an improvement could be made to address #81 as well? Right now slot functionality in the vue WC wrapper is basically broken because of it. This fork fixes it by using polling, but it would be nice if the MutationObserver could be fixed in ShadyDOM. https://github.com./TeliaSweden/vue-web-component-wrapper-ie11/blob/7646d635a322363d98d3699ec8e46dc63840948b/src/index.js#L114 |
| addShadyPartAttributes(host, parts); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This does more than just add the attributes. Suggest adding explanation. Also find the name parts confusing for describing an array of elements.
There was a problem hiding this comment.
This does more than just add the attributes. Suggest adding explanation.
Done (styleShadowParts with new jsdoc description of exactly what it does).
There was a problem hiding this comment.
Also find the name
partsconfusing for describing an array of elements.
Done, renamed to partNodes. I also updated some other spots to try and use the variables partName and partNode consistently instead of part.
There was a problem hiding this comment.
Thanks, I think that improves the readability a lot.
| if (!root) { | ||
| return; | ||
| } | ||
| const staleParts = []; |
There was a problem hiding this comment.
Done (stalePartNodes though, I've been using nodes more consistently).
| <script type="module"> | ||
| import {pierce, style, black, blackQuad, red, green} from './test-utils.js'; | ||
|
|
||
| suite('exportparts attribute mutations', () => { |
There was a problem hiding this comment.
Add comment that state is purposefully maintained from test to test here.
There was a problem hiding this comment.
It's not actually, setup and teardown happen on every test case. Updated the initial state test description to verify initial state since I can see that would imply it sets state rather than just checks it.
| }); | ||
|
|
||
| test('move parent from grand-1 to grand-2', () => { | ||
| grand2.shadowRoot.appendChild(parent); |
There was a problem hiding this comment.
Would this work if the parent was just removed? Do you need to also patch removeChild in ShadyDOM? (If so, can file/handle as a separate issue.)
There was a problem hiding this comment.
Seems like if an exportparts node is removed, then any descendant parts aren't connected, so we don't care if the shady-part attributes are stale and can safely do nothing. Once they get re-connected somewhere else we do care, but that's already handled here because of the onInsert logic. I added some removeChild calls to the test case just to ensure nothing weird is happening.
Is there another scenario I'm not thinking of?
There was a problem hiding this comment.
No, I think that makes sense.
NOTE: Recommend viewing each commit separately!
Small bugfixes:
::partselector parse bug for element names with >1 dash::partselector parse bug for comma-delimited group selectors in custom property handling logiconInsertBeforehookImproved mutation support:
partattribute withsetAttributeandremoveAttributeexportpartsattribute withsetAttributeandremoveAttributeexportpartsnodes, and moving them between shadow roots (part nodes already worked, did add more tests though).Part of #252
Fixes #353
Fixes #354
Fixes #355