-
Notifications
You must be signed in to change notification settings - Fork 189
Add Canvas widget #875
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
Add Canvas widget #875
Changes from all commits
3967c6e
e30fcfc
b4dfea6
ec5c653
033e5bd
b63a2c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,197 @@ | ||||||
| // Copyright 2025 the Xilem Authors and the Druid Authors | ||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||
|
|
||||||
| //! A canvas widget. | ||||||
|
|
||||||
| use std::sync::Arc; | ||||||
|
|
||||||
| use accesskit::{Node, Role}; | ||||||
| use smallvec::SmallVec; | ||||||
| use tracing::{Span, trace_span}; | ||||||
| use vello::Scene; | ||||||
| use vello::kurbo::Size; | ||||||
|
|
||||||
| use crate::core::{ | ||||||
| AccessCtx, AccessEvent, BoxConstraints, EventCtx, LayoutCtx, PaintCtx, PointerEvent, | ||||||
| PropertiesMut, PropertiesRef, QueryCtx, RegisterCtx, TextEvent, Update, UpdateCtx, Widget, | ||||||
| WidgetId, WidgetMut, | ||||||
| }; | ||||||
|
|
||||||
| /// A widget allowing custom drawing. | ||||||
| pub struct Canvas { | ||||||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||||||
| alt_text: Option<String>, | ||||||
| } | ||||||
|
|
||||||
| // --- MARK: BUILDERS --- | ||||||
| impl Canvas { | ||||||
| /// Create a new canvas with the given draw function. | ||||||
| pub fn new(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Self { | ||||||
| Self::from_arc(Arc::new(draw)) | ||||||
| } | ||||||
|
|
||||||
| /// Create a new canvas from a function already contained in an [`Arc`]. | ||||||
| pub fn from_arc(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Self { | ||||||
| Self { | ||||||
| draw, | ||||||
| alt_text: None, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Set the text that will be used to communicate the meaning of the canvas to | ||||||
| /// those using screen readers. | ||||||
| /// | ||||||
| /// Users are encouraged to set alt text for the canvas. | ||||||
| /// If possible, the alt-text should succinctly describe what the canvas represents. | ||||||
| /// | ||||||
| /// If the canvas is decorative or too hard to describe through text, users should set alt text to `""`. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PoignardAzur, do you have an example of something which is "too hard" to describe through text, but which should be a canvas? That is, if the "too hard" clause is for highly dynamic content, then it probably should be a bespoke widget with a proper accessibility implementation, and so the "too hard" clause doesn't apply. Would it be better for the alt text to say "we've chosen not to make this accessible because we can't be bothered" in that case?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The canonical way to say "We've chosen to not include this in the accessibility tree" is to add an empty string as alt-text.
For this kind of question, you must keep in the mind the experience of reading a page with a screen reader. Like, if you're reading an article or using an app, does it make sense to have the reader say "Menu, Help, Files, New Project, Open Project, We're sorry but we've chosen to not make this app accessible"? Does it help you navigate the app better? I don't have answers for the general case, and I'd advise against giving one. I want to discourage the mode of thinking that making an app accessible is about giving each widget a maximally descriptive alt text.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good point. My thinking here does need to be focused on the user experience, and having a snarky message isn't that. But I still think there is an important distinction between the two cases, and using the same handling for both is doing a disservice to the end user. That is, the cases are:
This sentence in the docs conflates those two cases, I think. Having both be the same case has one of two consequences:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is entirely correct.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one? You mean Daniel's?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. You were also correct about the distinction between no alt text and empty alt text. |
||||||
| pub fn with_alt_text(mut self, alt_text: impl Into<String>) -> Self { | ||||||
| self.alt_text = Some(alt_text.into()); | ||||||
| self | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // --- MARK: WIDGETMUT --- | ||||||
| impl Canvas { | ||||||
richard-uk1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /// Update the draw function | ||||||
| pub fn set_painter( | ||||||
| this: WidgetMut<'_, Self>, | ||||||
| draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static, | ||||||
| ) { | ||||||
| Self::set_painter_arc(this, Arc::new(draw)); | ||||||
| } | ||||||
|
|
||||||
| /// Update the draw function | ||||||
| pub fn set_painter_arc( | ||||||
| mut this: WidgetMut<'_, Self>, | ||||||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||||||
| ) { | ||||||
| this.widget.draw = draw; | ||||||
| this.ctx.request_render(); | ||||||
| } | ||||||
|
|
||||||
| pub fn set_alt_text(mut this: WidgetMut<'_, Self>, alt_text: String) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This not getting a warning for |
||||||
| this.widget.alt_text = Some(alt_text); | ||||||
| this.ctx.request_accessibility_update(); | ||||||
| } | ||||||
|
|
||||||
| pub fn remove_alt_text(mut this: WidgetMut<'_, Self>) { | ||||||
| this.widget.alt_text = None; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the semantics of no alt text as opposed to setting it to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd have to ask @mwcampbell for an authoritative answer, but from what I remember:
At least I think that's how ARIA treats it. |
||||||
| this.ctx.request_accessibility_update(); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // --- MARK: IMPL WIDGET --- | ||||||
| impl Widget for Canvas { | ||||||
| fn on_pointer_event( | ||||||
| &mut self, | ||||||
| _ctx: &mut EventCtx, | ||||||
| _props: &mut PropertiesMut, | ||||||
| _event: &PointerEvent, | ||||||
| ) { | ||||||
| } | ||||||
|
|
||||||
| fn accepts_pointer_interaction(&self) -> bool { | ||||||
| true | ||||||
| } | ||||||
|
|
||||||
| fn on_text_event( | ||||||
| &mut self, | ||||||
| _ctx: &mut EventCtx, | ||||||
| _props: &mut PropertiesMut, | ||||||
| _event: &TextEvent, | ||||||
| ) { | ||||||
| } | ||||||
|
|
||||||
| fn on_access_event( | ||||||
| &mut self, | ||||||
| _ctx: &mut EventCtx, | ||||||
| _props: &mut PropertiesMut, | ||||||
| _event: &AccessEvent, | ||||||
| ) { | ||||||
| } | ||||||
|
|
||||||
| fn register_children(&mut self, _ctx: &mut RegisterCtx) {} | ||||||
|
|
||||||
| fn update(&mut self, _ctx: &mut UpdateCtx, _props: &mut PropertiesMut, _event: &Update) {} | ||||||
|
|
||||||
| fn layout( | ||||||
| &mut self, | ||||||
| _ctx: &mut LayoutCtx, | ||||||
| _props: &mut PropertiesMut, | ||||||
| bc: &BoxConstraints, | ||||||
| ) -> Size { | ||||||
| // use as much space as possible - caller can size it as necessary | ||||||
| bc.max() | ||||||
| } | ||||||
|
|
||||||
| fn paint(&mut self, ctx: &mut PaintCtx, _props: &PropertiesRef, scene: &mut Scene) { | ||||||
| (self.draw)(scene, ctx.size()); | ||||||
| } | ||||||
|
|
||||||
| fn accessibility_role(&self) -> Role { | ||||||
| Role::Canvas | ||||||
| } | ||||||
|
|
||||||
| fn accessibility(&mut self, _ctx: &mut AccessCtx, _props: &PropertiesRef, node: &mut Node) { | ||||||
| if let Some(text) = &self.alt_text { | ||||||
| node.set_description(text.clone()); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| fn children_ids(&self) -> SmallVec<[WidgetId; 16]> { | ||||||
| SmallVec::new() | ||||||
| } | ||||||
|
|
||||||
| fn make_trace_span(&self, ctx: &QueryCtx<'_>) -> Span { | ||||||
| trace_span!("Canvas", id = ctx.widget_id().trace()) | ||||||
| } | ||||||
|
|
||||||
| fn get_debug_text(&self) -> Option<String> { | ||||||
| self.alt_text.clone() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // --- MARK: TESTS --- | ||||||
| #[cfg(test)] | ||||||
| mod tests { | ||||||
| use insta::assert_debug_snapshot; | ||||||
| use vello::kurbo::{Affine, BezPath, Stroke}; | ||||||
| use vello::peniko::{Color, Fill}; | ||||||
|
|
||||||
| use super::*; | ||||||
| use crate::assert_render_snapshot; | ||||||
| use crate::testing::TestHarness; | ||||||
|
|
||||||
| #[test] | ||||||
| fn simple_canvas() { | ||||||
| let canvas = Canvas::new(|scene, size| { | ||||||
| let scale = Affine::scale_non_uniform(size.width, size.height); | ||||||
| let mut path = BezPath::new(); | ||||||
| path.move_to((0.1, 0.1)); | ||||||
| path.line_to((0.9, 0.9)); | ||||||
| path.line_to((0.9, 0.1)); | ||||||
| path.close_path(); | ||||||
| path = scale * path; | ||||||
| scene.fill( | ||||||
| Fill::NonZero, | ||||||
| Affine::IDENTITY, | ||||||
| Color::from_rgb8(100, 240, 150), | ||||||
| None, | ||||||
| &path, | ||||||
| ); | ||||||
| scene.stroke( | ||||||
| &Stroke::new(4.), | ||||||
| Affine::IDENTITY, | ||||||
| Color::from_rgb8(200, 140, 50), | ||||||
| None, | ||||||
| &path, | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| let mut harness = TestHarness::create(canvas); | ||||||
|
|
||||||
| assert_debug_snapshot!(harness.root_widget()); | ||||||
| assert_render_snapshot!(harness, "hello"); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(I'm not sure if |
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| source: masonry/src/widgets/canvas.rs | ||
| expression: harness.root_widget() | ||
| --- | ||
| Canvas |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Copyright 2024 the Xilem Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| use std::sync::Arc; | ||
|
|
||
| use masonry::widgets; | ||
| use vello::Scene; | ||
| use vello::kurbo::Size; | ||
|
|
||
| use crate::core::{DynMessage, Mut, ViewMarker}; | ||
| use crate::{MessageResult, Pod, View, ViewCtx, ViewId}; | ||
|
|
||
| /// A non-interactive text element. | ||
| /// # Example | ||
| /// | ||
| /// ```ignore | ||
| /// use xilem::palette; | ||
| /// use xilem::view::label; | ||
| /// use masonry::TextAlignment; | ||
| /// use masonry::parley::fontique; | ||
| /// | ||
| /// label("Text example.") | ||
| /// .brush(palette::css::RED) | ||
| /// .alignment(TextAlignment::Middle) | ||
| /// .text_size(24.0) | ||
| /// .weight(FontWeight::BOLD) | ||
| /// .with_font(fontique::GenericFamily::Serif) | ||
| /// ``` | ||
|
Comment on lines
+13
to
+28
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these docs are right...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops sorry I missed this! |
||
| pub fn canvas(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Canvas { | ||
| Canvas { | ||
| draw: Arc::new(draw), | ||
| alt_text: None, | ||
| } | ||
| } | ||
|
|
||
| /// Create a canvas view. | ||
| #[must_use = "View values do nothing unless provided to Xilem."] | ||
| pub struct Canvas { | ||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||
| alt_text: Option<String>, | ||
| } | ||
|
|
||
| impl Canvas { | ||
| /// Sets alt text for the contents of the canvas. | ||
| /// | ||
| /// Users are strongly encouraged to provide alt text for accessibility tools | ||
| /// to use. | ||
| pub fn alt_text(mut self, alt_text: String) -> Self { | ||
| self.alt_text = Some(alt_text); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl ViewMarker for Canvas {} | ||
| impl<State, Action> View<State, Action, ViewCtx> for Canvas { | ||
| type Element = Pod<widgets::Canvas>; | ||
| type ViewState = (); | ||
|
|
||
| fn build(&self, ctx: &mut ViewCtx) -> (Self::Element, Self::ViewState) { | ||
| let widget = widgets::Canvas::from_arc(self.draw.clone()); | ||
|
|
||
| let widget_pod = ctx.new_pod(widget); | ||
| (widget_pod, ()) | ||
| } | ||
|
|
||
| fn rebuild( | ||
| &self, | ||
| prev: &Self, | ||
| (): &mut Self::ViewState, | ||
| _ctx: &mut ViewCtx, | ||
| element: Mut<Self::Element>, | ||
| ) { | ||
| if !Arc::ptr_eq(&self.draw, &prev.draw) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is... always true, because Essentially, you'd need to combine it with something like |
||
| widgets::Canvas::set_painter_arc(element, self.draw.clone()); | ||
| } | ||
| } | ||
|
|
||
| fn teardown(&self, (): &mut Self::ViewState, _: &mut ViewCtx, _: Mut<Self::Element>) {} | ||
|
|
||
| fn message( | ||
| &self, | ||
| (): &mut Self::ViewState, | ||
| _id_path: &[ViewId], | ||
| message: DynMessage, | ||
| _app_state: &mut State, | ||
| ) -> MessageResult<Action> { | ||
| tracing::error!( | ||
| "Message arrived in Canvas::message, but Canvas doesn't consume any messages, this is a bug" | ||
| ); | ||
| MessageResult::Stale(message) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be slightly better style to indent this to match the start of the string, rather than the
panic