Add getModifierState definition to TouchEvent by dtapuska · Pull Request #91 · w3c/touch-events · GitHub
Skip to content
This repository has been archived by the owner on Jul 9, 2024. It is now read-only.

Add getModifierState definition to TouchEvent #91

Merged
merged 1 commit into from
May 10, 2022

Conversation

dtapuska
Copy link
Contributor

@dtapuska dtapuska commented Oct 23, 2017

This matches the text that is used in the UI Events spec.

@patrickhlauke
Copy link
Member

Looks conceptually good to me (though, as an aside - and not having properly looked at the UI Events spec - what would the keyArg actually look like in use? i.e. if i had a touch, what would I write to query if shift is also pressed? something like e.targetTouches[0].getModifierState("Shift") ? and would upper/lowercase etc matter?)

further, per #90, do we need to make sure to add web platform tests for it?

@dtapuska
Copy link
Contributor Author

This is on the TouchEvent and not TouchPoint so you might say something like:

e.getModifierState("CapsLock") or e.getModifierState("Shift")

Specifically this adds the ability to query the non (alt/ctrl/meta/shift) modifiers that getModifierState allows... which is the useful part.

@patrickhlauke
Copy link
Member

This is on the TouchEvent and not TouchPoint so you might say something like:

Oops, right you are.

e.getModifierState("CapsLock") or e.getModifierState("Shift")

Ah, perfect.

So yeh, happy to merge when/if we include a web platform test (if that's feasible/easy to test)

@patrickhlauke
Copy link
Member

@dtapuska is this mergeable? are there any web platform tests for it now?

@dtapuska
Copy link
Contributor Author

It is mergeable but we don't have an implementation or web platform tests since there wasn't any traction on it. @NavidZ can you get someone to complete this?

@NavidZ
Copy link
Member

NavidZ commented Oct 17, 2018

@dtapuska you mean the Chrome implementation or the test or both? Has there been any recent tractions and requests to get this in or is it just to align touch events with ui events?

@dtapuska
Copy link
Contributor Author

The Chrome implementation has neither. But it does have accessors for alt,ctrl,shift,meta. So really it should have the API on the IDL for modifierState to query them all. This is just a consistency thing. The data should be there already for the Chrome implementation, the IDL just needs to change and web platform tests need to be written.

@patrickhlauke
Copy link
Member

@dtapuska @NavidZ what's the state of this? should it be merged?

@dtapuska
Copy link
Contributor Author

I think it should for consistency. However I don't think any browser implements it yet. @flackr @mustaqahmed ?

@patrickhlauke
Copy link
Member

patrickhlauke commented May 8, 2022

@flackr @mustaqahmed any idea? (sorry for harking on here, but keen to clear the deck of any long-standing PRs etc)

@mustaqahmed
Copy link
Member

I agree with the consistency comment, but also share the cross-browser support concern there! So LGTM provided we have either a supporting comment from another browser or a WPT.

@flackr
Copy link

flackr commented May 9, 2022

I think merging this and adding a wpt is reasonable. As @dtapuska pointed out we already have all of the information so adding support should be simple and non-controversial.

@patrickhlauke
Copy link
Member

I'll go ahead and merge, and make a new issue about getting a wpt. Now the million dollar question...who's got the knowledge/time to make a wpt? ...

@patrickhlauke patrickhlauke changed the title Add getModifierState definition to TouchEvent. This matches the text Add getModifierState definition to TouchEvent May 10, 2022
@patrickhlauke patrickhlauke merged commit 3ec386a into w3c:gh-pages May 10, 2022
patrickhlauke pushed a commit that referenced this pull request May 10, 2022
 This matches the text that is used in the UI Events spec.
patrickhlauke added a commit that referenced this pull request May 10, 2022
This matches the text that is used in the UI Events spec.

Co-authored-by: Dave Tapuska <dtapuska@chromium.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants