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

Complete overhaul of the interesttarget explainer #1141

Merged
merged 6 commits into from
Jan 24, 2025

Conversation

mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Jan 23, 2025

This should bring it in line with all/most of the recent discussions. Hopefully I didn't miss something important, or butcher a concept that was previously better explained.

This should bring it in line with all/most of the recent discussions. Hopefully I didn't miss something important, or butcher a concept that was previously better explained.
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jan 23, 2025

@keithamus @lukewarlow comments appreciated. Having said that, it'd be great if we could get this landed ASAP and then iterate on it via issues or just direct PRs or whatnot. I'd like to start socializing this more, and I need an up-to-date explainer to point folks to. That's not to say you shouldn't give this a thorough review now - just perhaps focus on critical mistakes and omissions and things, and not smaller nits.

Thanks in advance!!

@lukewarlow
Copy link
Collaborator

Taking the above comment into account I'm going to leave comments on what I notice but will only request changes if I find something substantial, feel free to resolve or make issues out of the rest as you see fit.

Copy link
Collaborator

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

In general this looks great to land, but I found 1 typo 😄

site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved

On the web, however, most browsers *already overload* the long-press gesture to provide additional functionality. For example, long-pressing on plain text nodes often creates a text selection around that text, and sometimes provides an additional context menu containing actions like "copy" or "look up". Long-pressing a link `<a>` element in most browsers provides a more involved context menu with additional operations such as "open in a new tab", "add to reading list", or "share". Users often appreciate these additional capabilities, and do not want to lose access to them. Therefore, this proposal provides a way to **keep both** capabilities: the `interesttarget` behavior **and** the existing context menus and behaviors.

To show interest in an element via touchscreen, the user simply long-presses that element. This does not activate (e.g. click) the element, it merely triggers interest in the element. This long press immediately fires the `"interest"` event, but *also* shows any context menus that would have shown if the `interesttarget` attribute were not present. So both things happen together. In the case of a popover target element, the idea is that the hovercard popover shows up **in addition** to the context menu, so that the user can tap on either of those things. The user can trigger `"loseinterest"` by tapping either outside the target popover, or on one of the provided context menu items (if any).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This discussion on contextmenu does make me think would people perhaps want the mouse trigger to be right click for somethings? Like the existing UA contextmenu is?

Now obviously part of this is that we don't want this to impact on the UA afforded patterns, so perhaps it's good that this is different. Again perhaps something to consider for future expansion. e.g. a Why hover not right click? Section below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'd really prefer not to open that can of worms. The entire discussion around touch screens is based on the fact that the user only has one way to indicate interest: long press. So we have to do extra work to make that one action do both things. On mouse systems, there really are two ways, hovering and right-clicking. So I don't think there's a need to try to combine and overlap those things.

};
```

Both `interesttarget` and `commandfor` can exist on the same element at the
same time, and both should be respected.
Both `interesttarget` and `commandfor` can exist on the same element at the same time, and both should be respected/functional, since they are activated using strictly separate actions by the user.
Copy link
Collaborator

@lukewarlow lukewarlow Jan 23, 2025

Choose a reason for hiding this comment

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

I have an accessibility question related to this, so while I remember. To be clear this is something for an issue to discuss in future.

How does aria-expanded work here? Say you're interested in a tooltip popover, but haven't opened the main dialog that this launches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I added a mention, since it seems like aria-expanded should get set also. But more discussion is needed here. @scottaohara and @aleventhal, FYI and call for help! If this ends up landing before you have a chance to look at the PR, no problem, I'm happy to land followups.

@lukewarlow
Copy link
Collaborator

Feel free to disregard most of what I've commented just wanted to leave thoughts down but there's a few things might be interesting.

Copy link
Collaborator Author

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

Thanks!!

site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
};
```

Both `interesttarget` and `commandfor` can exist on the same element at the
same time, and both should be respected.
Both `interesttarget` and `commandfor` can exist on the same element at the same time, and both should be respected/functional, since they are activated using strictly separate actions by the user.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I added a mention, since it seems like aria-expanded should get set also. But more discussion is needed here. @scottaohara and @aleventhal, FYI and call for help! If this ends up landing before you have a chance to look at the PR, no problem, I'm happy to land followups.

site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/interest-invokers.explainer.mdx Outdated Show resolved Hide resolved
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jan 24, 2025

Thanks for the reviews! Please open issues for anything leftover that is still broken, and I'll be happy to fix.

@mfreed7 mfreed7 merged commit 3bfc793 into openui:main Jan 24, 2025
4 of 5 checks passed
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