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

paper-select causes the app to become unresponsive #925

Closed
chbonser opened this issue May 14, 2018 · 22 comments · Fixed by #1201
Closed

paper-select causes the app to become unresponsive #925

chbonser opened this issue May 14, 2018 · 22 comments · Fixed by #1201

Comments

@chbonser
Copy link
Contributor

chbonser commented May 14, 2018

Occasionally (5 or 10% of the time) paper-select causes my app to become unresponsive.

When this occurs the #ember-basic-dropdown-wormhole contains both an md-backdrop element and a div for the dropdown content #ember-basic-dropdown-content-....

The content div returns the following from getBoundingClientRect(), making it invisible and unclickable:

{
  "x": 0,
  "y": 0,
  "width": 0,
  "height": 0,
  "top": 0,
  "right": 0,
  "bottom": 0,
  "left": 0
}

The md-backdrop element masks the rest of the app and does not respond to clicks making the app unresponsive. There are no errors in the console.

The only other unusual thing about this use case is that the drop down exists in a modal (which animates open) which has tabs (with animations between tabs). Could the animation of the parent context be causing this bug?

Related (?) Bugs

This was observed with both Ember 3.0.0 and 3.1 applications. I'm also using paper-select with a 3.0 application and this behavior does not occur

I do sometimes experience two other issues which I'll write up separately:

  • sometimes the options list appears a few hundred px above the expected location
  • sometimes the user has to click multiple times to make a selection
@panthony
Copy link
Contributor

panthony commented May 14, 2018

I did experience the "click multiple times to make a selection". Like I had to click exactly on the label's text and not anywhere else, but since it was hard to reproduce easily I gave up trying to figure out why.

