geut / mithril-transition

A lightweight library for MithrilJS to create your own custom transitions based on the lifecycle of your components.
https://geut.github.io/mithril-transition/
MIT License
39 stars 4 forks source link

IE doesn't support remove() #3

Closed leftiness closed 8 years ago

leftiness commented 8 years ago

Source

 lastElem.remove();

link

Problem?

remove() isn't liked by IE.

link

Solution?

 x.parentNode.removeChild(x)

link

Note

Supporting IE is a pretty pointless goal, so maybe you'll just want to close this issue, and I'll just learn about Modernizr. :)

tinchoz49 commented 8 years ago

I agree with your note, but for now and until the world finally accept that IE is the worst browser of the history :dancers: !! is better keep the support for now.

tinchoz49 commented 8 years ago

I made some changes to add support for >= IE8. Please can you tell me if is working (i don't have a computer or VM with Windows right now)

leftiness commented 8 years ago

See this section on MDN.

If child is actually not a child of the element node, the method throws an exception. The method throws an exception in 2 different ways:

If the child was in fact a child of element and so existing on the DOM, but was removed the method throws the following exception:

  1. ​​Uncaught NotFoundError: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

This plunk is an example of that error. With this example, even Chrome thinks that there is an error.

However, in the latest version of mithril-transition, IE thinks that you made an error while Firefox and Chrome are still happy.

Do you think that IE makes sense? I can do some more debugging later, but I noticed quickly that lastElem thinks that his parentNode is undefined when the NotFoundError is thrown in IE on line 119. I don't really know a lot about vanilla JS DOM manipulation, so maybe that's not a useful thing to note...

tinchoz49 commented 8 years ago

yes, i was using the removeChild in a wrong way, the state of the parentNode in mithril-transition is always taked from the last transition, but if you have old transitions unfinished yet the parentNode.remove(elem) is not going to work.

Thanks for you help by the way!

tinchoz49 commented 8 years ago

try again, i changed it by lastElem.parentNode.removeChild(lastElem)

leftiness commented 8 years ago

The lastElem thinks that his parentNode is undefined at the point where the error is thrown. I'll test it more this weekend and maybe do a pull request if I find a working solution. :)

Unable to get property 'removeChild' of undefined or null reference

Source

tinchoz49 commented 8 years ago

That's weird, today i tested in IE8, IE9 and IE10 and worked ok.

leftiness commented 8 years ago

IE 11.103.10586.0 pic

After your first change, IE isn't broken. Instead of animating the transition, the element disappears for a moment, and then the new element appears. (The guide that I followed does not bother trying to support IE.) However, the error is still present.

tinchoz49 commented 8 years ago

ahh ok, can you show me your anim function? video

leftiness commented 8 years ago

Sure.I'll show you what I've written.

video live

If you know what's wrong there, I'd appreciate a tip. However, I think the animation problem that I'm facing in Chrome is probably just something wrong in my code. Are you thinking that the error in IE is related to my code?

tinchoz49 commented 8 years ago

Hi @leftiness thanks for share the code. I see two problems here:

  1. Your vendor.js has an outdated mithril-transition version.
  2. In IE your events animationend is executing three times by each transition. If you execute three times the event cbLast is natural that the lastElem does't have a parentNode because you deleted the element on the first event execute.

I'm going to close this issue since the error about the remove() support is fixed.

This code works:

transition = require "mithril-transition"

# From Modernizr
whichTransitionEvent = ->
  t = undefined
  el = document.createElement('fakeelement')
  transitions =
    'animation': 'animationend'
    'webkitAnimation': 'webkitAnimationEnd'
    'mozAnimation': 'mozAnimationEnd'
    'msAnimation': 'msAnimationEnd'
    'oanimation': 'oanimationend'
  for t of transitions
    if el.style[t] != undefined
      return transitions[t]
  return

class Klass
    create: (intro, outro) ->
        return transition
            anim: (lastEl, newEl, dir, cbLast, cbNew) ->
                event = whichTransitionEvent()
                lastEl.addEventListener event, handler = (e) ->
                    e.target.removeEventListener e.type, handler
                    cbLast()
                    return
                newEl.addEventListener event, handler = (e) ->
                    e.target.removeEventListener e.type, handler
                    newEl.classList.remove intro
                    cbNew()
                    return
                # TODO If dir isnt "next" then set different forward/backward animations
                lastEl.classList.add outro
                newEl.classList.add intro
                return

module.exports = new Klass()
leftiness commented 8 years ago
  1. Your vendor.js has an outdated mithril-transition version.

I did a check with an updated mithril-transition version without publishing the change to Github, so the code on Github wasn't up-to-date.

animationend is executing three times by each transition. This code works:

You're right. Thanks for the tip. I'm going to use the transitions = "animation": "animationend" ... bit. :)