-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(ngTouch): deprecate ngClick override directive, disable it by def… #13857
Conversation
b4d97c6
to
61fe343
Compare
Great stuff @Narretz !! I think the commit title should be I am only slightly concerned about introducing a new Other than that I like this. |
Services starting with |
We could also call the service $click or something like that, because we already have $swipe. But I don't like that. |
No let's stick with |
61fe343
to
4ed2d01
Compare
I've added the option to read the value of ngClickOverrideEnabled from the $touch service. |
4ed2d01
to
5248ec3
Compare
Should I change |
I am happy to go with the more verbose. |
this.ngClickOverrideEnabled = function(enabled) { | ||
if (angular.isDefined(enabled)) { | ||
|
||
if (enabled && !ngClickOverrideEnabled) { |
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.
Maybe a little farfetched, but enabling -> disabling -> enabling will add two instances of the directive (and further disabling will only remove one instance).
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.
Nevermind, it does the right thing already. My mistake.
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 catch, updated.
5248ec3
to
2f6cee4
Compare
} else { | ||
// drop the ngTouch ngClick directive if the override has been re-disabled (because | ||
// we cannot de-register added directives) | ||
$delegate.splice(1, 1); |
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.
My only concern is what happens if the user has also created their ngClick
directive.
It would be better if we looped through $delegate
and removed the instances returned by ngTouchClickDirectiveFactory
(assuming we can reliably do that).
Else, we should have a BC notice that you if you are defining an ngClick
directive, your module should be loaded after ngTouch
.
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 agree that it is dangerous to try to remove the ngTouch
's ngClick
.
Instead, I propose that we change the API so that you can only enable the override and not disable it once it has been overridden.
E.g. to enable the directive: $touchProvider.overrideNgClick()
and then to check whether it has been overridden: $touch.isNgClickOverridden()
.
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.
We can set a property on the factory to identify the correct ngClick later. There's actually a mechanism in the loader that sets a $$moduleName property on the factory, so that the compileMultiDir error can be thrown with a good context. I'll set this manually (because the logic is a bit broken and doesn't work for the $compileProvider anyway), and then check for it in the delegate.
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.
@petebacondarwin That's also an option, but it really digresses from the way most provider's handle options in angular1. My previous comment shows how we can achieve safe removing.
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've updated the PR with the change for safely removing the ngTouch click dir.
One minor concern (see #13857 (comment)). |
2f6cee4
to
d2fe3a0
Compare
LGTM |
This commit deprecates the ngClick directive from the ngTouch module. Additionally, it disables it by default. It can be enabled in the new $touchProvider with $touchProvider.ngClickOverrideEnabled() method. The directive was conceived to remove the 300ms delay for click events on mobile browsers, by sending a synthetic click event on touchstart. It also tried to make sure that the original click event that the browser sends after 300ms was "busted", so that no redundant "ghost-clicks" appear. There are various reasons why the directive is being deprecated. - "This is an ugly, terrible hack!" (says so in the source) - It is plagued by various bugs that are hard to fix / test for all platforms (see below) - Simply including ngTouch activates the ngClick override, which means even if you simply want to use ngSwipe, you may break parts of your app - There exist alternatives for removing the 300ms delay, that can be used very well with Angular: [FastClick](https://github.com/ftlabs/fastclick), [Tappy!](https://github.com/filamentgroup/tappy/) (There's also hammer.js for touch events / gestures) - The 300ms delay itself is on the way out - Chrome and Firefox for Android remove the 300ms delay when the usual `<meta name="viewport" content="width=device-width">` is set. In IE, the `touch-action` css property can be set to `none` or `manipulation` to remove the delay. Finally, since iOs 8, Safari doesn't delay "slow" taps anymore. There are some caveats though, which can be found in this excellent article on which this summary is based: http://developer.telerik.com/featured/300-ms-click-delay-ios-8/ Note that this change does not affect the `ngSwipe` directive. Issues with interactive elements (input, a etc.) when parent element has ngClick: Closes angular#4030 Closes angular#5307 Closes angular#6001 Closes angular#6432 Closes angular#7231 Closes angular#11358 Closes angular#12082 Closes angular#12153 Closes angular#12392 Closes angular#12545 Closes angular#12867 Closes angular#13213 Closes angular#13558 Other issues: - incorrect event order - incorrect event propagation - ghost-clicks / failing clickbusting with corner cases - browser specific bugs - et al. Closes angular#3296 Closes angular#3347 Closes angular#3447 Closes angular#3999 Closes angular#4428 Closes angular#6251 Closes angular#6330 Closes angular#7134 Closes angular#7935 Closes angular#9724 Closes angular#9744 Closes angular#9872 Closes angular#10211 Closes angular#10366 Closes angular#10918 Closes angular#11197 Closes angular#11261 Closes angular#11342 Closes angular#11577 Closes angular#12150 Closes angular#12317 Closes angular#12455 Closes angular#12734 Closes angular#13122 Closes angular#13272 Closes angular#13447 BREAKING CHANGE: The `ngClick` override directive from the `ngTouch` module is **deprecated and disabled by default**. This means that on touch-based devices, users might now experience a 300ms delay before a click event is fired. If you rely on this directive, you can still enable it with the `$touchProvider.ngClickOverrideEnabled()`method: ```js angular.module('myApp').config(function($touchProvider) { $touchProvider.ngClickOverrideEnabled(true); }); ``` For migration, we recommend using [FastClick](https://github.com/ftlabs/fastclick). Also note that modern browsers remove the 300ms delay under some circumstances: - Chrome and Firefox for Android remove the 300ms delay when the well-known `<meta name="viewport" content="width=device-width">` is set - Internet Explorer removes the delay when `touch-action` css property is set to `none` or `manipulation` - Since iOs 8, Safari removes the delay on so-called "slow taps" See this [article by Telerik](http://developer.telerik.com/featured/300-ms-click-delay-ios-8/) for more info on the topic. Note that this change does not affect the `ngSwipe` directive.
d2fe3a0
to
bd1133d
Compare
Landed as 0dfc1df |
…ault
This commit deprecates the ngClick directive from the ngTouch module.
Additionally, it disables it by default. It can be enabled in the new $touchProvider with
$touchProvider.ngClickOverrideEnabled() method.
The directive was conceived to remove the 300ms delay
for click events on mobile browsers, by sending a synthetic click event on touchstart.
It also tried to make sure that the original click event that the browser sends after 300ms
was "busted", so that no redundant "ghost-clicks" appear.
There are various reasons why the directive is being deprecated.
to use ngSwipe, you may break parts of your app
FastClick, Tappy!
(There's also hammer.js for touch events / gestures)
when the usual
<meta name="viewport" content="width=device-width">
is set. In IE, thetouch-action
css property can be set tonone
ormanipulation
to remove the delay. Finally,since iOs 8, Safari doesn't delay "slow" taps anymore. There are some caveats though, which can be
found in this excellent article on which this summary is based: http://developer.telerik.com/featured/300-ms-click-delay-ios-8/
Note that this change does not affect the
ngSwipe
directive.Issues with interactive elements (input, a etc.) when parent element has ngClick:
Closes #4030
Closes #5307
Closes #6001
Closes #6432
Closes #7231
Closes #11358
Closes #12082
Closes #12153
Closes #12392
Closes #12545
Closes #12867
Closes #13213
Closes #13558
Other issues:
Closes #3296
Closes #3347
Closes #3447
Closes #3999
Closes #4428
Closes #6251
Closes #6330
Closes #7134
Closes #7935
Closes #9724
Closes #9744
Closes #9872
Closes #10211
Closes #10366
Closes #10918
Closes #11197
Closes #11261
Closes #11342
Closes #11577
Closes #12150
Closes #12317
Closes #12455
Closes #12734
Closes #13122
Closes #13272
Closes #13447
BREAKING CHANGE:
The
ngClick
override directive from thengTouch
module is deprecated and disabled by default.This means that on touch-based devices, users might now experience a 300ms delay before a click event is fired.
If you rely on this directive, you can still enable it with the
$touchProvider.ngClickOverrideEnabled()
method:For migration, we recommend using FastClick.
Also note that modern browsers remove the 300ms delay under some circumstances:
<meta name="viewport" content="width=device-width">
is settouch-action
css property is set tonone
ormanipulation
See this article by Telerik for more info on the topic.
Note that this change does not affect the
ngSwipe
directive.