Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion masonry/src/testing/harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,10 @@ impl TestHarness {
// Remove '<test_name>.new.png' file if it exists
let _ = std::fs::remove_file(&new_path);
new_image.save(&new_path).unwrap();
panic!("Snapshot test '{test_name}' failed: No reference file");
panic!(
"Snapshot test '{test_name}' failed: No reference file\n\
New screenshot created with `.new.png` extension. If correct, change to `.png`"
Copy link
Member

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

);
}
}
}
197 changes: 197 additions & 0 deletions masonry/src/widgets/canvas.rs
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 `""`.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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?

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.

Copy link
Member

Choose a reason for hiding this comment

The 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:

  1. Not in the accessibility tree/only for visual display purposes. In that case setting to the empty string is the right behaviour. I agree completely with that.
  2. The content of this widget is potentially important to all users, but we've decided not to make it accessible because doing so would be "too hard". Add Canvas widget #875 (comment) seems to answer this; that if it's "too hard" to add alt text, then we should just not add it. That would then use the screen reader's default handling of "developer didn't bother to make this accessible".

This sentence in the docs conflates those two cases, I think. Having both be the same case has one of two consequences:

  1. It just completely hides that important information is missing from the user; i.e. creates an unknown unknown; OR
  2. It forces the user to doubt all items which are indicated as display-only

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is entirely correct.

Copy link
Contributor

@PoignardAzur PoignardAzur Mar 14, 2025

Choose a reason for hiding this comment

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

Which one? You mean Daniel's?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
/// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

This not getting a warning for missing_docs suggests that something is wrong here. (It's fine for the docs to just point to Canvas::with_alt_text)

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;
Copy link
Member

Choose a reason for hiding this comment

The 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 ""? (cc @PoignardAzur, as you suggest setting it to Some(String::new()) if it's not a semantically meaningful canvas)

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • Setting no alt-text means you "forgot" to add it, and some screen reader tools might add a default one for you.
  • Setting an empty alt text means you consider that the item shouldn't be described by screen readers.

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");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_render_snapshot!(harness, "hello");
assert_render_snapshot!(harness, "stroked_triangle");

(I'm not sure if _ or - is more idiomatic here)

}
}
2 changes: 2 additions & 0 deletions masonry/src/widgets/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod tests;

mod align;
mod button;
mod canvas;
mod checkbox;
mod flex;
mod grid;
Expand All @@ -31,6 +32,7 @@ mod zstack;

pub use self::align::Align;
pub use self::button::Button;
pub use self::canvas::Canvas;
pub use self::checkbox::Checkbox;
pub use self::flex::{Axis, CrossAxisAlignment, Flex, FlexParams, MainAxisAlignment};
pub use self::grid::{Grid, GridParams};
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
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
92 changes: 92 additions & 0 deletions xilem/src/view/canvas.rs
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
Copy link
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 these docs are right...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

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

This is... always true, because canvas always allocates. I think the only sound way to handle this currently is to force the user to provide an already Arced function. That's... far from ideal from an ergonomics perspective, but it matches the semantics you need.

Essentially, you'd need to combine it with something like memoize for this API to be reasonable; it's the unfortunate ergonomics papercut around closures being incomparable (rust-lang/rfcs#3538)

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)
}
}
3 changes: 3 additions & 0 deletions xilem/src/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ pub use worker::*;
mod button;
pub use button::*;

mod canvas;
pub use canvas::*;

mod checkbox;
pub use checkbox::*;

Expand Down