Skip to content

Race condition on toggleMenu in headers #252

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

Closed
lingz opened this issue Dec 4, 2013 · 13 comments
Closed

Race condition on toggleMenu in headers #252

lingz opened this issue Dec 4, 2013 · 13 comments

Comments

@lingz
Copy link

lingz commented Dec 4, 2013

There's a bug easily observed in this demo here:
http://ionicframework.com/examples/side-menu/#

When you click the Toggle on the top left, the menu opens as normal. However, when you click it again, it doesn't close. I investigated and found two competing functions that race:

It's from line 55-62 here:
https://github.com/driftyco/ionic/blob/master/js/controllers/sideMenuController.js

And line 67-69 here:
https://github.com/driftyco/ionic/blob/master/js/ext/angular/src/directive/ionicSideMenu.js

Essentially, when clicking close on the menu, it might either:
#1) First fire the menu tap close event, setting percentage to 0, and then fire the toggleLeft setting it back to 100, which results in no change which is wrong.
#2) Fire it in reverse order, which results in expected behavior.

The simple solution would be to create a lock which prevents the setPercentage function from running more than once every 100ms. Let me know if anyone has any better solutions.

@mlynch mlynch closed this as completed in 861e9ac Dec 4, 2013
@mlynch
Copy link
Contributor

mlynch commented Dec 4, 2013

Shit, my bad. I pushed a little feature this morning that I shouldn't have. I disabled it until I can do it right. Also updated the most recent release.

@lingz
Copy link
Author

lingz commented Dec 4, 2013

Actually, when it is open and you click the toggle, setPercentage tries to fire three times. Once from the actual toggleLeft(), once from the menu being clicked, and once from the gesture control:

Lines 214 -217
https://github.com/driftyco/ionic/blob/master/js/controllers/sideMenuController.js

  // Going right, more than half, or quickly (snap open)
  else if(direction == 'right' && ratio >= 0 && (ratio >= 0.5 || velocityX > velocityThreshold)) {
    this.openPercentage(100);
  }

The click is a gesture, and it seems to default to right, even when there is no movement. Then, the ratio is obviously 1, because the menu is open, so it tries to reopen the menu. If this fires after the toggleLeft(), it'll reopen the menu. So we have a case where there is the menu tap event trying to close it, the gesture event trying to open it, and the toggleLeft trying to close it, all racing.

@mlynch mlynch reopened this Dec 4, 2013
@lingz
Copy link
Author

lingz commented Dec 11, 2013

This seemed to have been fixed momentarily, but on the latest release the problem is still there. toggleLeft doesn't close the leftside when it is open.

mlynch added a commit that referenced this issue Dec 15, 2013
@mlynch
Copy link
Contributor

mlynch commented Dec 15, 2013

Hey, I just pushed a fix for this, I'd love to know if it's working for you now. Thanks!

@thienedits
Copy link

Seems to be working good now on my Nexus 5.

@mlynch
Copy link
Contributor

mlynch commented Jan 31, 2014

If not mistaken this one might still be a bug as it's not related to the claim system but rather the menu implementation

Sent from my iPhone

On Jan 30, 2014, at 8:44 PM, Adam Bradley [email protected] wrote:

Closed #252.


Reply to this email directly or view it on GitHub.

@flinn
Copy link

flinn commented Mar 1, 2014

I believe this still is a bug. It results in some very odd behavior for me, as when I click the button it will occasionally open and then close the menu immediately or it will work as expected...Results are about 50/50.

@mlynch
Copy link
Contributor

mlynch commented Mar 1, 2014

@flinn, I am looking at this right now and trying to narrow it down. Which device are you testing on?

@flinn
Copy link

flinn commented Mar 1, 2014

I've been testing this with an iPhone 4S -- When I use the iOS simulator or the Ripple Emulator I've never seen this behavior, only on the device.

@mlynch
Copy link
Contributor

mlynch commented Mar 1, 2014

Okay, thanks. On my iPhone 5 I can't seem to reproduce it. If you had a test case for us to try that would really help. Thanks!

Edit: also make sure you are using a new version of Ionic.

@flinn
Copy link

flinn commented Mar 1, 2014

My code is a very close replication of this codepen, http://codepen.io/ionic/pen/vqhzt ...

The only changes that I have are to the styling of the button... I am just using a span styled with a background/logo image instead of an < i > element.

@vanderleisp
Copy link

I'm trying to use this codepen http://codepen.io/ionic/pen/vqhzt with a newer version of Ionic but still couldn't.
From version 0.9.24 to the 0.9.25 the main change was the ion- directives, that I added, but don't know what else need to be changed as can't run it with any of the newer releases of Ionic.

@mhartington
Copy link
Contributor

@vanderleisp take a look at our codepens which are all updated

http://codepen.io/ionic/pen/QwamEW

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants