ionic-team / ionic-v1

The repo for Ionic 1.x. For the latest version of Ionic, please see https://github.com/ionic-team/ionic
Other
193 stars 187 forks source link

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

Open jgw96 opened 7 years ago

jgw96 commented 7 years ago

From @leschirmeur on November 7, 2016 15:43

Short description of the problem:

Commit https://github.com/driftyco/ionic/commit/6ed725325987559371450bedfed9ed74f9f153cd 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: driftyco/ionic#9069

jgw96 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

From @leschirmeur on November 10, 2016 2:46

@brandyscarney this is v1 not v2

jgw96 commented 7 years ago

From @brandyscarney on November 10, 2016 2:56

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

jgw96 commented 7 years ago

From @leschirmeur on November 10, 2016 6:35

@brandyscarney :-)

jgw96 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

From @ps1dr3x on December 5, 2016 12:2

+1

jgw96 commented 7 years ago

From @VinceOPS on December 9, 2016 14:31

Hi. As referenced a few minutes ago, I created an issue for the same bug: https://github.com/driftyco/ionic/issues/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 commented 7 years ago

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 commented 7 years ago

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

shanesmith commented 7 years ago

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

CheetahDev commented 7 years ago

Hi @jgw96,

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

Thanks!

wilsolutions commented 6 years ago

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

jfbloom22 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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):

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.