fix(ngTouch): remove ngClick override and $touchProvider.ngClickOverr… #15761

Narretz · 2017-02-28T17:27:19Z

…ideEnabled()

BREAKING CHANGE:

The ngClick directive from the ngTouch module has been removed, and with the also the
corresponding $touchProvider.ngClickOverrideEnabled() method.

If you have included ngTouch in your application with a version of 1.5.0 or higher, and have not
changed the value of $touchProvider.ngClickOverrideEnabled()`, then there are no migration steps.

The ngClick override directive had been deprecated and by default disabled since v1.5.0,
because of buggy behavior in edge cases, and a general trend to avoid special touch based
overrides of click events. If you still need a touch override, consider using
Fastclick.

Note that the $touch service still offers the ngClickOverrideEnabled() function for
libraries that want to detect if an app uses the click override. Starting with 1.7.0, it
will always return false.

Other info:

This one is a bit tricky, as we have deprecated the ngClick directive, but at the same time introduced two new APIs: $touchProvider.ngClickOverrideEnabled and $touch.ngClickOverrideEnabled().
I have removed the former, because it's directly tied to the directive. I think that's fine, as someone who uses the ngClick override must migrate to a different solution anyway, and will see an error when he updates and the provider fn is missing. I've kept the actual provider for the unlikely case that we introduce new config for the ngTouch module.
I've also kept the $touch.ngClickOverrideEnabled() method, as this one is used by material to check if they should include their own touch stuff. I think if we removed this then they wouldn't be able to determine if the ngClick override is present (because ngTouch module but no $touch service could also mean a pre 1.5.0 version of ngTouch)

mgol · 2017-02-28T22:30:05Z

src/ngTouch/touch.js

-       *
-       * @returns {*} current value of `ngClickOverrideEnabled` set in the {@link ngTouch.$touchProvider $touchProvider},
-       * i.e. if {@link ngTouch.ngClick ngTouch's ngClick} directive is enabled.
+       *       *


A redundant line? Also, an extra *.

mgol · 2017-02-28T22:30:25Z

src/ngTouch/touch.js


  /**
  * @ngdoc service
  * @name $touch
  * @kind object
  *
  * @description
-  * Provides the {@link ngTouch.$touch#ngClickOverrideEnabled `ngClickOverrideEnabled`} method.
+  * Provides the legacy {@link ngTouch.$touch#ngClickOverrideEnabled `ngClickOverrideEnabled`} method.


Can we deprecate it at the same time?

Is there any reason to keep this, if we are removing ngClick?

@gkalpak It's all explained in the first post of the pull request. :-)

Oh, we are supposed to read that, right? Doh!

So, it looks like material never implemented a check with ngClickOverrideEnabled, it simply is not compatible with ngTouch: https://github.com/angular/material/blob/24370e7627389a873a95777f8bebc8bb43c842ce/src/core/core.js#L23

I think that means we can remove $touch.ngClickOverrideEnabled
I doubt that anyone else uses it (famous last words)

mgol · 2017-02-28T22:34:55Z

I think if we removed this then they wouldn't be able to determine if the ngClick override is present (because ngTouch module but no $touch service could also mean a pre 1.5.0 version of ngTouch)

This is unclear to me - if there was no $touch service, injecting it would result in an error. Is the service missing or just a method? If it's just a method then just checking for its existence could be treated as it returning false. Obviously we can still go through a deprecation cycle but I don't see why we shouldn't be able to remove it eventually.

This one is a bit tricky, as we have deprecated the ngClick directive, but at the same time introduced two new APIs: $touchProvider.ngClickOverrideEnabled and $touch.ngClickOverrideEnabled().
I have removed the former, because it's directly tied to the directive. I think that's fine, as someone who uses the ngClick override must migrate to a different solution anyway, and will see an error when he updates and the provider fn is missing.

Why does a person using the override have to migrate to a different solution? This doesn't break the app and in current mobile browsers the click delay is disabled in many situations anyway so it may very well be redundant. If we're keeping the method on $touch for now I think we may as well keep the setter on the provider and just document it's now a noop (and deprecate at the same time).

Narretz · 2017-03-01T11:08:19Z

This is unclear to me - if there was no $touch service, injecting it would result in an error. Is the service missing or just a method? If it's just a method then just checking for its existence could be treated as it returning false. Obviously we can still go through a deprecation cycle but I don't see why we shouldn't be able to remove it eventually.

Good point. It turns out material never used the $touch service. FWIW, you can use $injector.has() or try-catch to find out if a service is available or not.

Why does a person using the override have to migrate to a different solution? This doesn't break the app and in current mobile browsers the click delay is disabled in many situations anyway so it may very well be redundant. If we're keeping the method on $touch for now I think we may as well keep the setter on the provider and just document it's now a noop (and deprecate at the same time).

They don't have to, but I assume they want to. If they are on 1.6, they have to opt-in to the ngClick override, soif they did that, they want this behavior, which means they want it in the future, too.
But since material doesn't actually use $touch.ngClickOverrideEnabled we can remove the function.

petebacondarwin · 2017-03-02T08:32:30Z

I think we should remove the $touch.ngClickOverrideEnabled getter too. Now that we have landed the module.info() method then library providers can work out what version of the ngTouch module is loaded.

Narretz · 2017-03-02T16:21:12Z

I've removed the fn on the service, PTAL

mgol · 2017-03-02T19:40:13Z

How long do we want to keep the noop $touch service?

Narretz · 2017-03-03T10:02:42Z

I've kept it in case we want to put some stuff on it in the future. At the same time I think this is unlikely and what we should do instead is to deprecate ngTouch completely.

gkalpak · 2017-03-03T10:24:39Z

Why deprecate ngTouch completely? Aren't the ngSwipes still useful?

Narretz · 2017-03-03T10:33:41Z

My main point is that we have a few open PRs and bugs about ngSwipe that we will probably never merge / fix because we don't have the resource to test and make sure they work on all platforms. There are other touch gestures libraries that can provide this stuff, we should really focus on the core. There's also not touch stuff in Angular, it's either material or something 3rd party.

mgol · 2017-03-03T10:37:31Z

I guess it makes sense to not keep this zombie stuff here then.

Narretz · 2017-03-03T10:46:14Z

With Zombie stuff you mean the $touchProvider?

mgol · 2017-03-03T10:58:32Z

The whole ngTouch, if we're not really maintaining it.

mgol · 2017-03-03T11:00:26Z

In jQuery we have a separate org - https://github.com/jquery-archive/ - where we keep projects that we no longer maintain. Were we to remove ngTouch completely one day we could put it somewhere for others to pick up if they want.

petebacondarwin · 2017-03-03T11:29:08Z

That is where ngScenario should go too :-)

Narretz · 2017-03-03T11:35:15Z

I think I have a PR for removing ngScenario lying around.

And in that case, I will remove the $touchProvider completely.

petebacondarwin · 2017-03-03T13:37:30Z

We were trying to get someone in the community to take ownership of ngScenario in https://github.com/angular/ngScenario but no one stepped up. Perhaps if we remove it in 1.7/8 then it will trigger more interest?

mgol · 2017-03-03T16:55:22Z

@Narretz Regarding the BREAKING CHANGE commit message - in 99% of cases instead of using FastClick today it's better to set the proper viewport or a proper touch-manipulation value on html (I don't remember which one). I feel it's better to direct people to those solutions as they avoid all the problems related to the hacks solutions like FastClick have to apply.

petebacondarwin · 2017-03-08T12:20:10Z

I agree with @mgol that the commit message should point to documentation (not necessarily ours) on best practices for this sort of thing rather than just pointing to fastclick. Otherwise LGTM (for master only)

Closes angular#15761 Closes angular#15755 BREAKING CHANGE: The `ngClick` directive from the ngTouch module has been removed, and with the also the corresponding $touchProvider and $touch service. If you have included ngTouch in your application with version 1.5.0 or higher, and have not changed the value of $touchProvider.ngClickOverrideEnabled()`, or injected and used the $touch service, then there are no migration steps for your code. Otherwise you must remove references to the provider and service. The `ngClick` override directive had been deprecated and by default disabled since v1.5.0, because of buggy behavior in edge cases, and a general trend to avoid special touch based overrides of click events. In modern browsers, it should not be necessary to use a touch override library: - Chrome and Firefox for Android remove the 300ms delay when the well-known `<meta name="viewport" content="width=device-width">` is set. - Internet Explorer, Edge, and Chrome remove the delay when the `touch-action` css property is set to `none` or `manipulation`. - Since iOS 8, Safari removes the delay on so-called "slow taps". You can find out more in this Telerik article: http://developer.telerik.com/featured/300-ms-click-delay-ios-8/

