Skip to content
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

prevent field refresh on keydown event - fixes #110 #115

Closed
wants to merge 3 commits into from

Conversation

b-laporte
Copy link
Member

This PR brings a fix to issue #110.
The underlying problem raised by this issue was that hashspaces loses the value that is being typed by a user when the application updates the field value on the keydown event (or at least before the keyup event) . Indeed hashspaces updates the data model on the keyup event, which is the first event that allows to retrieve the full field value.
As there is no obvious solution to this problem, I decided that the field refresh (i.e. data model synchronization) should be stopped between the keydown and keyup events (note that the global refresh is not stopped, only the field that receives the key events). I think that this is what corresponds to the most frequent scenario, and it prevents 'strange' behaviors - such as the one described in issue #110.
If applications need more advanced possibilities, then they can always develop their own component and use the $getElement() method to access the DOM directly.

@piuccio
Copy link
Contributor

piuccio commented Mar 31, 2014

Dealing with the AT autocomplete taught me a lesson: listening to keydown and keyup is the easiest way to make you dislike your job.
Soon you'll have to deal with copy/paste/cut events which are not keydown/up, as well as clicking on the browser autofill suggestion.

The solution should be the input event, but as you might imagine, it's not supported in IE8 and it's buggy in IE9. There are ways around that.

If you want some inspiration you might want to have a look at twitter typeahead implementation

@benouat
Copy link
Member

benouat commented Mar 31, 2014

We had the same discussion with @PK1A, and we end up in such the same kind of conversation.
Never rely on key-whatever event to deal with input values in environments where binding are used.

Can't we just simply listen to the data model changes instead ?

PS: @b-laporte speaking of $getElement() did you see my comment last week on 33b7794 ?

@olaf-k
Copy link

olaf-k commented Mar 31, 2014

Can't we just simply listen to the data model changes instead ?

The user could (should?) yes, but to update it first, Hashspace needs to retrieve the value, which is the tricky part because of the messy events support (thanks Oba IE!), though I like the clever shim described in the article linked by @piuccio.

@b-laporte
Copy link
Member Author

@piuccio: thanks for the tip - I will use the input event instead of keyup to synchronize the data model. When input is not supported I will either use the shim you propose - or rely on the current implementation.

@benouat: sorry I didn't see your comment on $getElement() - this said, I understand your argument, but I still would prefer relying on getElement() vs. an argument array. The reason is that I would like to update getElement() to support basic selectors as argument (e.g. $getElement(".myclass")) so that people can easily develop their own component templates without caring about the order of the root elements in the component controller (I actually tried to use querySelector() - but because of some limatations I preferred to abandon and come back to an implementation with a simple integer argument)

@b-laporte
Copy link
Member Author

ok - the PR is now updated with the input event, but I still kept keyup for IE8 and IE9. The good news is that now there is no more delay when typing in a field linked to a value that is also displayed in other fields (i.e. changes are immediate, whereas with keyup you get a very little delay).
So this input event is really a good thing - maybe we good integrate a shim for IE8/9 so that applications can use it safely for their validation.. (or at least propose a shim that can be easily integrated if we don't want to put it in the core framework)

@PK1A
Copy link
Contributor

PK1A commented Apr 3, 2014

+1 for using the input event, funnily enough I was suggesting this few weeks back. I agree that this input-related event handling can get quite messy, this is what is done in AngularJS (see in-lined comments:

// if the browser does support "input" event, we are fine - except on IE9 which doesn't fire the
  // input event on backspace, delete or cut
  if ($sniffer.hasEvent('input')) {
    element.on('input', listener);
  } else {
    var timeout;

    var deferListener = function() {
      if (!timeout) {
        timeout = $browser.defer(function() {
          listener();
          timeout = null;
        });
      }
    };

    element.on('keydown', function(event) {
      var key = event.keyCode;

      // ignore
      //    command            modifiers                   arrows
      if (key === 91 || (15 < key && key < 19) || (37 <= key && key <= 40)) return;

      deferListener();
    });

    // if user modifies input value using context menu in IE, we need "paste" and "cut" events to catch it
    if ($sniffer.hasEvent('paste')) {
      element.on('paste cut', deferListener);
    }
  }

  // if user paste into input using mouse on older browser
  // or form autocomplete on newer browser, we need "change" event to catch it
  element.on('change', listener);

@PK1A
Copy link
Contributor

PK1A commented Apr 3, 2014

Can't we just simply listen to the data model changes instead ?

I would say so as well. IMO #space should abstract messiness of detecting input change and allow component developers to write a component that just reacts on model change. This is how I did bootstrap typeahead impl and would be happy to share approaches / lessons learned.

@benouat
Copy link
Member

benouat commented Apr 3, 2014

This is how I did bootstrap typeahead impl and would be happy to share approaches / lessons learned.

I would be interested in listening to that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants