Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NcCheckboxRadioSwitch): only bind aria attributes to the input #5777

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

@skjnldsv skjnldsv self-assigned this Jul 4, 2024
@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews regression Regression of a previous working feature feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Jul 4, 2024
@susnux
Copy link
Contributor

susnux commented Jul 4, 2024

I am not sure here, this is not what I would expect, I use a checkbox component. So the only thing I can expect is that the component is a checkbox.
If I assign any attributes then those are expected to be on the checkbox not on any random unknown element.

Instead I think the tests on server are broken by design, they assume some internal component layout that is not guaranteed. It is not guaranteed that outer-element-selector input does exist.
We could even have <div class="the checkbox"><span role="checkbox"></div> as the component structure.

So instead you should probably use cy.findByRole('checkbox', { name: 'The checkbox accessible name' }) because this is what an app can control.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 4, 2024

Instead I think the tests on server are broken by design, they assume some internal component layout that is not guaranteed. It is not guaranteed that outer-element-selector input does exist.
We could even have <div class="the checkbox"><span role="checkbox"></div> as the component structure.

The tests itself could have been written differently, but this is not really the issue here. The tests failure just made us notice this change. We are touching a different dilemma about what we expect.
I personally think dropping all attributes to the wrapper is a dangerous and counter-intuitive pattern to use.

That's why in the past we always declared aria variable as props, so that we can see all the attributes that are forwarded to the background, and document them.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 4, 2024

If I assign any attributes then those are expected to be on the checkbox not on any random unknown element.

Even if that was the case, this change should not have been a minor nor a patch. Whatever decision taken prior, even if less than ideal, should stay consistent :)

Anyway, let's move forward, while I think and would like us to be more careful in the future, I think we can indeed adjust and only keep data attributes on the outer layer. I'm too deep into other areas to notice potential missed pitfalls. As this is your code @susnux, i'll let you take the decision.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 4, 2024

Server fixed with this PR
image

@susnux
Copy link
Contributor

susnux commented Jul 4, 2024

Even if that was the case, this change should not have been a minor nor a patch. Whatever decision taken prior, even if less than ideal, should stay consistent :)

It is because that it internal, for components the structure is private like for classes private methods or properties.
This change here is more like forcing people to only use the public API, like if you change your JS class from

class Foo {
_private: 'foo'
}

to

class Foo {
#private: 'foo'
}

Same problem we have with class names because we do not use two way scoping, people use internal class names of components which is dangerous as that is nothing public. Meaning even accessing .button-vue might work there is no good reason for us not to change it to something different anytime.
Of course we do not do it because we would bit our own tail as we then have to fix a lot of stuff.

To sum it up, what I want to say: If you have the possibility to make tests less fragile, we should do it.
But in this case I agree if it affects a lot of places, then we should do this here for DX! (BTW I already work with low priority on more robust tests for account management)

@susnux susnux removed the regression Regression of a previous working feature label Jul 4, 2024
@susnux susnux added this to the 8.14.0 milestone Jul 4, 2024
@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 4, 2024

To sum it up, what I want to say: If you have the possibility to make tests less fragile, we should do it.
But in this case I agree if it affects a lot of places, then we should do this here for DX! (BTW I already work with low priority on more robust tests for account management)

I agree with any internal structure, it's subject to change and it's fine.

This is not what it is about here. We're preventing any dev applying standard html attributes to a component now. We're not talking about anything private or public, we're literally stripping away the very core of a component rendering, controlling the outer layer.

What if I want to add some styling to the component, will it be applied to the input now?
That would be problematic.

EDIT: answering my own question, <NcCheckboxRadioSwitch style="whatever" /> is applied to the input and not the component root. That is not acceptable to me 😕

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 4, 2024
@susnux
Copy link
Contributor

susnux commented Jul 4, 2024

EDIT: answering my own question, <NcCheckboxRadioSwitch style="whatever" /> is applied to the input and not the component root. That is not acceptable to me 😕

@skjnldsv no it is not for Vue2 style and class are considered special attributes and always bound to parent.
Meaning if you want to apply styling you still can assign class or style to NcCheckboxRadiosSwitch and it will be placed on the wrapper element (while style is probably bad as inline-style is considered insecure and we should move away from it to someday allow app developers using CSP without unsafe-inline)

image

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@susnux
Copy link
Contributor

susnux commented Jul 4, 2024

@skjnldsv Pushed a test to ensure attributes like class and style are correctly set.

…lass and style are correctly set

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working Developer experience DX feature: checkbox-radio-switch Related to the checkbox-radio-switch component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants