Add Canvas widget - take 3 by PoignardAzur · Pull Request #1506 · linebender/xilem

PoignardAzur · 2025-11-30T12:23:24Z

Based on @richard-uk1's #875 and @philocalyst's #1445.

Add Canvas widget to Masonry.
Add canvas element to Xilem.

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

Returning childids now

Made no op as it previosly did not exist

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

…uilds

Unneeded &mut

I can't find the vello rexport :(

@Philipp-M

Thanks to @Philipp-M!!! I forgot about the suggestion

RagibHasin

Looks good to me. Sorry for the late review.

RagibHasin · 2025-12-06T02:48:09Z

+    WidgetMut,
+};
+
+// TODO - Add background color?


I think it is better done in the draw function?

Yeah I would just remove TODO, I think it's a good default to draw nothing when nothing is specified in draw.

RagibHasin · 2025-12-06T02:48:56Z

+    fn on_pointer_event(
+        &mut self,
+        _ctx: &mut EventCtx<'_>,
+        _props: &mut PropertiesMut<'_>,
+        _event: &PointerEvent,
+    ) {
+    }


Shouldn't methods with default impls be omitted?

Philipp-M

There's an issue in the View::rebuild implementation, otherwise looks good.

Philipp-M · 2025-12-07T11:07:20Z

+    WidgetMut,
+};
+
+// TODO - Add background color?


Yeah I would just remove TODO, I think it's a good default to draw nothing when nothing is specified in draw.

Philipp-M · 2025-12-07T11:07:25Z

