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

Specify wheel event groups #344

Merged
merged 1 commit into from Dec 4, 2023
Merged

Conversation

dlrobertson
Copy link
Member

@dlrobertson dlrobertson commented Mar 16, 2023

Specify the behavior of wheel event groups.

Closes #338

The following tasks have been completed:

Implementation commitment:

Copy link
Member Author

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

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

Just a first pass at describing wheel event groups. Critiques would be greatly appreciated!

occur over the child element. In the case that {{Event/preventDefault()}}
is called in a wheel event handler for the child element, targetting
the child element could result in unexpected behavior for the user.
</p>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if the example text is helpful as is, but I thought some sort of example might clarify things.

Copy link
Member

Choose a reason for hiding this comment

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

I think having examples can be really useful.

I'm not sure what the takeaway is for this example, though. It ends with "could result in unexpected behavior", which makes it sound like a cautionary tale. Is this trying to explain why it is spec'ed this way, or is there something that the user needs to do to avoid the unexpected behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my goal was "to explain why it is spec'ed this way". Is something like that typical in a spec or just contained to the spec issue and PR? This is also something that is easier to see with an actual example with a scrollable frame with a subframe that preventDefault's wheel events

Copy link
Member

Choose a reason for hiding this comment

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

IMO the last sentence is not necessary and might confuse more than clarify.

@@ -2180,11 +2180,12 @@ myDiv.addEventListener("auxclick", function(e) {
+| Sync / Async | Async |
+| Bubbles | Yes |
+| Trusted Targets | <code>Element</code> |
+| Cancelable | <a href="#cancelability-of-wheel-events">Varies</a> |
+| Cancelable | <a href="#cancelability-of-wheel-events">Varies</a> |
+| Composed | Yes |
Copy link
Member

Choose a reason for hiding this comment

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

I assume that this change was not intentional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, removed the change.

occur over the child element. In the case that {{Event/preventDefault()}}
is called in a wheel event handler for the child element, targetting
the child element could result in unexpected behavior for the user.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

I think having examples can be really useful.

I'm not sure what the takeaway is for this example, though. It ends with "could result in unexpected behavior", which makes it sound like a cautionary tale. Is this trying to explain why it is spec'ed this way, or is there something that the user needs to do to avoid the unexpected behavior?

@@ -381,6 +381,12 @@ the definitions for more information.
the [[#conf-interactive-ua]] and [[#conf-author-tools]] for details on the
requirements for a <em>conforming</em> user agent.

: <dfn>wheel event group</dfn>
Copy link
Member

Choose a reason for hiding this comment

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

We're moving away from having entries in glossary since they are not normative and mostly duplicate definitions that are properly defined elsewhere.

I think this should just be included up with the rest of the text.

Copy link
Member Author

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback!

occur over the child element. In the case that {{Event/preventDefault()}}
is called in a wheel event handler for the child element, targetting
the child element could result in unexpected behavior for the user.
</p>
Copy link
Member Author

Choose a reason for hiding this comment

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

I think my goal was "to explain why it is spec'ed this way". Is something like that typical in a spec or just contained to the spec issue and PR? This is also something that is easier to see with an actual example with a scrollable frame with a subframe that preventDefault's wheel events

@@ -2023,6 +2023,21 @@ myDiv.addEventListener("auxclick", function(e) {
deltaY value respectively).
</p>

A <a>user agent</a> SHOULD create a <a>wheel event group</a> when the first wheel event
Copy link
Member

Choose a reason for hiding this comment

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

I would make this "MUST" so that it's well-defined what the target is for wheel events. The UA-defined timeout allows UAs to not implement the grouping already.

@dlrobertson dlrobertson force-pushed the wheel-event-groups branch 2 times, most recently from 7531f06 to b00e1f7 Compare May 31, 2023 16:47
Copy link
Member Author

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

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

Removed the last sentence of the example and changed setting the target to ensure it is well defined.

@@ -2180,11 +2180,12 @@ myDiv.addEventListener("auxclick", function(e) {
+| Sync / Async | Async |
+| Bubbles | Yes |
+| Trusted Targets | <code>Element</code> |
+| Cancelable | <a href="#cancelability-of-wheel-events">Varies</a> |
+| Cancelable | <a href="#cancelability-of-wheel-events">Varies</a> |
+| Composed | Yes |
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, removed the change.

@dlrobertson
Copy link
Member Author

See web-platform-tests/wpt#37445 (comment), in particular:

I think I'd still prefer to call this thing something else than "event group". It is more like a wheel transaction or some such.

I'll update the text here to use "wheel transaction" instead of "wheel event group"

CC @smaug----

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

LGTM but needs a rebase.

Specify the behavior of wheel event groups.
@dlrobertson
Copy link
Member Author

@zcorpan @garykac rebased

@garykac garykac merged commit ee7998e into w3c:main Dec 4, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Dec 4, 2023
SHA: ee7998e
Reason: push, by garykac

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@dlrobertson
Copy link
Member Author

\o/ Thanks @zcorpan and @garykac!

@dlrobertson dlrobertson deleted the wheel-event-groups branch December 4, 2023 21:25
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.

Wheel event groups are not defined
3 participants