-
Notifications
You must be signed in to change notification settings - Fork 199
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
Conversation
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.
@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!! |
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. |
There was a problem hiding this 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 😄
|
||
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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
}; | ||
``` | ||
|
||
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. |
There was a problem hiding this comment.
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.
Thanks for the reviews! Please open issues for anything leftover that is still broken, and I'll be happy to fix. |
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.