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

detached is called when an element with disable-upgrade is removed from the page #4550

Closed
6 tasks done
ghost opened this issue Apr 19, 2017 · 5 comments
Closed
6 tasks done

Comments

@ghost
Copy link

ghost commented Apr 19, 2017

Description

When removing an element with disable-upgrade from the page the detached method gets called. This can lead to errors if the detached method of the element or its behaviors relies on initialized properties (e.g. PolymerElements/iron-a11y-keys-behavior#71).

In my opinion detached should never be called if the element has disable-upgrade on it.

Live Demo

Thanks to @valdrinkoshi for the jsbin:
http://jsbin.com/miqoxuf/3/edit?html,console,output

Steps to Reproduce

  1. Create an element with disable-upgrade on it.
  2. Remove the element from the page.

Expected Results

detached is not called, because the element has disable-upgrade on it.

Actual Results

detached is called and can lead to errors.

Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • Safari 9
  • Safari 8
  • IE 11

Versions

  • Polymer: v1.8.1 and v1.9.1
  • webcomponents: v0.7.24
@valdrinkoshi
Copy link
Member

Yes, this looks like a bug, since detached shouldn't be called without a prior attached.

Just to understand better the use case: why are you detaching an element before it gets even upgraded? This looks like something that might lead to poor performance anyways, since you're creating elements in the wrong spot in the first place, right?

@ghost
Copy link
Author

ghost commented Apr 20, 2017

Some parts of our single-page app are initially not visible but can become visible via user interaction. We heavily improved our performance by using disable-upgrade on elements within these parts and removing disable-upgrade only when the element becomes visible.
If the user navigates to another view within the page, the old content gets replaced via Ajax. This can lead to the removal of elements with disable-upgrade from the page.

We would like this issue to be fixed in a future Polymer update but since we are only experiencing problems in combination with the iron-a11y-keys-behavior a quick workaround there would be nice. That's why I submitted a quick little workaround in this pull request: PolymerElements/iron-a11y-keys-behavior#71.

@valdrinkoshi
Copy link
Member

Thanks for the clarification 👌
Unfortunately we cant afford to have a workaround on iron-a11y-keys-behavior and ship a new version, as this bug might affect any polymer element doing stuff on detached.
I can only think of app side workarounds like overriding the detached method on the instance causing troubles, but meh, not great.

@sorvell @kevinpschaaf any suggestions?

kevinpschaaf added a commit that referenced this issue Apr 28, 2017
Due to #4550, the feature has a flaw for native ES6 classes and would be better implemented as either a mixin or patch to `customElements.define`.
@kevinpschaaf
Copy link
Member

Agree this is a bug, and it should be pretty easily fixable in Polymer 1.x.

However, it does point out a flaw in Polymer 2 class-based syntax, in that there is no easy way to prevent connectedCallback or disconnectedCallback from running, since we don't use indirection as we did in 1.x. As such we're electing to remove the disable-upgrade from 2.x before final release, and will likely re-introduce it in a different form separate from the core library.

@sorvell sorvell added p1 and removed 2.x p2 labels May 16, 2017
sorvell pushed a commit that referenced this issue May 17, 2017
…readied.” This fixes an issue that allowed an element with `disable-upgrade` to process the `detached` callback.
@kevinpschaaf
Copy link
Member

Resolved in 1.x via #4607

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

No branches or pull requests

3 participants