search

Famous / engine

MIT License
1.75k stars 250 forks source link

fix: Fix DOMElement#onShow, Node#show, hide (#470) #471

Open panarch opened 9 years ago

panarch commented 9 years ago

DOMElement.onShow should not update display property to block when its node.isShown() is false

panarch commented 9 years ago

@alexanderGugel

If a parent node is hidden, all its children are hidden (this._shown === false)

Currently, although Dispatch#show calls Node#onShow and DOMElement#onShow, it does not affect Node#_shown because Dispatch does not call Node#show. And it's same in case of Dispatch#hide So, the assumption was already broken and I just used that state. I intended affecting to current structure as less as I can, and the result is current PR.

But in this time, if you want to fix not only for the issue I suggested, but also change the current structure to fit the assumption, I have an another idea.

I think it is not possible to fit the assumption in current structure because actual Node#_shown state is more than two;

  1. Node is shown
  2. Parent is shown, Node is hidden
  3. Node is hidden or
  4. Node is shown
  5. Parent is hidden, Node is shown
  6. Node is hidden

It seems... first one might be more intuitive... hmm.. may be not, anyway.

The main key point would be this: Parent node's visible state change should not affect child node's visible(hidden) state which is manually made. And once Node#hide called by user, it should be preserved when user calls ParentNode#show.

I think my PR can be thought as a hotfix to simply fix this without touching the current structure. How about applying this PR first and then thinking about the solution to do refactoring of Node#_shown state to be more intuitive?