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

this._times.length is undefined in Map.Drag.js #4311

Closed
oldrich-s opened this issue Mar 9, 2016 · 7 comments
Closed

this._times.length is undefined in Map.Drag.js #4311

oldrich-s opened this issue Mar 9, 2016 · 7 comments
Labels
Milestone

Comments

@oldrich-s
Copy link

I have just got report from my client that this._times.length is undefined in Map.Drag.js in _onDragEnd on line noInertia = !options.inertia || this._times.length < 2;. I could not reproduce the error so far. The error was triggered on IPad, iOS 9.2.1 in WKWebview. options.inertia is kept at default, i.e. true.

My only explanation to this error is that in rare situations the _onDragEnd is called without _onDragStart which then means that this

if (map.options.inertia) {
    this._positions = [];
    this._times = [];
}

is never called and this._times remains undefined.

What do you think?

@IvanSanchez
Copy link
Member

Yes, the only case where this should happen is when the inertia option of the map is changed mid-drag, or somehow _onDragStart not firing properly. I would not be surprised if this is a browser bug about the dragstart event not firing.

I wonder if prosthetic-hand could be used to write a reproducible test case. Otherwise it can be hard and tedious to debug.

Maybe we can just work around the problem by setting this._times in addHooks() no matter what. I'm not sure we want to do that.

@mourner
Copy link
Member

mourner commented Mar 9, 2016

Maybe we can just work around the problem by setting this._times in addHooks() no matter what. I'm not sure we want to do that.

There should be no side effects of this though, so I would add it.

@IvanSanchez
Copy link
Member

If @mourner thinks it's safe to add it, then I'll second.

The change seems quite trivial, so I'll let a newbie commit the changes (wink wink nudge nudge, @yourfirstpr @errebenito @Charlotteis !).

  • Claim the issue
  • Fork the Leaflet repo
  • git clone
  • Create a feature branch like git checkout -b drag-undefineds
  • Edit the code: Initialize this._times (and this._positions for good measure) to an empty array in the addHooks() method of L.Map.Drag
  • Check that you didn't break anything by running npm test
  • git commit and git push
  • Create a pull request

@errebenito
Copy link
Contributor

@IvanSanchez I'm at work currently and therefore unable to take a look at this issue, but it seems simple enough, so if nobody else has claimed it by the time I get home, count me in.

@IvanSanchez
Copy link
Member

@errebenito Let's give @yourfirstpr a bit of time to tweet about it and see if anyone else wants to jump in.

@errebenito
Copy link
Contributor

@IvanSanchez Sure. It's gonna be a long day for me at work either way, so there's even the possibility I won't want to look at more source code by the time I finish my shift here ;)

@IvanSanchez
Copy link
Member

I see @LucasMouraDeOliveira has opened #4324, but I don't think it's quite right and it's looking good.

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

No branches or pull requests

4 participants