sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.04k stars 4.08k forks source link

Make aria- dependencies optional #12241

Closed gterras closed 1 month ago

gterras commented 1 month ago

Describe the problem

The light shed on the latest state of the A11y repo in this issue is concerning to say the least. In a post-xz-attack world focus regarding third-party code should probably be stricter.

I write closed-circle apps that strictly don't need Aria. Until now as for others it was a minor annoyance clogging CI, logs and templates, and being a pain to shut down. Now in addition to this it's also some potential anytime shady code brute-committed to a repo with unclear ownership.

And at this point there is for me exactly the same number of reason to embed aria in my Svelte projects than there is to replace dequal with deep-equal-json in my package.json. I have trouble understanding why I should carry this burden.

People can already disable it anyway, just in a clunky and discouraging way. It's like a taboo.

To quote the dev team on this thread:

Engineering is about trade-offs.

The upcoming Svelte5 release is an great time to trade Aria requirement off and offer the ability to opt-out of shady npm manipulation.

Describe the proposed solution

aria?: boolean; 
DEFAULT: true 

Importance

i cannot use svelte without it

henrikvilhelmberglund commented 1 month ago

I'm guessing this isn't going to happen but warnings can be avoided with workspace settings: .vscode/settings.json

{
  "svelte.plugin.svelte.compilerWarnings": {
    "a11y_aria_attributes": "ignore",
    "a11y_incorrect_aria_attribute_type": "ignore",
    "a11y_unknown_aria_attribute": "ignore",
    "a11y_hidden": "ignore",
    "a11y_misplaced_role": "ignore",
    "a11y_unknown_role": "ignore",
    "a11y_no_abstract_role": "ignore",
    "a11y_no_redundant_roles": "ignore",
    "a11y_role_has_required_aria_props": "ignore",
    "a11y_accesskey": "ignore",
    "a11y_autofocus": "ignore",
    "a11y_misplaced_scope": "ignore",
    "a11y_positive_tabindex": "ignore",
    "a11y_invalid_attribute": "ignore",
    "a11y_missing_attribute": "ignore",
    "a11y_img_redundant_alt": "ignore",
    "a11y_label_has_associated_control": "ignore",
    "a11y_media_has_caption": "ignore",
    "a11y_distracting_elements": "ignore",
    "a11y_structure": "ignore",
    "a11y_mouse_events_have_key_events": "ignore",
    "a11y_missing_content": "ignore",
    "a11y_click_events_have_key_events": "ignore",
    "a11y_no_static_element_interactions": "ignore"
  }
}

svelte.config.ts

onwarn: (warning, handler) => {
    if (warning.code.startsWith('a11y')) {
      return;
    }
    handler(warning);
  },

Of course this is all pretty pointless if you write good HTML and you will probably be expected to make accessible sites when making publically facing sites so might as well learn it, the warnings basically tell you what to do.

gterras commented 1 month ago

All of this have been discussed in the linked thread, the point of this issue is that a11y went from convenient to overzealous to an npm manipulation scheme, and people should have the choice not to promote and carry this liability.

In this context the usual answer Yes but a11y is your friend even if you strictly don't need it suddenly lost a lot of weight.

dominikg commented 1 month ago

a11y is not a tradeoff, it is a requirement. These warnings are enabled by default for very good reasons and can be silenced via configuration (you shouldn't!). Making these checks depend on an optional (opt-in) peer dependency is not an option. Also axobject-query is dropping both dequal and deep-equal-json in favor of an inline check, so no more extra dependencies at all, in both v3 and v4.

gterras commented 1 month ago

I don't want this to become a duplicate of the other thread, so the main point is :

Also axobject-query is...

Is a repo that started acting in a truly unconventional manner and I don't want to support this. And you probably shouldn't too, for very good reasons.

As for the rest:

a11y is not a tradeoff, it is a requirement

It's not. Not technical, not legal. Not even moral. It's a strong recommendation for the most common case. Other cases are simply dismissed here. You can't rightfully write (you shouldn't!) while ignoring the context, which is... there is actually exactly no reason why I shouldn't.

Making these checks depend on an optional [...] is not an option

Same, not an option in your context, which is different from other's context. Writing forms without any kind of csrf protection is arguably even more not an option yet the compiler does not warn about it.

I understand the underlying postulates about a11y topic, and I even understand why it is coerced in Svelte, but pretending it should be unconditionally applied under any circumstances is purposely ignoring reality.

dominikg commented 1 month ago

as the threads you linked demonstrate, we are monitoring the situation around that dependency very closely. If you don't trust us to keep doing that, most package managers can be used to override and pin it to a specific version or even replace it with your own noop implementation.

We are not going to change the default a11y options to (almost) everyone's detriment just so that you don't have to use a little custom configuration for your closed circle app.

Importance i cannot use svelte without it

you can, but we don't force you to.

benmccann commented 1 month ago

Both dequal and deep-equal-json have been removed as dependencies, so the project now has zero dependencies: https://github.com/A11yance/axobject-query/commit/39030e7419ae9f9e198645240e47b6a3d9aaa14c

We caught this issue very quickly and axobject-query is part of a larger org with other maintainers also keeping an eye on it. Additionally, the latest version has been explicitly updated with an engines field specifying that it only supports node 6+ moving forward.

It is possible we'll add something along the lines of compilerOptions: { warnings: { disable: [list of codes to not warn about] } }.

gterras commented 1 month ago

We are not going to change the default a11y options to (almost) everyone's detriment just so that you don't have to use a little custom configuration for your closed circle app.

That is a quite contemptuous answer for someone that stood entirely corrected. My "little" use case currently makes for both the top liked issue and the top commented issue of the history of the repo. I understand you may take this topic personally but it shouldn't prevent you from seeing reality as it is.

We caught this issue very quickly and axobject-query is part of a larger org with other maintainers also keeping an eye on it.

@benmccann Thanks for a non-dogmatic answer. It's just that now I feel compelled to also do so since odds are it's a matter of time before we get to hear about this repo in a strange fashion. Right now the "yes this repo does shady stuff but we're monitoring it closely!" positioning feels bizarre.

It is possible we'll add something along the lines of compilerOptions: { warnings: { disable: [list of codes to not warn about] } }.

Hope some advancement will be made on the original thread. To quote the initial message, that yet again this very thread has validated:

People can already disable it anyway, just in a clunky and discouraging way. It's like a taboo.

benmccann commented 1 month ago

It's just that now I feel compelled to also do so

Some of this can be automated. E.g. there is tooling to check if you depend on discouraged packages (eslint-plugin-depend) or to check your dependency authors (npx dependency-maintainers) and many others

"yes this repo does shady stuff but we're monitoring it closely!" positioning feels bizarre.

Any dependency can add additional dependencies. This isn't an issue specific to even this one repository even if that's where it was most acute. We're always working on shrinking our dependency tree and it's something that takes continuous work rather than a one time effort

henrikvilhelmberglund commented 1 month ago

People can already disable it anyway, just in a clunky and discouraging way. It's like a taboo.

I disagree that it is clunky, you create a file and copy paste the code I posted earlier, it takes like 10 seconds. It is discouraged because disabling a11y warnings does not better the web and ignoring accessibility can be illegal, see ADA for US (and other countries have similar laws). These laws will likely be enforced more strictly at the government and state level (for their websites) in a few years, see https://www.ada.gov/resources/2024-03-08-web-rule/ .