Implement update_scrolls pass #550

PoignardAzur · 2024-08-25T10:41:18Z

This is part of the Pass Specification RFC: linebender/rfcs#7

Note: This PRs comes with a lot of new TODO items. Addressing most of these items is difficult without major refactors, because Portal code deals with accessing values across multiple widgets, which is still hard to do elegantly.

PoignardAzur · 2024-08-25T10:43:03Z

And that's the last pass implemented!

Once we've merged all of them, the next step will be to replace RenderRoot::post_event_processing with something like RenderRoot::run_rewrite_passes, following the logic outlined in the pass spec RFC.

DJMcNab

Is there any way in the current examples to test this?

Actual code changes look reasonable, presuming that the panning maths is correct

DJMcNab · 2024-08-26T12:44:02Z

masonry/src/passes/update.rs

+        run_targeted_update_pass(root, Some(target), |widget, ctx| {
+            let event = LifeCycle::RequestPanToChild(rect);
+            widget.lifecycle(ctx, &event);
+


How will smooth scroll would be handled?

The best logic I can see if to "get where the widget will be", and all the parents scroll to that.

Depends what you mean by smooth scroll. Do you mean on a touchscreen?

I think the most straightforward solution is to have CSS-style animatable properties, and have scroll progress be one of them.

I don't mean on a touchscreen, I mean scrolling to a position over a certain amount of time.

Right, yeah. This logic would have to change at that point, but it's also deliberately unaddressed at the moment.

DJMcNab · 2024-08-26T12:45:09Z

masonry/src/passes/update.rs

+    let _span = info_span!("update_scroll").entered();
+
+    let scroll_request_targets = std::mem::take(&mut root.state.scroll_request_targets);
+    for (target, rect) in scroll_request_targets {


Does the order matter here? Is it meaningful to document that?

The order doesn't matter. scroll_request_target was initially an option, and I turned it to a Vec just to cover hypothetical cases where someone would want to pan several scroll areas at once.

I guess several targets could interfere if they're in the same scroll region. I think this will be rare enough that we won't need to document it.

DJMcNab · 2024-08-26T12:48:57Z

masonry/src/widget/portal.rs

+    // Note - Rect is in child coordinates
+    // TODO - Merge with pan_viewport_to
+    // Right now these functions are just different enough to be a pain to merge.
+    pub fn pan_viewport_to_raw(


This is pub, so needs a doc comment.

It's not supposed to be pub. Fixed.

DJMcNab · 2024-08-26T12:49:36Z

masonry/src/widget/portal.rs

+            viewport.min_y()..viewport.max_y(),
+            target.min_y()..target.max_y(),
+        )
+        .start;


So we prefer the top-left of the rectangle to go into view. That seems reasonable, but should probably be documented.

I added in a comment on the pass function. I'll do a more thorough documentation pass later.

PoignardAzur · 2024-08-26T12:59:44Z

Is there any way in the current examples to test this?

The to_do_list example.

The scroll functionality in the suggested example is completely broken by this change

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

The to_do_list example.

Thanks, checked it out and I can see that it is now impossible to scroll in that example.

PoignardAzur · 2024-08-26T17:26:01Z

Thanks, checked it out and I can see that it is now impossible to scroll in that example.

That doesn't sound right, it worked when I tested it. Maybe I forgot to push some changes? Will look into it.

DJMcNab · 2024-08-27T08:03:27Z

Ah, this is #424 . Indeed, applying

diff --git a/masonry/src/widget/textbox.rs b/masonry/src/widget/textbox.rs
index 8a85f2d8e..7ab850e26 100644
--- a/masonry/src/widget/textbox.rs
+++ b/masonry/src/widget/textbox.rs
@@ -13,6 +13,7 @@ use vello::{
     peniko::{BlendMode, Color},
     Scene,
 };
+use winit::event::Ime;
 
 use crate::widget::{LineBreaking, WidgetMut};
 use crate::{
@@ -208,7 +209,9 @@ impl Widget for Textbox {
         // If focused on a link and enter pressed, follow it?
         if result.is_handled() {
             // TODO - Use request_scroll_to with cursor rect
-            ctx.request_scroll_to_this();
+            if !matches!(event, TextEvent::Ime(Ime::Preedit(..))) {
+                ctx.request_scroll_to_this();
+            }
             ctx.set_handled();
             // TODO: only some handlers need this repaint
             ctx.request_layout();

makes this work

DJMcNab

With the workaround mentioned above, this now works well.

Some remaining concerns are documenting the preferred part of the rectangle which is shown, and fixing the scrollbars properly.

DJMcNab · 2024-08-27T08:07:14Z

masonry/src/widget/portal.rs

+                let portal_size = ctx.size();
+                let content_size = ctx.get_raw_ref(&mut self.child).ctx().layout_rect().size();
+
+                // FIXME - Update scrollbars


We need to do this in this PR, or explain why it's hard. The behaviour without that is quite bad.

DJMcNab · 2024-08-27T08:09:11Z

masonry/src/widget/textbox.rs

@@ -207,6 +207,8 @@ impl Widget for Textbox {
        let result = self.editor.text_event(ctx, event);
        // If focused on a link and enter pressed, follow it?
        if result.is_handled() {
+            // TODO - Use request_scroll_to with cursor rect


I imagine this will happen in #515 ?

Note: This PRs comes with a lot of new TODO items. Addressing most of these items is difficult without major refactors. This pass is still mostly functional.

DJMcNab

One small remaining proposed tweak.

DJMcNab · 2024-09-02T14:42:16Z

masonry/src/widget/textbox.rs

        if result.is_handled() {
+            // Some platforms will send a lot of spurious Preedit events.
+            // We only want to request a scroll on user input.
+            if !matches!(event, TextEvent::Ime(Ime::Preedit(..))) {


Ooh, apparently we can do

Suggested change

if !matches!(event, TextEvent::Ime(Ime::Preedit(..))) {

if !matches!(event, TextEvent::Ime(Ime::Preedit(preedit, ..)) if preedit.is_empty()) {

which in my testing has better behaviour (but is not the easiest to reason about in its current form)

DJMcNab previously approved these changes Aug 26, 2024

View reviewed changes

DJMcNab reviewed Aug 27, 2024

View reviewed changes

PoignardAzur added 2 commits August 30, 2024 12:22

Implement update_scrolls pass

443ef28

Note: This PRs comes with a lot of new TODO items. Addressing most of these items is difficult without major refactors. This pass is still mostly functional.

Implement changes from code reviews

7f359c8

PoignardAzur force-pushed the implement_pass_spec_update_scrolls branch from 54b607b to 7f359c8 Compare August 30, 2024 10:29

Fix scrollbar behavior on scroll request

094dbe0

DJMcNab approved these changes Sep 2, 2024

View reviewed changes

Tweak Textbox Ime event

2b4c8df

PoignardAzur enabled auto-merge September 9, 2024 11:24

PoignardAzur added this pull request to the merge queue Sep 9, 2024

Merged via the queue into main with commit 2ae7326 Sep 9, 2024
17 checks passed

PoignardAzur deleted the implement_pass_spec_update_scrolls branch September 9, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement update_scrolls pass #550

Implement update_scrolls pass #550

PoignardAzur commented Aug 25, 2024 •

edited

Loading

PoignardAzur commented Aug 25, 2024

DJMcNab left a comment

DJMcNab Aug 26, 2024

PoignardAzur Aug 26, 2024 •

edited

Loading

DJMcNab Aug 26, 2024

DJMcNab Aug 26, 2024

PoignardAzur Aug 26, 2024

DJMcNab Aug 26, 2024

PoignardAzur Aug 30, 2024

DJMcNab Aug 26, 2024

PoignardAzur Aug 30, 2024

PoignardAzur commented Aug 26, 2024

DJMcNab commented Aug 26, 2024

PoignardAzur commented Aug 26, 2024

DJMcNab commented Aug 27, 2024 •

edited

Loading

DJMcNab left a comment

DJMcNab Aug 27, 2024

PoignardAzur Aug 30, 2024

DJMcNab Aug 27, 2024

PoignardAzur Aug 30, 2024

DJMcNab left a comment

DJMcNab Sep 2, 2024

	if !matches!(event, TextEvent::Ime(Ime::Preedit(..))) {
	if !matches!(event, TextEvent::Ime(Ime::Preedit(preedit, ..)) if preedit.is_empty()) {

Implement update_scrolls pass #550

Implement update_scrolls pass #550

Conversation

PoignardAzur commented Aug 25, 2024 • edited Loading

PoignardAzur commented Aug 25, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur Aug 26, 2024 • edited Loading

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 commented Aug 26, 2024

DJMcNab commented Aug 26, 2024

PoignardAzur commented Aug 26, 2024

DJMcNab commented Aug 27, 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

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur commented Aug 25, 2024 •

edited

Loading

PoignardAzur Aug 26, 2024 •

edited

Loading

DJMcNab commented Aug 27, 2024 •

edited

Loading