For the options list that appears above the expected location it may have been recently fixed (#895).

@chbonser
Copy link
Contributor Author

@panthony Thanks! Nice find on #895. I'll cross that off my list of potentially related things.

I'm still digging into my original issue. And I've made an additional observation... sometimes the paper-dialog content (and/or md-backdrop) are left in the dom after the paper-dialog has been removed. It seems clear that something about my use case breaks how ember-paper removes elements from the wormholes.

@chbonser
Copy link
Contributor Author

I've traced this into ember-css-transitions. The callback provided to later here never gets called when the problem occurs. The code in that callback is responsible for cleaning up the cloned dom elements.

Anyone have ideas as to why the callback never fires? Or advice on how to debug this further?

@panthony
Copy link
Contributor

@chbonser
Copy link
Contributor Author

@panthony that was my thought too. I added a console statement there and it isn't called.

@chbonser
Copy link
Contributor Author

To be doubly sure, I setup a debugger to trigger only if the 'timeout' event returned by the later call is about to be cleared. I recreated the issue and the debugger is not tripped. Something else is canceling (skipping?) the later call.

@chbonser
Copy link
Contributor Author

When I can get the app into the error state there are hundreds of timers in backburner's _timers prop and many have error like this one...

Error at Backburner._setTimeout (http://localhost:4200/e/assets/vendor.js:25400:38) 
at Backburner.later (http://localhost:4200/e/assets/vendor.js:25193:25) 
at Function.run.later (http://localhost:4200/e/assets/vendor.js:38617:31) 
at leaveHandler (http://localhost:4200/e/assets/vendor.js:164386:21)

Or this one

Error at Backburner._setTimeout (http://localhost:4200/e/assets/vendor.js:25400:38) 
at Backburner.later (http://localhost:4200/e/assets/vendor.js:25193:25) 
at Function.run.later (http://localhost:4200/e/assets/vendor.js:38617:31) 
at Ember.run.schedule (http://localhost:4200/e/assets/vendor.js:154603:35) 
at invoke (http://localhost:4200/e/assets/vendor.js:24832:24) 
at Queue.flush (http://localhost:4200/e/assets/vendor.js:24752:25) 
at DeferredActionQueues.flush (http://localhost:4200/e/assets/vendor.js:24905:31) 
at Backburner.end (http://localhost:4200/e/assets/vendor.js:25037:42) 
at Backburner._boundAutorunEnd (http://localhost:4200/e/assets/vendor.js:24999:23)

I've turned on Ember.run.backburner.DEBUG = true; and will try to reproduce the error again...

Does this create any additional ideas for anyone?

@panthony
Copy link
Contributor

I'm also using paper-select with a 3.0 application and this behavior does not occur

It may be a regression in the backburner introduced in 3.1.

Looking at ember changelog there was one that one reverted in v3.1.0-beta.4:

emberjs/ember.js#16297

@chbonser
Copy link
Contributor Author

I've been working on an Ember 3.0 app today and saw the same issue. (original post is updated)

@betocantu93
Copy link
Contributor

I built a cordova app using ember paper and I had to extend quite a few components to make it more mobile friendly, one of those components was paper-backdrop, because I too had the issue described before with the backdrop when opening and closing the sidenav, I thought it was because I added a Hammer swipe event listener to close/open the sidenav, something related to ghost clicks, but now that I see others with the issue I feel a little of relief "?", to be honest I didn't had much time to dig in, but I manage to solve it (not sure why) by extending the paper-backdrop and overriding the func addDestroyedElementClone(/original, clone/) {} notice there's no this._super(...arguments). Hope it helps.

@chbonser
Copy link
Contributor Author

@betocantu93 Thanks for chiming in with your observation. Two questions for you...

  1. How easy was it to reproduce the issue for you? I have not found a way to consistently reproduce it which makes testing possible solutions difficult.
  2. How exactly did you change addDestroyedElementClone?

paper-backdrop's definition is...

addDestroyedElementClone(original, clone) {
  original.parentNode.appendChild(clone);
},

The super class definition (from ember-css-transitions mixin:transition-mixin)...

addDestroyedElementClone(original, clone) {
  original.parentNode.insertBefore(clone, original.nextElementSibling);
},

insertBefore and appendChild are very similar in behavior. The main difference is perhaps in the order in which the cloned element is inserted into the DOM. From my reading of the code in transition-mixin the order should not matter since it keeps a reference to the clone to use to remove it (vs a potential order dependent look up in the dom).

@betocantu93
Copy link
Contributor

@chbonser answering your questions

  1. It happened when I tried to close the sidenav with a gesture of swiping (Hammer, implemented by me) starting from the backdrop, so it happens through onBackdropTap() in the paper-sidenav.js, which by the way is certainly not a tap, its a click and/or touchEnd implemented in paper-backdrop.js, my wild guess is that maybe the swipe gesture didn't trigger any of the click or touchEnd methods or (maybe) somehow doing it twice with ghost clicks involved (clicks are triggered 300ms later than taps or touchs), so again, maybe this leads to the backdrop staying there for a few seconds after the sidenav has closed, yes, after a few seconds the left over backdrop disappears which is the wierdest.

  2. I left it empty, literally addDestroyedElementClone () {}. I was poking around, trying some console.logs, more like an try - error attempts, it finally worked and I left it at that, for an iteration maybe later.

I didn't read the mixin:transition-mixin implementation, but as far as I can tell now, doing nothing in that method works in my scenario, I'm going to try to debug this in a chance later this week.

@chbonser
Copy link
Contributor Author

@betocantu93 Thanks for the extra details!

This morning I tried the empty method for addDestroyedElementClone and sadly I was still able to lock up the app. This time however, the backdrop clone wasn't the problem (as it never got cloned), instead several other things were left in the dom... several instances of ember-basic-dropdown-wormhole and an instance of md-dialog-container.

My suspicion is that backburner's timers queue is getting jacked up and stops calling timers and the app is off the rails after that.

@chbonser
Copy link
Contributor Author

I've upgraded to Ember 3.3 and can no longer create the issue. I don't know for sure but I suspect that backburner's 'new microtask-based autorun architecture' may have resolve the issue. It was included in ember since 3.2.0-beta.1.

Related backburner issue... BackburnerJS/backburner.js#332

I'm going to close this issue for now. We can reopen it if someone experiences the issue with Ember 3.2+

@Subtletree
Copy link
Collaborator

Subtletree commented Sep 9, 2018

I think I'm seeing this on 3.3.2 with paper-menu. Will see if I can track it down

@patodevilla
Copy link

I am also experiencing this in ember 3.3.2

@chbonser
Copy link
Contributor Author

My biggest struggle with this issue is how hard it was to reproduce. If someone can provide a way to consistently reproduce the issue, I'm willing to look into it with you (I'm 'cbonser' on the Ember community discord).

@chbonser chbonser reopened this Sep 13, 2018
@heberuriegas
Copy link

heberuriegas commented Sep 15, 2018

Same problema here, any workaround?

backspace added a commit to backspace/prison-rideshare-ui that referenced this issue Dec 17, 2018
I found a few more that I think might have worked but then
I got into some vortex of breakage, possibly the same
mentioned here:
adopted-ember-addons/ember-paper#925

I’m updating everything I can that works with no
modifications before I try to update Ember etc.
@panthony
Copy link
Contributor

panthony commented Dec 26, 2018

I'm reproducing it with paper-menu occasionally with:

  • ember: 3.1.2
  • ember-paper: 1.0.0-beta.7

I feels like it happens when I click somewhere before the opening transition is completed but it does not happen constantly.

But I can't recall a time where I go stuck by clicking anywhere once the menu was properly opened.

@panthony
Copy link
Contributor

I noticed that paper-backdrop has shouldTransition to false if not opaque (make sense since it's invisible) but this property is neither used by paper-backdrop nor transition-mixin.

Maybe implementing this property would, at least, bypass the issue for paper-menu and paper-select as the backdrop is not opaque...

But i'm not sure if this property was expected to be done in ember-paper or ember-css-transition, since there is a comment TransitionMixin not far above I'd say the later..

@panthony
Copy link
Contributor

I noticed that TransitionMixin does nothing if transitionName is missing so for now I did the following in my fork hoping that it will "workaround" the issue I have with paper-menu since I don't really care for transition effect on an invisible div:

  // addon/components/paper-backdrop.js
  shouldTransition: bool('opaque'),
  transitionName: computed('shouldTransition', function() {
    // TransitionMixin is NoOp if transitionName is not given
    return this.get('shouldTransition') ? 'ng' : null;
  }),

@ruzz311
Copy link

ruzz311 commented Mar 21, 2019

@panthony - I followed your lead with the transitionName and I've noticed my problems are gone.

I was noticing this issue intermittently but recently I've added quite a few components that would possibly use backdrop (sidebar, a paper-menu, etc). It wasn't until I render multiple multiple menus that I really saw this happen reliably.

I've not really looked into how that backdrop is managed but just reporting my findings and confirming what @panthony commented above.

Tested On:

Chrome Version 72.0.3626.121
Ember | 3.3.2
Ember Paper | 1.0.0-beta.24

ruzz311 added a commit to ruzz311/ember-paper that referenced this issue Mar 21, 2019
backspace added a commit to backspace/prison-rideshare-ui that referenced this issue May 9, 2019
This has some failures that I’ve “fixed” with this hack:
adopted-ember-addons/ember-paper#925 (comment)

It has other test failures that are skipped in a
forthcoming commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants