search

w3c / touch-events

https://w3c.github.io/touch-events/
26 stars 24 forks source link

Added legacy method initTouchEvent #47

Closed chong-z closed 8 years ago

chong-z commented 8 years ago

Added standardized initTouchEvent interface for legacy support, see #29

Preview it here http://htmlpreview.github.io/?https://github.com/choniong/touch-events/blob/initTouchEvent-interface/index.html#h-legacy-event-initializers

RByers commented 8 years ago

Thanks. I like that you've added a 'legacy' section like UI Events has. But if you're going to do this, then createTouch and createTouchList belong there too. Maybe create the section with just those two methods first, and then we can add this third method (#48)?

RByers commented 8 years ago

Perhaps we should omit scale and rotation, since there's no meaningful use for them and not having them in the spec is completely interoperable with code written for Safari which may provide them.

RByers commented 8 years ago

For screenX etc. how about we just name these as "unused" to make it clear they have no defined semantics?

RByers commented 8 years ago

Also you need to remove the note about initTouchEvent in section 6.1.
Also please add "add initTouchEvent" to section "C" (changes list last publication).

chong-z commented 8 years ago

Sure I will add createTouch and createTouchList in another branch.

For scale and rotation, Apple's doc says they are optional, but actually all arguments are required in Safari. Do we want to match docs or shipped code?

For those unused arguments I've renamed them to unused and added note about it, but for the change list I need to update commit link after landed.

RByers commented 8 years ago

For scale and rotation, Apple's doc says they are optional, but actually all arguments are required in Safari. Do we want to match docs or shipped code?

Good point. @dtapuska and I chatted about this for a bit. Given that our goal is that there should be some pattern people can use that works in all major browsers, and Safari requires these parameters, I think you should keep them (as "unused"). I guess there's no real benefit to omitting them from the spec, only likely to cause confusion. Hopefully once constructors are widely supported we can rip out all this crap.

but for the change list I need to update commit link after landed.

Actually once you have a final commit ready (we'll want to squash the intermediate commits together with 'git rebase -i'), you can insert a link with that commit hash. Merging the pull request shouldn't change the commit hash, so that should just work (I believe that's what I've done in some previous commits).

chong-z commented 8 years ago

Squashed the commits and updated change list link, hope I'm doing it correctly.

RByers commented 8 years ago

It still shows up as two commits in github. Perhaps you need to do a "git push --force"? Don't be alarmed by the warning - as long as no-one else has cloned your branch it's not going to confuse anyone to rewrite the history in this way...

chong-z commented 8 years ago

Yes I have two commits, one for the spec change and the other references the first one in change list.

If I squash these two won't the commit hash change again...?

RByers commented 8 years ago

Oh, hah hah - of course it will, sorry :-). This looks good to me.

Do you want me to just land this now and you can worry about moving createTouch and createTouchList in a follow-up CL (rather than doing that first as I originally proposed)?

chong-z commented 8 years ago

Ya it would be nice to land this now so I don't need to worry about conflicts and I can send the intent-to-ship first

RByers commented 8 years ago

Sorry, noticed a couple other little issues:

To make additional updates easier, feel free to leave inserting the commit link until after we merge this PR (sorry I was wrong).

chong-z commented 8 years ago

Sorry for the issues, seems I had some misunderstanding about when to use 'standardized' and 'normative'.

I've removed "is not standardized and", renamed 'unused*', removed "this section is non-normative" and reverted change list update to leave it later. Hope it works this time.

RByers commented 8 years ago

This looks great to me, thanks. Want to squash these two commits together again and I'll merge it?

chong-z commented 8 years ago

Sure, it should be only one commit now, should I create another pull request for the change list?

RByers commented 8 years ago

Ok, merged. Yes please open another PR for the changelog entry.