Fix element not calling attach internals #130
Conversation
christophe-g
force-pushed
the
fix-element-not-calling-attach-internals
branch
2 times, most recently
from
906d195 to
df3a298
Compare
|
Let’s rebase this and I’ll get it reviewed/merged. Appreciate the contribution @christophe-g and the feedback in the PR @asyncLiz. |
This is to allow run playwright webkit on linux Mint
christophe-g
force-pushed
the
fix-element-not-calling-attach-internals
branch
from
4441adb to
96428e8
Compare
|
@calebdwilliams - rebased against latest commit. |
| @@ -393,9 +393,6 @@ export function forceElementInternalsPolyfill(forceCustomStateSet = true) { | |||
| } else if (this.tagName.indexOf('-') === -1) { | |||
| throw new Error(`Failed to execute 'attachInternals' on 'HTMLElement': Unable to attach ElementInternals to non-custom elements.`); | |||
| } | |||
| if (internalsMap.has(this)) { | |||
There was a problem hiding this comment.
I'm not sure this is the right fix. This is removing a guard against attaching a second internals instance. I think a better fix would be to not call upgrade internals if an instance it doesn't exist.
There was a problem hiding this comment.
I tested this, and removing the line didn't work, as the fix for upgradeInternals is needed to run for tests to pass with Stencil components with Jest, so in my take on it in #142 I added a flag to make to not throw the error if attachInternals is run in the upgradeInternals function
|
@christophe-g and @calebdwilliams is this PR considered stale? We are also in need of this fix, and very much hope that this could get looked at! If stale, then could I branch out and address the requested change to make a new PR? |
|
Let’s get the fix rebased and I’ll get that merged/released. |
|
I might have some time to work on this before the end of the week
…On Tue, Mar 25, 2025 at 9:06 PM Caleb Williams ***@***.***> wrote:
Let’s get the fix rebased and I’ll get that merged/released.
—
Reply to this email directly, view it on GitHub
<#130 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI5FNOMA3Z3LROJAK2LVJ32WGZKXAVCNFSM6AAAAABZVA55AOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONJSGM4TCNRUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
[image: calebdwilliams]*calebdwilliams* left a comment
(calebdwilliams/element-internals-polyfill#130)
<#130 (comment)>
Let’s get the fix rebased and I’ll get that merged/released.
—
Reply to this email directly, view it on GitHub
<#130 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI5FNOMA3Z3LROJAK2LVJ32WGZKXAVCNFSM6AAAAABZVA55AOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONJSGM4TCNRUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
--
<https://preignition.org/>
Christophe Geiser / CTO
We are defined by the lines we choose to cross or to be confined by
Co-Founder of preignition.org / accessiblesurveys.com
|
|
Hello @christophe-g, do you plan to work on rebasing this PR soon? Also I don't think the polyfill need to warn about 'ElementInternals missing from the element' as you added in your PR. If web-component didn't call attachInternals, it will not work even when browser implemented the API. |
|
Due to the inactivity here, and both me and @yannhub being in need of this fix, I forked the branch of the PR, and created a new PR (#142) hopefully to get this resolved soon. It is rebased on main, so @christophe-g should still be author of the appropriate commits, but would be more than happy to close my PR (or change it to only have my changes) if the original PR gets merged. But I really hope that this can get looked at, that would be very much appreciated @calebdwilliams |
Fix #127
The regression test does not pass on webkit without the fix