Implement paint and accessibility passes #522

PoignardAzur · 2024-08-17T09:45:01Z

No description provided.

PoignardAzur · 2024-08-17T09:46:14Z

(This is very much a draft, more so than usual. ~~Accessibility isn't ported yet~~, and there's a bunch of changes I deliberately delayed to avoid creating huge diffs.)

PoignardAzur · 2024-08-20T11:18:44Z

I think this is ready for review now. I left some things for later (notably removing WidgetPod::paint and WidgetPod::accessibility and using a more general shape for the clip path) because the diff is large enough as it is.

I don't intend to add a default implementation for paint and accessibility, because I think widgets that don't need to do anything with them will be rare (even eg Flex will get properties like a background color); for accessibility in particular, I think we should encourage people to look at accessibility tree values before decide they don't need any, and having a default impl goes against that.

jaredoconnell

Looks good. I just have one minor comment.

jaredoconnell · 2024-08-20T13:08:47Z

masonry/src/contexts.rs

+        self.widget_state.request_accessibility = true;
+        self.widget_state.needs_accessibility = true;
+        self.widget_state.needs_paint = true;


I've seen this repeated at least three times. It may make sense to create a function just for marking the state change for needing paint and accessibility.

I'd like to keep these kinds of refactor for last, especially if we're only factoring three lines or less. I think there's a benefit to being able to see directly what the code does instead of jumping to another function.

waywardmonkeys · 2024-08-20T13:13:22Z

masonry/src/passes/event.rs

@@ -165,7 +165,7 @@ pub fn root_on_text_event(
    handled
 }