…ideEnabled() Closes angular#15755 BREAKING CHANGE: The `ngClick` directive from the ngTouch module has been removed, and with the also the corresponding $touchProvider.ngClickOverrideEnabled() method. If you have included ngTouch in your application with a version of 1.5.0 or higher, and have not changed the value of $touchProvider.ngClickOverrideEnabled()`, then there are no migration steps. The `ngClick` override directive had been deprecated and by default disabled since v1.5.0, because of buggy behavior in edge cases, and a general trend to avoid special touch based overrides of click events. If you still need a touch override, consider using [Fastclick](https://github.com/ftlabs/fastclick). Note that the `$touch` service still offers the `ngClickOverrideEnabled()` function for libraries that want to detect if an app uses the click override. Starting with 1.7.0, it will always return `false`.

Closes angular#15761 Closes angular#15755 BREAKING CHANGE: The `ngClick` directive from the ngTouch module has been removed, and with the also the corresponding $touchProvider and $touch service. If you have included ngTouch in your application with version 1.5.0 or higher, and have not changed the value of $touchProvider.ngClickOverrideEnabled()`, or injected and used the $touch service, then there are no migration steps for your code. Otherwise you must remove references to the provider and service. The `ngClick` override directive had been deprecated and by default disabled since v1.5.0, because of buggy behavior in edge cases, and a general trend to avoid special touch based overrides of click events. In modern browsers, it should not be necessary to use a touch override library: - Chrome and Firefox for Android remove the 300ms delay when the well-known `<meta name="viewport" content="width=device-width">` is set. - Internet Explorer, Edge, and Chrome remove the delay when the `touch-action` css property is set to `none` or `manipulation`. - Since iOS 8, Safari removes the delay on so-called "slow taps". You can find out more in this Telerik article: http://developer.telerik.com/featured/300-ms-click-delay-ios-8/

