basic-web-components / basic-carousel

Lets the user navigate through a sequence of child elements using next/previous buttons and arrow keys
MIT License
3 stars 0 forks source link

Error when removing the carousel #4

Closed MarkusKramer closed 9 years ago

MarkusKramer commented 9 years ago

When the carousel component is removed from the DOM I get:

General error TypeError: Cannot read property '_lightParent' of undefined
    at Object.Polymer.DomApi.DomApi._removeNodeFromHost (http://localhost:7272/bower_components/polymer/polymer-mini.html:607:18)
    at Object.Polymer.DomApi.DomApi.removeChild (http://localhost:7272/bower_components/polymer/polymer-mini.html:504:6)
    at basic-page-dots.Polymer._createDots (http://localhost:7272/bower_components/basic-page-dots/basic-page-dots.html:185:35)
    at basic-page-dots.Polymer.contribute.itemsChanged (http://localhost:7272/bower_components/basic-page-dots/basic-page-dots.html:123:12)
    at Object.Collective._invokeImplementations (http://localhost:7272/bower_components/basic-shared/Collective.js:305:16)
    at Object.Collective.invokeMethod (http://localhost:7272/bower_components/basic-shared/Collective.js:106:17)
    at Object.window.Basic.ContentItems.contribute.contentChanged (http://localhost:7272/bower_components/basic-content-items/basic-content-items.html:21:23)
    at Object.Collective._invokeImplementations (http://localhost:7272/bower_components/basic-shared/Collective.js:305:16)
    at Object.Collective.invokeMethod (http://localhost:7272/bower_components/basic-shared/Collective.js:106:17)
    at Object.<anonymous> (http://localhost:7272/bower_components/basic-children-content/basic-children-content.html:58:25)

The reason seems to be this section in basic-page-dots.html

      // Remove extra dots.
      for (var i = count; i < existingDotCount; i++) {
        var existingDot = existingDots[i];
        Polymer.dom(dotContainer).removeChild(existingDot);
      }

When removing it's children the list existingDots is getting shorter and shorter. Using

      while (existingDots.length > 0) {
        var existingDot = existingDots[0];
        Polymer.dom(dotContainer).removeChild(existingDot);
      }

solves this issue.

Using basic-web-components/basic-carousel#0.6.2

JanMiksovsky commented 9 years ago

Hi @MarkusKramer, thanks for this bug report.

The code in basic-page-dots does look wrong, and I want to fix it. (In general, the topic of removing this component or its children from the DOM could stand more exploration.) But before I just blindly incorporate a fix, I would first like to reproduce the bug you're seeing. I can't see this myself in Chrome under Shady DOM or Shadow DOM.

What I'm trying:

  1. Load the demo page on its its own, or with ?dom=shadow to get native Shadow DOM.
  2. In the debugger, try removing one of the carousel's children. Under Shady DOM, take care to use Polymer.dom(carousel).removeChild(child).
  3. Try removing the basic-carousel itself, or basic-page-dots (including the carousel), or basic-arrow-direction (including the dots and carousel). Again, under Shady DOM, take care to use Polymer.dom().

While I get some incorrect behavior, I'm not seeing the specific bug you're hitting. Can you provide more detail about the environment you're using, or the specific repro steps? I want to make sure I'm actually fixing your bug, not just covering it up.

MarkusKramer commented 9 years ago

Thanks for the quick response. I've created this jsfiddle that reproduces the problem. http://jsfiddle.net/ktbvev4a/1/

It appears the problem only exists if you're using a dom-repeat within the carousel. Hope that helps.

JanMiksovsky commented 9 years ago

@MarkusKramer Just wanted to let you know that I've fixed an additional bug that was preventing your jsFiddle from working as expected.

While fixing the particular bug related to carousel removal, I noticed a separate, rendering issue: the carousel looked as if it were were trying to show one more item that actually existed. Among other things, there were 3 dots, but only 2 images. I traced this to your use of the <template is="dom-repeat">. The carousel was incorrectly interpreting the <template> itself as an element, hence the additional dot.

Your use of <template is="dom-repeat"> inside a list-like component seems like a really useful web component idiom, so I wanted to fix the underlying problem. This is, in fact, a criteria on the evolving Gold Standard checklist for web components: a component should cope with Auxiliary Content like link, script, style, and template.

I've just checked in a fix for this such that all list-like components (including basic-carousel and basic-list-box) now ignore auxiliary content nodes. So you should be able to use a template inside a carousel, and the carousel should reflect the correct number of visible child elements.

This fix will go out in the 0.6.3 release in the next couple of weeks.

JanMiksovsky commented 9 years ago

Oh, forgot to mention: I've posted a demo showing the use of a template within a carousel. Source

MarkusKramer commented 9 years ago

Awesome. I actually noticed that last blank page but never really bothered about it ;-) I m looking forward to the 0.6.3 release. Thanks for the great work on basic-carousel !!

JanMiksovsky commented 9 years ago

Thanks!

Version 0.6.3 has now been released.