search

webcomponents / polyfills

Web Components Polyfills
BSD 3-Clause "New" or "Revised" License
1.14k stars 165 forks source link

fix: toggleAttribute polyfill should retain force argument #534

Closed jmcgavin closed 1 year ago

jmcgavin commented 1 year ago

Addresses an issue where toggleAttribute would not retain force, the second positional argument passed to it.

Fixes https://github.com/webcomponents/polyfills/issues/533

jmcgavin commented 1 year ago

@aomarks, @bicknellr, this is a pretty significant bug because it prevents us from using the native feature set of toggleAttribute. Let me know how I can help expedite this merge request

u-b-i-p-i-x-e-l-s commented 1 year ago

Hey @aomarks and @bicknellr 👋

Could you please review? Thanks!!

bicknellr commented 1 year ago

Hi, thanks for sending this! Could you add a test or two? It doesn't seem like we have any for the attribute functions on Element yet, but Element.test.html.js seems like it would be the right place to put it.

jpzwarte commented 1 year ago

@jmcgavin any chance you could incorporate the review comments? Do you mind if somebody takes over?

jmcgavin commented 1 year ago

@jpzwarte Apologies for the delay! I've addressed the comments that were left

jmcgavin commented 1 year ago

@bicknellr Thank you for the review and my apologies for the tardiness of my response. I've addressed your comments. Let me know if it requires additional changes or tests

bicknellr commented 1 year ago

Thanks for the updates!

I looked into the failing test and it's seems like an unrelated bug that was exposed by a new getHTML call. This line is unconditionally calling attributeChangedCallback even though it might be undefined:

https://github.com/webcomponents/polyfills/blob/0ab5679e40e1363cceb8e7f4d8279d3bdfebce1f/packages/scoped-custom-element-registry/src/scoped-custom-element-registry.js#L393

The whole outer iteration over observedAttributes could be nested in an if (definition.attributeChangedCallback) { ... } to work around this:

https://github.com/webcomponents/polyfills/blob/0ab5679e40e1363cceb8e7f4d8279d3bdfebce1f/packages/scoped-custom-element-registry/src/scoped-custom-element-registry.js#L391-L400

Also, could you add a change log entry? (example here)