Add Canvas widget #875

richard-uk1 · 2025-02-24T01:05:28Z

I'm playing with making a vector path editor in xilem. This requires some extra stuff over what is currently available so I'm going to maintain a fork. I will try to merge upstream anything I think is more widely useful.

This PR allows the user to draw directly onto the scene fragment for a widget. Currently, the user must recreate the draw function every time there is a change in state. I wonder if there should be separated out state and draw parts, so the function needs to be updated less often.

DJMcNab

Apart from the pointer event TODO comment, this seems reasonable as an addition to Masonry.

DJMcNab · 2025-02-24T09:30:24Z

masonry/src/widgets/canvas.rs

+// --- MARK: IMPL WIDGET ---
+impl Widget for Canvas {
+    fn on_pointer_event(&mut self, _ctx: &mut EventCtx, _event: &PointerEvent) {
+        // TODO: ensure coordinates are correct and pass to callback


This comment is misleading. Something has gone wrong if the canvas widget is managing the pointer event. At that point, the user should be writing their own widget, I believe.

A canvas is basically equivalent to an Image, but containing a vector scene (ish).

DJMcNab · 2025-02-24T09:31:48Z

masonry/src/widgets/canvas.rs

+    }
+
+    fn accessibility(&mut self, _ctx: &mut AccessCtx, _node: &mut Node) {
+        // TODO: should probably give the caller the opportunity to handle accessibility


This would probably just be an alt-text like mechanism, although maybe we should also be strongly encouraging that a caption exist?

We haven't handled this for Image, which this is very similar to.

richard-uk1 · 2025-02-24T21:37:06Z

One question I have: can a scene fragment be retained and appended to the scene on paint?

DJMcNab · 2025-02-25T08:37:09Z

Masonry is already doing that internally; your paint callback will only be called once, except when it gets replaced or the layout changes

richard-uk1 · 2025-02-27T12:48:03Z

OK ready for review.

richard-uk1 · 2025-02-27T12:48:17Z

How is the CI so fast!?!?!

masonry/src/widgets/canvas.rs

PoignardAzur · 2025-03-01T12:30:39Z

masonry/src/widgets/canvas.rs

+    fn accepts_pointer_interaction(&self) -> bool {
+        false
+    }


I don't think you should set this flag to false. It makes Canvas transparent to pointers, meaning for example that if there's a widget behind the canvas, that widget will get pointer events, affect the pointer icon, etc.

I'm not certain that I agree; I can see arguments either way (since the canvas isn't filled by default, for example). But since if it did intercept pointer events, they would still bubble up to the parents, then either way is fine.

I suggest setting it to true for now, then seeing how that works in practice.

PoignardAzur · 2025-03-01T12:32:32Z

masonry/src/widgets/canvas.rs

+    fn paint(&mut self, ctx: &mut PaintCtx, scene: &mut Scene) {
+        (self.draw)(scene, ctx.size());
+    }


So, to be clear, this is a static canvas, right? The draw function isn't expected to paint different things from frame to frame?

I think so - it could be used for things like icons. But what should be different if it were more dynamic?

masonry/src/widgets/canvas.rs

Co-authored-by: Olivier FAURE <couteaubleu@gmail.com>

richard-uk1 · 2025-03-10T15:24:38Z

Should be ready for review again (assuming CI is green). I will squash before merging, as I don't think there is anything to be gained by keeping the commits.

Philipp-M · 2025-03-10T19:41:54Z

I will squash before merging, as I don't think there is anything to be gained by keeping the commits.

Not necessary though, as this is the default anyways when merging (with the title/text in the PR top comment as commit description), so keeping the commits should give a better timeline of this PR shouldn't it?

DJMcNab · 2025-03-11T08:27:05Z

xilem/src/view/canvas.rs

+/// 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)
+/// ```


I don't think these docs are right...

Oops sorry I missed this!

DJMcNab · 2025-03-11T08:37:20Z

xilem/src/view/canvas.rs

+        _ctx: &mut ViewCtx,
+        element: Mut<Self::Element>,
+    ) {
+        if !Arc::ptr_eq(&self.draw, &prev.draw) {


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)

DJMcNab · 2025-03-11T08:38:10Z

masonry/src/widgets/canvas.rs

+        let mut harness = TestHarness::create(canvas);
+
+        assert_debug_snapshot!(harness.root_widget());
+        assert_render_snapshot!(harness, "hello");


Suggested change

assert_render_snapshot!(harness, "hello");

assert_render_snapshot!(harness, "stroked_triangle");

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

DJMcNab · 2025-03-11T08:40:43Z

masonry/src/widgets/canvas.rs

+        this.ctx.request_render();
+    }
+
+    pub fn set_alt_text(mut this: WidgetMut<'_, Self>, alt_text: String) {


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)