Narretz · 2017-03-11T17:20:36Z

I've updated the commit message (see last commit)

mgol

@Narretz Re commit msg:

and with the also the

A typo.

Since iOS 8, Safari removes the delay on so-called "slow taps".

This is incomplete. I wouldn't even mention iOS 8 due to its low market share (https://developer.apple.com/support/app-store/) but only 9.3+ which have conditions very similar to Chrome/Firefox/Edge mobile:
https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html#//apple_ref/doc/uid/TP40014305-CH10-SW8

Closes angular#15761 Closes angular#15755 BREAKING CHANGE: The `ngClick` directive from the ngTouch module has been removed, and with it the corresponding `$touchProvider` and `$touch` service. If you have included ngTouch v1.5.0 or higher in your application, and have not changed the value of `$touchProvider.ngClickOverrideEnabled()`, or injected and used the `$touch` service, then there are no migration steps for your code. Otherwise you must remove references to the provider and service. The `ngClick` override directive had been deprecated and by default disabled since v1.5.0, because of buggy behavior in edge cases, and a general trend to avoid special touch based overrides of click events. In modern browsers, it should not be necessary to use a touch override library: - Chrome, Firefox, Edge, and Safari remove the 300ms delay when `<meta name="viewport" content="width=device-width">` is set. - Internet Explorer 10+, Edge, Safari, and Chrome remove the delay on elements that have the `touch-action` css property is set to `manipulation`. You can find out more in these articles: https://developers.google.com/web/updates/2013/12/300ms-tap-delay-gone-away https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html#//apple_ref/doc/uid/TP40014305-CH10-SW8 https://blogs.msdn.microsoft.com/ie/2015/02/24/pointer-events-w3c-recommendation-interoperable-touch-and-removing-the-dreaded-300ms-tap-delay/

Narretz · 2017-03-12T13:21:30Z

@mgol Updated again, PTAL

Closes angular#15761 Closes angular#15755 BREAKING CHANGE: The `ngClick` directive from the ngTouch module has been removed, and with it the corresponding `$touchProvider` and `$touch` service. If you have included ngTouch v1.5.0 or higher in your application, and have not changed the value of `$touchProvider.ngClickOverrideEnabled()`, or injected and used the `$touch` service, then there are no migration steps for your code. Otherwise you must remove references to the provider and service. The `ngClick` override directive had been deprecated and by default disabled since v1.5.0, because of buggy behavior in edge cases, and a general trend to avoid special touch based overrides of click events. In modern browsers, it should not be necessary to use a touch override library: - Chrome, Firefox, Edge, and Safari remove the 300ms delay when `<meta name="viewport" content="width=device-width">` is set. - Internet Explorer 10+, Edge, Safari, and Chrome remove the delay on elements that have the `touch-action` css property is set to `manipulation`. You can find out more in these articles: https://developers.google.com/web/updates/2013/12/300ms-tap-delay-gone-away https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html#//apple_ref/doc/uid/TP40014305-CH10-SW8 https://blogs.msdn.microsoft.com/ie/2015/02/24/pointer-events-w3c-recommendation-interoperable-touch-and-removing-the-dreaded-300ms-tap-delay/

