Skip to content

Document ARIA mappings for string properties#753

Open
blinding-pixels wants to merge 3 commits into
AccessKit:mainfrom
blinding-pixels:Docs-first-pass
Open

Document ARIA mappings for string properties#753
blinding-pixels wants to merge 3 commits into
AccessKit:mainfrom
blinding-pixels:Docs-first-pass

Conversation

@blinding-pixels

Copy link
Copy Markdown

This is a first pass at #749, starting with the string properties as discussed.

The inline documentation covers the following -:

  • explains what each string property represents;
  • links to the corresponding ARIA concept or property where one exists;
  • notes when there is no direct ARIA equivalent; and
  • calls out specific differences for Value and KeyboardShortcut.

The newer row and column index text and Braille properties link to the WAI-ARIA 1.3 draft and are identified as draft equivalents.

This is intentionally limited to one property type so we can agree on the format and wording before expanding the documentation further.

I ran the following checks,-

  • cargo fmt --all -- --check
  • RUSTDOCFLAGS="-D warnings" cargo doc -p accesskit --no-deps
  • cargo test -p accesskit
    Happy to revise anything if required.

@DataTriny DataTriny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for starting this.

Overall I think the format is good, except mentionning when there is no equivalent.

Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
/// An [IETF language tag](https://www.rfc-editor.org/info/bcp47).
/// Only present when different from parent.
/// Only set this when it differs from the parent. There is no ARIA equivalent;
/// web content supplies this information through the host language.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to talk about web content specifically. Saying that the default value is dictated by the host platform is obvious but maybe needs to be documented? @PoignardAzur

Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated
Comment thread common/src/lib.rs Outdated

@DataTriny DataTriny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good. Just two more improvements from me.

I'd value @PoignardAzur's opinion on this before merging.

Comment thread common/src/lib.rs Outdated
/// web content supplies this information through the host language.
/// Only set this when it differs from the parent.
(Language, language, set_language, clear_language),
/// If a text input has placeholder text, it should be exposed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing an introductory sentence to explain the purpose.

From ARIA: "A short hint (a word or short phrase) intended to aid the user with data entry when the control has no value. A hint could be a sample value or a brief description of the expected format."

Then reword the mention of Node::label as ARIA does too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we clearly communicate the fact that placeholder and label can be used in conjunction, they serve different purposes. ARIA does it well, at least people can click on the link.

Comment thread common/src/lib.rs

@DataTriny DataTriny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Let's give Olivier a chance to go through this.

@PoignardAzur PoignardAzur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried running cargo doc and the generated docs seem fine. I think the format is good as-is.

Some of the descriptions seem a little vague about how this data will be interpreted, but that's par for the course with accessibility standards, and this will do for now.

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants