Fix for makeClipping/undoClipping issue #63

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

victor-homyakov commented Aug 28, 2012

I've made earlier pull request with failing unit test for issue #1063 with Element#makeClipping()/Element#undoClipping(). Here is a patch for this issue. It deals with exact value of element.style, not with computed Element#getStyle().

@victor-homyakov victor-homyakov Fix for makeClipping/undoClipping issue
I've made earlier pull request with failing unit test for issue #1063 with `Element#makeClipping()`/`Element#undoClipping()`. Here is a patch for this issue. It deals with exact value of `element.style`, not with computed `Element#getStyle()`.
c5ee80d

@savetheclocktower savetheclocktower commented on the diff Aug 31, 2012

src/prototype/dom/layout.js
// called `makeClipping` already. An `undefined` value means we haven't.
- if (Object.isUndefined(madeClipping)) {
- var overflow = Element.getStyle(element, 'overflow');
+ if (Object.isString(madeClipping)) {
@savetheclocktower

savetheclocktower Aug 31, 2012

Collaborator

Shouldn't this be if (!Object.isString(madeClipping))? If it's a string, it means we've already made this element clip, and so we're doing extra work for no reason.

@savetheclocktower

savetheclocktower Sep 1, 2012

Collaborator

Also, do you need to change these conditionals at all? I might be missing something, but won't it suffice to do var overflow = element.style.overflow || '' like you did in the line below?

@victor-homyakov

victor-homyakov Sep 5, 2012

Contributor

Yes, you are absolutely correct, I've somehow missed negation in if (!Object.isString(madeClipping)).

And yes again, unchanged if (Object.isUndefined(madeClipping)) should work. I've made here isString() just for symmetry with check at line 1427: if (Object.isString(overflow)).

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