Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

Popover and Modals interfering, body classes do not get removed - bug newly introduced with 1.3.2 #71

Open
jgw96 opened this issue Jan 13, 2017 · 22 comments
Assignees
Labels

Comments

@jgw96
Copy link
Contributor

jgw96 commented Jan 13, 2017

From @leschirmeur on November 7, 2016 15:43

Short description of the problem:

Commit ionic-team/ionic-framework@6ed7253 introduces a bug that the "popover-open" class is not removed from body if a modal is open at the same time.

What behavior are you expecting?

Removing a popover must remove the "popover-open" class from body element, even though a modal is open at the same time.

Steps to reproduce:

  1. open a modal
  2. open a popover
  3. close the popover (modal still open)
    --> class "popover-open" not removed from body element
// if condition was newly introduced
if (!modalStack.length) {
     $ionicBody.removeClass(self.viewType + '-open');
}

As modalStack can contain both modals and popovers, the newly introduced if-condition must take the entry type into consideration, i.e. "modal" or "popover". Otherwise, a "popover-open" class is not removed if modalStack contains any modals and vice versa for class "modal-open".

Which Ionic Version? 1.x or 2.x
1.3.2

Run ionic info from terminal/cmd prompt: (paste output below)

Cordova CLI: 6.4.0
Gulp version: CLI version 1.2.2
Gulp local: Local version 3.9.1
Ionic CLI Version: 2.1.0
Ionic App Lib Version: 2.0.0-beta.20
ios-deploy version: 1.9.0
ios-sim version: 5.0.10
OS: Mac OS X El Capitan
Node Version: v4.6.0
Xcode version: Xcode 8.1 Build version 8B62

Copied from original issue: ionic-team/ionic-framework#9069

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @cBevilaqua on November 8, 2016 1:13

Already happened with me. Yesterday i updated to version 1.3.2 and after i opened a modal from a popover, for the second time, the screen has freezed and i see the "popover-open" in the body that is causing this issue.

@jgw96 jgw96 self-assigned this Jan 13, 2017
@jgw96 jgw96 added the v1 label Jan 13, 2017
@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @leschirmeur on November 8, 2016 6:33

Well, yes, it seems I forgot to mention this crucial fact: The screen freezes after opening a modal from a popover and then closing the modal. No screen interaction is possible anymore then.

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @leschirmeur on November 10, 2016 2:46

@brandyscarney this is v1 not v2

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @brandyscarney on November 10, 2016 2:56

@leschirmeur Oops, sorry about that. I must have been trigger happy selecting issues to label. :)

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @leschirmeur on November 10, 2016 6:35

@brandyscarney :-)

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @alaa-alshamy on November 21, 2016 9:35

and this bug makes the app freez in this scenario:
1- open a popover
2- open a modal from that popover without closing the popover
3- then close the modal and popover
and then the app got freez

and that's because the "modal-open" class doesn't remove if u closed the modal and there's a popover open and that class preventing any action on the page

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @alaa-alshamy on November 21, 2016 9:44

sorry i didn't noticed that u r mentioned that scenario above in the comments guys :)

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @TomSeldon on November 23, 2016 14:51

Hi guys, I've just run into the same issue.

Although this appears to be a regression in 1.3.2, it's easy enough to work around.

In my case I was doing something that boiled down to:

myPopover.show();

// on clicking an item in the popover

myPopover.hide();

$ionicModal.fromTemplateUrl('foo.html', { scope: $scope })
    .then(modal => {
        modal.show();
    });

This used to work, but now hits the issue reported here.

However, by waiting for the popover to hide before showing the modal, then the popover-open class is removed from body correctly.

So, the above changes to:

myPopover.show();

// on clicking an item in the popover

myPopover.hide()
    .then(() => {
        // Popover is hidden, so now open the modal
        $ionicModal.fromTemplateUrl('foo.html', { scope: $scope })
            .then(modal => {
                modal.show();
            });
    });

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @alaa-alshamy on November 23, 2016 16:3

i noticed that the problem not with popover only , it's happened if there's any tow of popup active together

so it's happened if there's any tow of those together : modal - alert - popover - confirm and i think even loading

any tow of those i mentioned above active together when close one of them the class will not remove

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @ps1dr3x on December 5, 2016 12:2

+1

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @VinceOPS on December 9, 2016 14:31

Hi. As referenced a few minutes ago, I created an issue for the same bug: ionic-team/ionic-framework#9538.

I wrote a fix (didn't take the pain to create a proper pull request as I don't know if the team is still working on v1...).

Find:

if (!modalStack.length) {

And replace by:

var otherSibling = false;

for (var i = 0; i < modalStack.length; ++i) {
  if (modalStack[i].viewType === self.viewType) {
    // there are other modal (or popover, depending on viewType)
    otherSibling = true;
    break;
  }
}

if (!otherSibling) {

Enjoy

@jgw96
Copy link
Contributor Author

jgw96 commented Jan 13, 2017

From @TomSeldon on December 9, 2016 14:56

Hi @VinceOPS,

I was under the impression that whilst there won't be new features for v1, bug fixes are still going to be addressed.

If you can, I'd definitely create a PR with the fix! :)

@wilsolutions
Copy link

Oooops! One more here with the same issue.
Looks likes somebody have a patch? Any plans to push it? tkx

@shanesmith
Copy link

PR created! I hope this gets merged, it's a pretty nasty yet subtle bug, thankfully the fix was simple.

@CheetahDev
Copy link

Hi @jgw96,

Any plan to merge @shanesmith PR and release a 1.3.4 soon?

Thanks!

@wilsolutions
Copy link

I thought this had been merged in 1.3.5 but it is not :(

@jfbloom22
Copy link

I am not sure why, but jgw96's fix did not work for me. I ended up overriding the ionic .modal-open class with this:
.modal-open{ pointer-events: all; }
I am running 1.3.5 and testing on the latest Chrome browser

@brandon-objectstudio
Copy link

@jfbloom22 Any negative impact you discovered with that styling fix? This seems the quickest way to address something that is really challenging to reproduce without changes to internal ionic code.

@jfbloom22
Copy link

@brandon-applied Nope, no negative impact. I imagine the reason Ionic is preventing pointer-events while a modal is open is to prevent users from clicking on something behind the modal. However, after adding this CSS override, I still cannot click on items behind the modal or around the modal (on larger screens).

@brandon-objectstudio
Copy link

While I haven't identified repro steps yet - I wanted to mention that even with the fix above (to update the modal-open styles) - this looks to still be an issue in some cases (on both iOS and Android). My client's QA team (and CEO) has been able to intermittently encounter issues with the app "freezing" after modals are displayed and dismissed. In certain cases, where we have modals reappear when the app is restored from the background, they are still interactive - showing that the app is not actually frozen but simply not clickable.

While I appreciate that the Ionic team wants to focus on the future - there are still production apps which companies have invested 100s of thousands in to that are being maintained. Convincing decision makers to support a rebuild to another version of Ionic when the team has basically forsaken its early adopters is a pretty tough sell.

This issue is over a year old and just need to get addressed (or PRs merged if already fixed).

@CheetahDev
Copy link

@brandon-applied I can't tell how much your message reflects our situation. We have build two large apps with Ionic v1 (from beta to release) and it has represented huge investments. It's really difficult - as you said - to defend a migration to a newest/"better" (not to mention the complete rewrite) version of Ionic as previous decision board easily see how quickly the v1 support has dropped. I hope the upcoming LTS version (see recent announcement) will offer better business substainability.

@brandon-objectstudio
Copy link

An update on my post above: After further investigation - it appears that the pointer-events modal-open CSS work around did work for me. The scss style sheet that I made the change in was not the one actually in use (as this is a recently migrated project from a different folder structure there were some lingering duplication in the project) - and once I determine this and got the correct style resource referenced, the problem appears at least mitigated.

In this process, I did determine reliable reproduction steps (applies to both iOS and Android):

  • Open a modal
  • Move the app in to the background
  • Resume the app
  • Close the modal
  • At this stage the modal-open class is still present on the body of document and the main content is no longer interactive (ie pointer-events do not fire)

Hopefully these steps can help move this issue to resolution. I still stand by my comments that Ionic really should consider offering a life line to v1 developers - even it it means paid support plans. We believed in web as the future of mobile and believed Ionic was the team to take us there - please repay this faith.

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

No branches or pull requests

6 participants