Narretz added component: ngTouch needs: breaking change labels Feb 28, 2017

Narretz added this to the 1.7.0 milestone Feb 28, 2017

googlebot added the cla: yes label Feb 28, 2017

Narretz added the needs: review label Feb 28, 2017

mgol suggested changes Feb 28, 2017

View reviewed changes

Narretz force-pushed the fix-remove-ngtouch-nglick branch from 1a4add9 to 015c837 Compare March 3, 2017 10:10

mgol approved these changes Mar 3, 2017

View reviewed changes

Narretz force-pushed the fix-remove-ngtouch-nglick branch from 9cd0d1f to 050db20 Compare March 8, 2017 12:38

Narretz added 3 commits March 11, 2017 18:13

fixup! remove touch service too

04cceb3

add test for

cde2ddb

Narretz force-pushed the fix-remove-ngtouch-nglick branch from 050db20 to a0b569a Compare March 11, 2017 17:16

mgol suggested changes Mar 11, 2017

View reviewed changes

Narretz force-pushed the fix-remove-ngtouch-nglick branch from a0b569a to 48da68e Compare March 12, 2017 13:20

mgol approved these changes Mar 12, 2017

View reviewed changes

Narretz merged commit 11d9ad1 into angular:master Mar 14, 2017

Narretz added the resolution: won't fix label Feb 1, 2018

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngTouch): remove ngClick override and $touchProvider.ngClickOverr… #15761

fix(ngTouch): remove ngClick override and $touchProvider.ngClickOverr… #15761

Narretz commented Feb 28, 2017

mgol Feb 28, 2017

mgol Feb 28, 2017

gkalpak Mar 1, 2017

mgol Mar 1, 2017

gkalpak Mar 1, 2017

Narretz Mar 1, 2017

mgol commented Feb 28, 2017

Narretz commented Mar 1, 2017

petebacondarwin commented Mar 2, 2017

Narretz commented Mar 2, 2017

mgol commented Mar 2, 2017

Narretz commented Mar 3, 2017

gkalpak commented Mar 3, 2017

Narretz commented Mar 3, 2017 •

edited

Loading

mgol commented Mar 3, 2017

Narretz commented Mar 3, 2017

mgol commented Mar 3, 2017

mgol commented Mar 3, 2017

petebacondarwin commented Mar 3, 2017

Narretz commented Mar 3, 2017

petebacondarwin commented Mar 3, 2017

mgol commented Mar 3, 2017

petebacondarwin commented Mar 8, 2017

Narretz commented Mar 11, 2017

mgol left a comment •

edited

Loading

Narretz commented Mar 12, 2017

fix(ngTouch): remove ngClick override and $touchProvider.ngClickOverr… #15761

fix(ngTouch): remove ngClick override and $touchProvider.ngClickOverr… #15761

Conversation

Narretz commented Feb 28, 2017

mgol Feb 28, 2017

Choose a reason for hiding this comment

mgol Feb 28, 2017

Choose a reason for hiding this comment

gkalpak Mar 1, 2017

Choose a reason for hiding this comment

mgol Mar 1, 2017

Choose a reason for hiding this comment

gkalpak Mar 1, 2017

Choose a reason for hiding this comment

Narretz Mar 1, 2017

Choose a reason for hiding this comment

mgol commented Feb 28, 2017

Narretz commented Mar 1, 2017

petebacondarwin commented Mar 2, 2017

Narretz commented Mar 2, 2017

mgol commented Mar 2, 2017

Narretz commented Mar 3, 2017

gkalpak commented Mar 3, 2017

Narretz commented Mar 3, 2017 • edited Loading

mgol commented Mar 3, 2017

Narretz commented Mar 3, 2017

mgol commented Mar 3, 2017

mgol commented Mar 3, 2017

petebacondarwin commented Mar 3, 2017

Narretz commented Mar 3, 2017

petebacondarwin commented Mar 3, 2017

mgol commented Mar 3, 2017

petebacondarwin commented Mar 8, 2017

Narretz commented Mar 11, 2017

mgol left a comment • edited Loading

Choose a reason for hiding this comment

Narretz commented Mar 12, 2017

Narretz commented Mar 3, 2017 •

edited

Loading

mgol left a comment •

edited

Loading