Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

masonry: replace set_active and is_active with pointer capture #564

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

tomcur
Copy link
Contributor

@tomcur tomcur commented Aug 29, 2024

Also improve documentation of pointer capture.

Continuation of 59ee615 (#488).

Makes has_pointer_capture available on all context types except LayoutCtx, like is_active used to be.

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

Good work.

Comment on lines 602 to 604
/// Discussion: there is currently some confusion about whether a widget can be considered hot
/// when some other widget has captured the pointer (for example, when clicking one widget and
/// dragging to the next). The documentation should clearly state the resolution.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any confusion, but maybe I missed a Zulip thread.

The semantics are this: if a widget has pointer capture, either that widget is hot/hovered, or no widget is. This matches web behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I duplicated the discussion point from here

/// Discussion: there is currently some confusion about whether a
/// widget can be considered hot when some other widget is active (for
/// example, when clicking to one widget and dragging to the next).
/// The documentation should clearly state the resolution.
pub fn is_hot(&self) -> bool {
self.widget_state.is_hot
}

I probably should've referenced it instead of copying. If the question is resolved I can remove the comment from this PR, and create a new PR removing the old discussion point on is_hot and document that behavior.

Somewhat related to this discussion is this Zulip thread about pointer exclusivity: https://xi.zulipchat.com/#narrow/stream/354396-xilem/topic/Exclusive.20events.20when.20interacting

Copy link
Contributor

@PoignardAzur PoignardAzur Aug 30, 2024

Choose a reason for hiding this comment

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

If the question is resolved I can remove the comment from this PR, and create a new PR removing the old discussion point on is_hot and document that behavior.

Sure.

Somewhat related to this discussion is this Zulip thread about pointer exclusivity: https://xi.zulipchat.com/#narrow/stream/354396-xilem/topic/Exclusive.20events.20when.20interacting

Oh, I forgot about that thread. I wonder if we have an active issue somewhere referencing it. We might want to add a comment with a link to either a github issue or the zulip thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the discussion point. I can update the docs with the behavior of capture/hot on top of this PR. Easiest after it's merged.

masonry/src/widget/scroll_bar.rs Outdated Show resolved Hide resolved
@PoignardAzur
Copy link
Contributor

LGTM.

I especially appreciate the added tests.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

A few small suggestions, but otherwise looks good. The same thing as in #563 applies

masonry/src/contexts.rs Outdated Show resolved Hide resolved
///
/// # Releasing the pointer
///
/// Any widget can [`release`] the pointer during any event. The pointer is automatically
Copy link
Member

Choose a reason for hiding this comment

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

In what context would it be valid for a pointer without the capture to release this capture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, any widget can release the capture during any event. That's either a target of an event or one of its ancestors. The target can be the widget with the pointer capture for a pointer event, or the widget with keyboard focus for a text event, or can be the target widget of an accessibility event. Widgets are not currently notified when they lose the capture in this way.

There's no hard requirement at the moment that says, e.g., the pointer capture widget (if any) should be equal to the widget with keyboard focus (if any).

masonry/src/contexts.rs Outdated Show resolved Hide resolved
@@ -577,9 +583,32 @@ impl_context_method!(
pub struct TimerToken;

impl EventCtx<'_> {
// TODO - Document
// TODO - clearly document all semantics of pointer capture when they've been decided on
Copy link
Member

Choose a reason for hiding this comment

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

Another question here: Is it valid for a disabled widget to have the pointer capture?

Copy link
Contributor Author

@tomcur tomcur Sep 3, 2024

Choose a reason for hiding this comment

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

It is at the moment. If we decide against that, the same should be invalid for keyboard focus. Capturing the pointer or requesting focus should then not be allowed when disabled, and in EventCtx::set_disabled we should traverse the tree up from the widgets with pointer capture and keyboard focus to check whether their subtree is now disabled.

Improve documentation of pointer capture.

Continuation of 59ee615
(linebender#488).

Makes `has_pointer_capture` available on all context types except
`LayoutCtx`, like `is_active` used to be.
@PoignardAzur
Copy link
Contributor

@tomcur Anything you'd like to change before merging?

@tomcur
Copy link
Contributor Author

tomcur commented Sep 10, 2024

Happy for this to be merged. I believe I should do the merge because of the author-merges policy, now Daniel added me to the org?

@PoignardAzur
Copy link
Contributor

Yup.

@tomcur tomcur added this pull request to the merge queue Sep 10, 2024
Merged via the queue into linebender:main with commit 9a3c8e3 Sep 10, 2024
17 checks passed
@tomcur tomcur deleted the remove-active branch September 10, 2024 12:04
@tomcur tomcur restored the remove-active branch September 10, 2024 12:05
@tomcur tomcur deleted the remove-active branch September 10, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants