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

bug: (angular) gesture partial back #18462

Closed
Kadinvanvalin opened this issue Jun 4, 2019 · 16 comments · Fixed by #18977
Closed

bug: (angular) gesture partial back #18462

Kadinvanvalin opened this issue Jun 4, 2019 · 16 comments · Fixed by #18977
Assignees
Labels

Comments

@Kadinvanvalin
Copy link

Kadinvanvalin commented Jun 4, 2019

Bug Report

Ionic version:

[x] 4.x

Current behavior:
When viewing a Detail view you can swipe back to return to the List view you came from. However if you only drag your finger partially across the screen and return back to the Detail view without removing your finger, the view begins to behave strangely. The List view is now visible.
Expected behavior:
If you start to swipe right to back out of a view and gesture back to pull the page back into view. The previous page should be hidden

Steps to reproduce:

  1. From the home view, tap on push me button
  2. Drag your finger from left to right, stopping in the middle of the screen
  3. Without removing your finger, drag your finger back to the left which will return you to the "other" page
    Related code:
    https://github.com/Kadinvanvalin/ionic-gesture-back
// stack-controller.ts
// I was able to fix locally by adding the else statement.

  endBackTransition(shouldComplete: boolean) {
    if (shouldComplete) {
      this.skipTransition = true;
      this.pop(1);
    } else {
      const leavingView = this.activeView;
      const viewsSnapshot = this.views.slice();
      if (leavingView) { cleanupAsync(leavingView, this.views, viewsSnapshot, this.location); }
    }
  }

Other information:

.ion-page-hidden is removed when you start the gesture, but isn't reapplied at the end of the gesture if you end on the "wrong page"
Ionic info:


Ionic:

   ionic (Ionic CLI)             : 4.3.1 (/Users/kadin/.nvm/versions/node/v11.1.0/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.4.2
   @angular-devkit/build-angular : 0.13.9
   @angular-devkit/schematics    : 7.3.9
   @angular/cli                  : 7.3.9
   @ionic/angular-toolkit        : 1.5.1

System:

   NodeJS : v11.1.0 (/Users/kadin/.nvm/versions/node/v11.1.0/bin/node)
   npm    : 6.9.0
   OS     : macOS
@ionitron-bot ionitron-bot bot added the triage label Jun 4, 2019
@uncvrd
Copy link

uncvrd commented Jun 5, 2019

Wanted to reference my two similar bugs I've discovered that relate to this bug report as well. Great to see that others were able to recreate these problems!

#18284 #18305

@smxdevst
Copy link

smxdevst commented Jun 7, 2019

Same issue on "@ionic/angular": "^4.4.2"

@dansiemens
Copy link

Also experiencing this issue. Page for which .ion-page-hidden is removed will show up behind an activated ion-refresher.

@k-samuelsan
Copy link

Same issue - would appreciate a fix

@AndreasGassmann
Copy link
Contributor

AndreasGassmann commented Jun 14, 2019

I can confirm this issue. In my case, I am using the cordova-plugin-camera-preview plugin on a subpage and have to make the whole content transparent. Everything works fine until I use swipe to go back.

The behaviour is very weird:

  • The sub-page becomes transparent, showing the parent page.
  • "Scrolling" gets applied to the parent page, the sub-page does not move
  • We have a drawing are on the sub-page, touch events still get registered there, but they also scroll the parent page.
  • Using swipe-back fixes the issue (by going back to the parent page).

We are currently migrating our app to Ionic 4 and this branch is not public yet. But if it would help you in reproducing the error, I can try to make it public.

Here is a video: https://streamable.com/tt8p9

EDIT: My temporary solution is to simply disable swipe to go back in the whole application. Ideally I would like to only disable it on some pages, but I couldn't find any docs for it. Is there a way to do that?

@hiepxanh
Copy link
Contributor

yes sir, I have the same issue, but the problem is now with cordova-plugin-googlemaps, I dont understand, why we need transparent child page?

@hiepxanh
Copy link
Contributor

<ion-content style="--ion-background-color: initial!important"> this work for me sir. I think you can try this

@hiepxanh
Copy link
Contributor

Screen Shot 2019-06-23 at 18 14 42
the google map set background transparent, If I set it back to normal, the google map will not work. But if ion content is transparent, it causing overlay problem like this description

@hiepxanh
Copy link
Contributor

can we have a hook on while start gesture ??? that can resolve the issue, I can remove the transparent when we while in gesturing and set it back to transparent if you release your finger

@joshstrange
Copy link

@hiepxanh Yep, I'm in the EXACT same boat as you. Using google maps but the swipe back can cause some odd things to happen. I'm looking into the swipe start gesture stuff as well yeah... I'm stumped.

@joshstrange
Copy link

@AndreasGassmann I've also considered disabling the swipe back but just for 1 page. In the end I think the UX suffers more from doing that than the chance they do a partial swipe back 😞 . But in the thought experiment I did on HOW to accomplish it:

  1. New Swipe service
  2. Swipe service exposes an observable that emits when swipe enable needs to be toggled
  3. app component (or whatever ts file corresponds with the html file containing ion-router-outlet) listens on this observable and sets a class property to true/false based on emit's.
  4. Class property is bound to ion-router-outlet on [swipeGesture]="myPropery"
  5. Swipe Service import Router and listens for route changes and when the page url matches (regex, exact match, however you want) the page you want to disable swipe for it emit's on the observable
  6. OR you could have the ngOnInit/ngOnDestroy for the page in question tell the service to emit but that won't always work I don't think (ion-tabs might play hell with this)

Really what I want to do is detect if they started a swipe and just force them back if they cancel it. Yes, it will be jarring but better that than to have them see the mess that happens if they cancel the swipe then try to do something more on the page.

@joshstrange
Copy link

joshstrange commented Jul 5, 2019

@hiepxanh Ok I've got a REALLY gross work around that involves patching @ionic/angular. It's not pretty but I think it covers all the bases. What I DON'T know is if it breaks anything else. I haven't scripted it yet but it shouldn't be hard to script as a hook.

What I'm doing is 2 things.

  1. Making a an edit to StackController.startBackTransition() so that it doesn't allow you to partially drag, once it detects a swipe it will just go back. You still get the animation, you just don't get it "following/tied to" your finger.
  2. Making an edit to StackController.endBackTransition so that shouldComplete is always true. This is needed because if you don't do this and hold your finger on the screen for the duration of the animation then it will look like you went back but technically you are still on the page you "left" and you have to swipe back a second time.

I do not like or advocate something like this normally, ideally we would at a minimum get the functionality I hacked in as an option like swipeType="instant" that we could put on ion-router-outlet but I'm terrible at naming so I'm sure someone else come up a better name. I make no promises that this will work on versions other than @ionic/angular@4.6.0 but feel free to try to make similar changes to other versions.

The hack

In ./node_modules/@ionic/angular/dist/fesm5.js add the following lines at the bottom of the file right above the export { .....} line. You could instead edit them in the file instead of overwriting them but I found this to be cleaner and more obvious as to what I'm doing.

// Start monkey patching
StackController.prototype.startBackTransition = function () {
    return __awaiter(this, void 0, void 0, function () {
        var leavingView, views, enteringView_1;
        var _this = this;
        return __generator(this, function (_a) {
            switch (_a.label) {
                case 0:
                    leavingView = this.activeView;
                    if (!leavingView) return [3 /*break*/, 2];
                    views = this.getStack(leavingView.stackId);
                    enteringView_1 = views[views.length - 2];
                    enteringView_1.ref.changeDetectorRef.reattach();
                    return [4 /*yield*/, this.wait((/**
                     * @return {?}
                     */
                    function () {
                        return _this.transition(enteringView_1, // entering view
                            leavingView, // leaving view
                            'back', true, false); // <---------- EDIT 1, used to be "'back', true, true);"
                    }))];
                case 1:
                    _a.sent();
                    _a.label = 2;
                case 2: return [2 /*return*/];
            }
        });
    });
};

StackController.prototype.endBackTransition = function (shouldComplete) {
    shouldComplete = true; // <---------- EDIT 2, I added this line
    if (shouldComplete) {
        this.skipTransition = true;
        this.pop(1);
    }
};
// End monkey patching

To be fair... It's not really "monkey patching" as I'm pretty sure that is something you do at runtime but I wasn't able to figure out a way to do true "monkey patching" but maybe someone smarter can figure it out so we don't have to edit node_modules. Also I know my second edit could have been done about 3 different ways but this was the cleanest IMHO and is obviously what I changed (vs just removing the if)

Like I said this should be easy enough to script into an after_prepare hook or something like that...

Edit: Well I feel like a moron, ANOTHER fix that requires NO PATCHING is to just set animated="false" on the ion-router-outlet. My patch will keep the animation (just not let you stop it) so I think I'll still ship with my "hack" but for those who don't want to hack it I understand. I might see if I can't just fork @ionic/angular and add a draggable input to the directive that will be used to do what my hack does. That way everyone could just set it to false to avoid this issue.

Obviously the best of all worlds would be to fix the actual issue here so we could drag it and cancel it and not have these problems but.... well someone smarter is going to have to do that, I'm tapped out.

Edit 2: Ok here is the file to patch it: https://gist.github.com/joshstrange/23546d07c9294671200d3b5f4c85c934 and you just need to add "prepare": "node scripts/patch-ionic-angular.js", to the scripts section in package.json (update the path to wherever you want to save the patch-ionic-angular.js file

@dansiemens
Copy link

@joshstrange seems overkill...better solution would be to just reapply .ion-page-hidden at the end of the gesture as @Kadinvanvalin suggested in the bug report.

@joshstrange
Copy link

@dansiemens 🤦‍♂ Yeah it probably would... Ok, well I'm rushing to ship this app in the next 30min but I will play around with a fix that JUST does that later. Thank you, I somehow missed that reading through everything.

@hiepxanh
Copy link
Contributor

hiepxanh commented Jul 6, 2019

@joshstrange
Your effort deserves to be rewarded 🎖

@manucorporat manucorporat self-assigned this Aug 1, 2019
manucorporat added a commit that referenced this issue Aug 1, 2019
manucorporat added a commit that referenced this issue Aug 6, 2019
manucorporat added a commit that referenced this issue Aug 6, 2019
manucorporat added a commit that referenced this issue Aug 6, 2019
manucorporat added a commit that referenced this issue Aug 6, 2019
@ionitron-bot
Copy link

ionitron-bot bot commented Sep 5, 2019

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

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

Successfully merging a pull request may close this issue.

9 participants