-pub fn root_on_access_event(
+pub(crate) fn root_on_access_event(


(Had commented directly on the commit, but then that doesn't show up here ...)

I use this in my own event loop integration. I need some way to pump an access event into the system ...

My mistake. I'm calling this on RenderRoot, not this one here.

DJMcNab · 2024-08-21T12:59:10Z

First thought when reviewing. The only thing which renders in mason is the textbox, which I don't think is the intention

PoignardAzur · 2024-08-21T13:15:33Z

The only thing which renders in mason is the textbox, which I don't think is the intention

This is also the case on main. Bisecting to find the culprit...

PoignardAzur · 2024-08-21T13:19:47Z

Apparently #527 introduced the regression.

DJMcNab

This is looking really good! I don't have any blocking concerns, but there are a few bits of clarification I'd like to see.

masonry/src/contexts.rs

DJMcNab · 2024-08-21T13:17:33Z

masonry/src/passes/accessibility.rs

+    state.item.request_accessibility = false;
+    state.item.needs_accessibility = false;


I suppose that since AccessCtx doesn't have these methods, it doesn't matter when these values are set, so long as it's after they are read?

I assume that by "these methods" you mean "the methods that gave me problems in the layout PR"?

To be honest, I haven't decided on (or documented) a principled solution for that class of problem. I'm mostly deferring it until all passes are implemented.

But yeah, your reasoning makes sense.

In this case, I meant request_accessibility. I hadn't checked if get_raw_mut were available in AccessCtx

masonry/src/passes/paint.rs

DJMcNab · 2024-08-21T13:34:49Z

masonry/src/passes/paint.rs

+            // This may have knock-on effects we'd need to document.
+            if state.item.is_stashed {
+                return;
+            }


Suggested change

}

}

// TODO: If child is completely outside of parent's clip path, don't paint it

// The naïve solution breaks if the widget overflows its layout box; ideally, widgets

// wouldn't do that, but they are currently allowed to do so.

There's also the fact that, if we adopt compositor layers, then you'll want to display some offscreen stuff so you don't get caught flatfooted when the user scrolls.

I've added a comment here.

DJMcNab · 2024-08-21T13:35:50Z

masonry/src/passes/paint.rs

+        widget_state: state.item,
+        widget_state_children: state.children.reborrow_mut(),
+        widget_children: widget.children.reborrow_mut(),
+        depth,


What is depth expected to be used for?

I think it was used in Druid for z-order stuff? I guess it's obsolete now.

Removed it.

DJMcNab · 2024-08-21T13:38:19Z

masonry/src/widget/widget_arena.rs

@@ -87,6 +93,20 @@ impl WidgetArena {
            .expect("get_state_mut: widget state not in widget tree")
    }

+    #[track_caller]
+    pub(crate) fn get_scene(&mut self, widget_id: WidgetId) -> &Scene {


These methods are never used...

I removed them.

DJMcNab · 2024-08-21T13:41:33Z

masonry/src/widget/widget_state.rs

@@ -65,7 +65,9 @@ pub struct WidgetState {
    // TODO - Document
    pub(crate) is_portal: bool,

-    pub(crate) request_compose: bool,
+    // TODO - Use general Shape


Suggested change

// TODO - Use general Shape

// The clip is a rectangle to maintain compatibility with system compositor APIs, which only know about rectangles.

// Note that we don't currently integrate with the system compositor.

// A future extension to use an arbitrary shape may be possible if there is a significant need for it.

I'd rather support arbitrary shapes. Or at the very least, rectangles with rounded corners. The reason I'm doing rectangles only is that Kurbo doesn't provide you with a type to store an arbitrary shape.

Right - I guess a solution for rounded rectangles and similar is to draw the parent surface "above" the child surfaces, and have a transparent cutout for the composited view. If compositors can implement that performantly, that would work well

My mistake, I think arbitrary paths are probably fine then, although it probably requires some careful use of blend modes to punch through a transparent region.

DJMcNab · 2024-08-21T13:43:27Z

masonry/src/widget/widget_arena.rs

    pub(crate) widget_states: TreeArena<WidgetState>,
+    pub(crate) scenes: HashMap<WidgetId, Scene>,


It's not obvious why this isn't a field on the WidgetState.

~~There are cases where you want to separately borrow both.~~

Wait, no. I think it's because Scene doesn't implement Debug and I didn't want to manually implement it in WidgetState.

DJMcNab · 2024-08-21T14:01:50Z

masonry/src/widget/widget_arena.rs

    pub(crate) widget_states: TreeArena<WidgetState>,
+    pub(crate) scenes: HashMap<WidgetId, Scene>,


Do these ever get cleaned up?

Good catch.

PoignardAzur · 2024-08-21T14:37:44Z

I think I've addressed all review comments.

This is a big one, so I'll wait until tomorrow to merge it, in case there's any concerns left.

Fix stashed child handling. Handle scrolling in compose pass. Fix set_child_translation method.

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

Recursing is done inside the paint and accessibility passes since ff7635e. I believe this is the correct continuation of #522, with the removal of these methods "left for later" as mentioned in #522 (comment). One note is that Flex now debug-paints its baseline under its children, rather than over them.

Fixes scrolling `Portal` on scrollbar drag by recomposing instead of relayouting. Regression probably caused in 59ee615 (#488) or ff7635e (#522).

PoignardAzur force-pushed the implement_pass_spec_paint_access branch from df31c3e to ca13a0d Compare August 17, 2024 13:09

PoignardAzur mentioned this pull request Aug 19, 2024

Implement update passes #525

Closed

PoignardAzur force-pushed the implement_pass_spec_paint_access branch 3 times, most recently from 8481254 to cbb10c0 Compare August 20, 2024 10:10

PoignardAzur marked this pull request as ready for review August 20, 2024 11:14

jaredoconnell reviewed Aug 20, 2024

View reviewed changes

waywardmonkeys reviewed Aug 20, 2024

View reviewed changes

DJMcNab approved these changes Aug 21, 2024

View reviewed changes

DJMcNab reviewed Aug 21, 2024

View reviewed changes

PoignardAzur force-pushed the implement_pass_spec_paint_access branch from e248536 to def14c0 Compare August 21, 2024 14:38

PoignardAzur added 13 commits August 21, 2024 16:51

Implement initial paint pass

8135172

TMP

6975765

Implement accessibility pass

2a433d1

Request accessibility and paint passes in compose pass

936584e

Factor out recurse_on_children function

9b057a6

Link to "Reserve scene" issue

805fec0

Add clip support to passes

94233fe

Remove unused import

fba1554

Remove unused import

7555bc4

Remove leftover dbg macro

54ba10c

Clarify needs/request flags

8d9266b

Integrate stashed widgets into paint and access passes

4127c98

Reset rebuild_access_tree flag

b33e09e

PoignardAzur and others added 8 commits August 21, 2024 16:51

Misc fixes

44f77ff

Add request_layout flag

4343e3f

Tweak clip path methods

2129655

Rework Portal widget

2f0bf41

Fix stashed child handling. Handle scrolling in compose pass. Fix set_child_translation method.

Add some doc comments

85daf06

Make some root methods private

d722aca

Apply suggestions from code review

1d4f30d

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

Misc fixes

61cfb07

PoignardAzur force-pushed the implement_pass_spec_paint_access branch from def14c0 to 61cfb07 Compare August 21, 2024 14:51

PoignardAzur added this pull request to the merge queue Aug 23, 2024

Merged via the queue into main with commit ff7635e Aug 23, 2024
17 checks passed

PoignardAzur deleted the implement_pass_spec_paint_access branch August 23, 2024 08:51

tomcur mentioned this pull request Aug 26, 2024

Remove manual recursion to child paint and accessibility #557

Merged

tomcur mentioned this pull request Aug 29, 2024

masonry: fix updating Portal on scrollbar drag #563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement paint and accessibility passes #522

Implement paint and accessibility passes #522

PoignardAzur commented Aug 17, 2024

PoignardAzur commented Aug 17, 2024 •

edited

Loading

PoignardAzur commented Aug 20, 2024

jaredoconnell left a comment

jaredoconnell Aug 20, 2024

PoignardAzur Aug 21, 2024

waywardmonkeys Aug 20, 2024

waywardmonkeys Aug 20, 2024

DJMcNab commented Aug 21, 2024

PoignardAzur commented Aug 21, 2024

PoignardAzur commented Aug 21, 2024 •

edited

Loading

DJMcNab left a comment

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

DJMcNab Aug 21, 2024

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

PoignardAzur Aug 21, 2024

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

PoignardAzur Aug 21, 2024

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

DJMcNab Aug 22, 2024

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024 •

edited

Loading

DJMcNab Aug 21, 2024

PoignardAzur Aug 21, 2024

PoignardAzur commented Aug 21, 2024

		state.item.request_accessibility = false;
		state.item.needs_accessibility = false;

-            }
+            }
+            // TODO: If child is completely outside of parent's clip path, don't paint it
+            // The naïve solution breaks if the widget overflows its layout box; ideally, widgets
+            // wouldn't do that, but they are currently allowed to do so.

-    // TODO - Use general Shape
+    // The clip is a rectangle to maintain compatibility with system compositor APIs, which only know about rectangles.
+    // Note that we don't currently integrate with the system compositor.
+    // A future extension to use an arbitrary shape may be possible if there is a significant need for it.

		pub(crate) widget_states: TreeArena<WidgetState>,
		pub(crate) scenes: HashMap<WidgetId, Scene>,

Implement paint and accessibility passes #522

Implement paint and accessibility passes #522

Conversation

PoignardAzur commented Aug 17, 2024

PoignardAzur commented Aug 17, 2024 • edited Loading

PoignardAzur commented Aug 20, 2024

jaredoconnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Aug 21, 2024

PoignardAzur commented Aug 21, 2024

PoignardAzur commented Aug 21, 2024 • edited Loading

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur commented Aug 21, 2024

PoignardAzur commented Aug 17, 2024 •

edited

Loading

PoignardAzur commented Aug 21, 2024 •

edited

Loading

PoignardAzur Aug 21, 2024 •

edited

Loading