fix: call formDisabledCallback in response to fieldset disabled #90
Conversation
|
Yeah, this will capture the expected behavior for polyfilled browsers. I'd want to make sure it works for multiple instances of the form-associated element though. |
|
does the last commit cover your comment? |
|
It does. Thanks! |
|
854e684 has some code which might work, but I had some trouble testing. Is it the case that I should test in an isolated virtual machine with older browser versions? |
|
Sorry for the delay in getting to this one. Looks like a test is failing |
|
What problems have you had testing? |
|
with this, and a peppering of console.log calls throughout the polyfill code, i'm unable to verify that the polyfill is loading at all during that test, let alone whether the fieldsets are registering :/ describe.only('inside a custom element with a form and containing fieldset', () => {I sent an aspirational commit, maybe it will pass in CI 🤷 PTAL when you get a chance |
This is what I get when testing.Asides from that, I'm able to test the correct behaviour if i force the polyfill to always activate by changing the test to |
|
Can you run |
|
Diving in to this, the only effect that toggling disabled has on the internals host is calling the The test that we should have here is probably something along the lines of it('will respect the fieldset disabled attribute', async () => {
const form = await fixture<HTMLFormElement>(html`<form>
<fieldset>
<test-el></test-el>
</fieldset>
</form>`);
const fieldset = form.querySelector<HTMLFieldSetElement>('fieldset')!;
const testEl = form.querySelector<TestEl>('TestEl')!;
fieldset.disabled = true;
expect(testEl.formDisabledCallback.called).to.be.true;
});Unfortunately since let disabledCallbackCount = 0;
class TestEl extends HTMLElement {
static formAssociated = true;
internals = this.attachInternals();
formDisabledCallback() {
disabledCallbackCount += 1;
}
}
customElements.define('test-el', TestEl);
describe('The thing', () => {
afterEach(() => {
disabledCallbackCount = 0;
});
it('is a passing test', () => {
expect(isItAwesome).to.be.true;
});
});This is a naive example though. Sorry it's take me so long to get back on top of this, I was really burned out before the holidays, but am trying to balance things better now. |
|
This is great work, thanks for investigating, @calebdwilliams So if I understood correctly, that means the polyfill cannot provide |
|
Unfortunately I don’t think it can but there’s a lot I don’t know. I think @jonathantneal might have a way to do this with CSSOM shenanigans. Maybe he can ping us in the right direction. |
|
@bennypowers is this something you’re still interested in tackling? |
|
yes, i'll take a go at it in light of above comments tomorrow |
|
To support What would it replace Here’s how they could generate that reference. /** Return a unique selector for a specific element. */
let getUniqueSelector = (/** @type {Element} */ element) => {
/** Unique selector for this element */
let selector = ''
/** @type {Element} */
let parent
/** @type {number} */
let nth
while (parent = element.parentElement) {
for (nth = 1; element = element.previousElementSibling; ++nth);
selector = ' > :nth-child(' + nth + ')' + selector
element = parent
}
return ':is(:root, :host)' + selector
}And then elements could push that selector up to some map that replaces those states on selectors, like Happy to help try to contribute something like this, if it would be desired / helpful. If it isn’t, no worries. |
bennypowers
changed the title
|
Fixed in a different PR |
closes #88
@calebdwilliams does this test represent expected behavior?