DJMcNab · 2025-03-11T08:41:51Z

masonry/src/widgets/canvas.rs

+    }
+
+    pub fn remove_alt_text(mut this: WidgetMut<'_, Self>) {
+        this.widget.alt_text = None;


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)

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.

DJMcNab · 2025-03-11T08:46:33Z

masonry/src/widgets/canvas.rs

+    /// 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 `""`.


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

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.

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:

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.

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:

It just completely hides that important information is missing from the user; i.e. creates an unknown unknown; OR

It forces the user to doubt all items which are indicated as display-only

This comment is entirely correct.

Which one? You mean Daniel's?

Yes. You were also correct about the distinction between no alt text and empty alt text.

DJMcNab · 2025-03-11T08:47:14Z

masonry/src/testing/harness.rs

-            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`"


I think it would be slightly better style to indent this to match the start of the string, rather than the panic

DJMcNab

(Marking as request changes for the sake of GitHub's UI, to indicate that this is waiting on the author)

Any new widget code needs to have full docs. Not having `missing_docs` trigger by default for new widgets is quite bad. See #875 and #882 for cases where this over-broad allow has bitten us. The comment about not using `expect(missing_docs)` because of rust-analyzer is confusing to me; I don't run into an issue. It might have been rust-lang/rust#130021, which is now fixed. That comment should have had a link to an upstream issue for more context.

DJMcNab reviewed Feb 24, 2025

View reviewed changes

richard-uk1 force-pushed the canvas_widget branch 2 times, most recently from 594c48d to 0326db5 Compare February 27, 2025 12:41

richard-uk1 requested a review from DJMcNab February 27, 2025 16:33

PoignardAzur changed the title ~~add canvas widget~~ Add Canvas widget Mar 1, 2025

PoignardAzur reviewed Mar 1, 2025

View reviewed changes

richard-uk1 and others added 4 commits March 10, 2025 15:15

add canvas widget

3967c6e

impl suggestions and add xilem view for canvas

e30fcfc

Apply suggestions from code review

b4dfea6

Co-authored-by: Olivier FAURE <couteaubleu@gmail.com>

implement suggestions

ec5c653

richard-uk1 force-pushed the canvas_widget branch from fb12fbe to ec5c653 Compare March 10, 2025 15:15

richard-uk1 added 2 commits March 10, 2025 15:19

fix after rebase

033e5bd

fix fmt

b63a2c8

richard-uk1 requested a review from PoignardAzur March 10, 2025 15:43

DJMcNab reviewed Mar 11, 2025

View reviewed changes

DJMcNab requested changes Mar 17, 2025

View reviewed changes

DJMcNab mentioned this pull request Mar 19, 2025

Make allow(missing_docs) for widgets much more tightly scoped #898

Merged

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

Add Canvas widget #875

Are you sure you want to change the base?

Add Canvas widget #875

Uh oh!

Conversation

richard-uk1 commented Feb 24, 2025

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richard-uk1 commented Feb 24, 2025

Uh oh!

DJMcNab commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard-uk1 commented Feb 27, 2025

Uh oh!

richard-uk1 commented Feb 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

richard-uk1 commented Mar 10, 2025

Uh oh!

Philipp-M commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PoignardAzur Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DJMcNab commented Feb 25, 2025 •

edited

Loading

PoignardAzur Mar 14, 2025 •

edited

Loading