add some accessibility features to web widgets #292
Conversation
| @@ -118,8 +131,7 @@ where | |||
| vdom.schedule_render(); | |||
| }) | |||
| .finish(), | |||
| span(bump).children(vec![ | |||
There was a problem hiding this comment.
i don't think this span is necessary, and the other inputs don't have it. so i removed it for consistency
| /// ``` | ||
| #[derive(Debug)] | ||
| pub struct Image { | ||
| /// The image path | ||
| pub handle: Handle, | ||
|
|
||
| /// The alt text of the image | ||
| pub alt: String, |
There was a problem hiding this comment.
images should always have alt text unless they are purely presentational, e.g. a background image. im not sure the best way to go about making this not necessary for those sorts of images, and having alt text be required feels like a good default
There was a problem hiding this comment.
Could always be passed an empty string? Not sure how that works on screen readers and such though.
There was a problem hiding this comment.
it could. i wasn't sure that implementing some sort of length requirement would be the correct thing to do since it would end up just being arbitrary. and i feel like most people ignore accessibility out of ignorance rather than malice, so having a reminder that alt text is required would be enough for most cases
There was a problem hiding this comment.
but it seems like adding them as builder methods as @hecrj suggested will result in less empty alt tags since people will have to opt in
There was a problem hiding this comment.
We can make alt text mandatory once we tackle accessibility in the native side of the ecosystem.
web/src/widget/text_input.rs
Outdated
| /// - an optional placeholder | ||
| /// - a label describing the input | ||
| /// - an id |
There was a problem hiding this comment.
placeholders aren't replacements for labels. i made the placeholder optional so that people can decide whether they want one or not, but made label text required
web/src/widget/text_input.rs
Outdated
| @@ -67,7 +75,13 @@ impl<'a, Message> TextInput<'a, Message> { | |||
| { | |||
| Self { | |||
| _state: state, | |||
| placeholder: String::from(placeholder), | |||
| placeholder: if let Some(p) = placeholder { | |||
There was a problem hiding this comment.
i'm not sure if there is a better way to do this. i did it this way because i wasn't sure how to conditionally add an attribute. it seems like there should be, but i don't know enough about dodrio
There was a problem hiding this comment.
placeholder.map(Into::into).unwrap_or_default() could work
web/src/widget/text_input.rs
Outdated
| .attr("for", text_input_id) | ||
| .children(vec
There was a problem hiding this comment.
The main challenge here is that we need to keep the intersection of iced_web and iced_native public APIs compatible. I believe it would be best to wait until we decide it's time to tackle accessibility for native platforms.
In the meantime, we could maybe add some of the accessibility information as additional builder methods, which would only be available when targetting Wasm.
|
okay, that makes sense i'll rework this into builder methods |
TomPridham
force-pushed
the
feature/accessibility-web
branch
from
a0cc6b1 to
8c4ea34
Compare
TomPridham
force-pushed
the
feature/accessibility-web
branch
from
8c4ea34 to
852d597
Compare
|
@hecrj, i updated this to use builder methods. i ended up just reverting the text input changes since it would require a lot more work to implement those changes |
starts on #282, at least for web
adds some basic accessibility functionality to radio buttons, checkboxes, text inputs, and images.