+/// That Scene is then used as the canvas' contents.
+pub struct Canvas {
+    draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>,
+    alt_text: Option<String>,


Doesn't hurt to have an Option here, but it's not necessary, we can just use the property is_empty for that.

The thing is, "alt text is unset" and "alt text is set to empty string" have different semantic meanings, though I'm not sure whether screen readers really interpret them that differently in practice.

Philipp-M · 2025-12-07T11:07:57Z

+        this.widget.draw = draw;
+        this.ctx.request_render();


Something I'm wondering for a longer time already, is whether we should check within these setters whether really something changed (i.e. in this case if the arc pointer has changed), and only then request a render.

As we often (always?) do this in Xilem it's not necessary here for Xilem at least, but I think it might be nice to have this built into masonry?

See also https://github.com/ricvelozo/xilem/pull/1/files#diff-7d717e7fd640e50cef624a6fa2afb9c20968470e5849c8b3b0abe9782499814eR67-R72
where I did this.

I don't know. In general, that kind of reasoning is how you end up with multiple layers of caching in your app that all check whether some parameter changed.

In practice, repainting is usually cheap and having Masonry always repaint something when a parameter is set is more predictable, so I'd lean towards the current design.

Philipp-M · 2025-12-07T11:09:15Z

+    /// Set the text that will describe the canvas to screen readers.
+    ///
+    /// See [`Canvas::with_alt_text`] for details.
+    pub fn set_alt_text(mut this: WidgetMut<'_, Self>, alt_text: String) {
+        this.widget.alt_text = Some(alt_text);
+        this.ctx.request_accessibility_update();
+    }
+
+    /// Remove the canvas' alt text.
+    ///
+    /// See [`Canvas::with_alt_text`] for details.
+    pub fn remove_alt_text(mut this: WidgetMut<'_, Self>) {
+        this.widget.alt_text = None;
+        this.ctx.request_accessibility_update();
+    }


See the comment below, I think we can just sum this up in set_alt_text with alt_text: String (or ArcStr)

I think we might just want to pass an Option<String> and merge them.

Philipp-M · 2025-12-07T11:11:00Z

+        if self.alt_text != prev.alt_text
+            && let Some(alt_text) = &self.alt_text
+        {
+            widgets::Canvas::set_alt_text(element, alt_text.clone());
+        }


I don't think this is safe?

When alt_test was Some and changed to None it won't be updated in masonry AFAICS.
So in the current state of this PR remove_alt_text needs to be called (but as described above, I think we can simplify this and the masonry code by not using an Option at all)

Philipp-M · 2025-12-07T11:14:03Z

+/// # my_canvas
+/// # }
+/// ```
+pub fn canvas(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas {


Btw. I think we should probably specify in the doc-comment, that this Arc<dyn Fn> should be ideally saved within the app state, otherwise it always results in a rerender. Ideally show how this is done better in the doc-comment example

Maybe we can also use a function pointer instead, to avoid allocs and (mostly) unnecessary generics?

Alternative to #1506 (actually it builds on top of it again and applies the review comments/suggestions, quite a group effort :)) I was bugged by the `Arc<dyn Fn>` and the way it was handled. IMO it's a little bit better layered this way and more predictable when the canvas draw function is actually changed, and allows the app state in the canvas draw function by using `Fn(Arg<'_, State>, &mut Scene, Size)` instead of just `Arc<dyn Fn(&mut Scene, Size)>` which would require capturing in any realistic use-case (accessing the user state) and thus the `Arc` would be somewhat useless, as always a new one needs to be created. Additionally I've added an example (as I think this is good practice when adding new widgets). (also to check whether this results in latency, i.e. when pressing the button, that the scene is updated immediately instead of a frame-delay (fortunately this doesn't result in frame-delay)) --------- Co-authored-by: Richard Dodd <richard.o.dodd@gmail.com> Co-authored-by: Olivier FAURE <couteaubleu@gmail.com> Co-authored-by: Myles Wirth <regime_skylark.0y@icloud.com> Co-authored-by: Miles Wirht <114884788+philocalyst@users.noreply.github.com> Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Co-authored-by: Muhammad Ragib Hasin <ragib.hasin@gmail.com>

RagibHasin · 2025-12-13T03:49:41Z

Completed in #1519

richard-uk1 and others added 30 commits November 30, 2025 12:37

add canvas widget

43856af

impl suggestions and add xilem view for canvas

1baa22e

Apply suggestions from code review

0d43e8a

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

implement suggestions

df55e68

fix after rebase

3e973d2

fix fmt

43a6cfd

Removed smallvec reliancy

2a0f296

Returning childids now

fixed signature, replacing queryctx with widgetid

afd330a

Made explicit hidden lifetimes

a30deae

fixed test harness import

62ca993

Fixed the harness running method, by pulling in props

2e03fad

Added the associated type Action

2f8957f

Made no op as it previosly did not exist

Fixed function signatures

053ff93

Changed name from new pod to create pod

aaa629d

Removed unused imports and parameters

07f2eb3

Documentation for the canvas accessibility methods

593cf85

removed needless qualification

8e36ac6

fixed test snapshot name

0d2dedc

Renamed hello.new to hello_new

288d062

Fixed the harness name (whoops)

2af781d

Removed unused files

47300c5

Removed unwanted debug snapshot test

ed36469

Renamed the testing snapshot to match test name

7d1f65e

Expanded the documentation in public-facing canvas api

4891caf

Removed needless image

bd83e3b

Fixed the example and enabled as required doc test

3b3cf61

Added alt text support on the "build" method

d162f63

Update masonry/src/widgets/canvas.rs

1e722dd

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Moved associated top to the beginning of impl

ffdd9cf

Fixed the fucntion signature of canvas

e5ce6ae

philocalyst and others added 17 commits November 30, 2025 12:38

Using vello through xilem

ce63927

Modified signatures and calls to effectively handle Canvas during reb…

b65db9e

…uilds

add canvas widget

d5da439

Fixed view location mismatch

a6f35cd

Renamed to messagectx

2e045f1

Fixed clippy lint

5310240

Unneeded &mut

Updated signatures

b3f677a

Fixed example

21ee038

I can't find the vello rexport :(

Incorporated suggestions for type-checked return value

82e54b6

Thanks to @Philipp-M!!! I forgot about the suggestion

Fix imports

554c799

Fix up screenshot files

d3a35b9

Remove extra annotation

f26556f

Fix vscode markers

af646b4

Implement clipping

403ecd7

Add comment

404cdd2

Update doc comments

c61251e

Fix doc test

c56864c

PoignardAzur requested review from DJMcNab, Philipp-M and RagibHasin December 2, 2025 14:11

RagibHasin approved these changes Dec 6, 2025

View reviewed changes

RagibHasin reviewed Dec 6, 2025

View reviewed changes

Philipp-M requested changes Dec 7, 2025

View reviewed changes

Philipp-M reviewed Dec 7, 2025

View reviewed changes

Philipp-M mentioned this pull request Dec 7, 2025

Add Canvas widget #1519

Merged

RagibHasin closed this Dec 13, 2025

Conversation

PoignardAzur commented Nov 30, 2025

Uh oh!

RagibHasin left a comment • 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!

Philipp-M Dec 7, 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!

Philipp-M left a comment

Choose a reason for hiding this comment

Uh oh!

Philipp-M Dec 7, 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!

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!

RagibHasin commented Dec 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RagibHasin left a comment •

edited

Loading

Philipp-M Dec 7, 2025 •

edited

Loading

Philipp-M Dec 7, 2025 •

edited

Loading