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

Update _checkOffset to handle invisible input #1050

Closed
wants to merge 1 commit into from

Conversation

IanSimpson
Copy link
Contributor

The _findPos function already searches for the nearest visible element, and returns its position. This is good.

The _checkOffset function, however, just deals with the current element, which causes issues where your invisible element is placed oddly by the browser.

The problem is only evident when the datepicker's input element is invisible (e.g. if you're showing an altField instead, and have a button to show the picker) and a descendant of a fixed element, and the screen is scrolled. The conditions never matched, so the documents scroll amount was never subtracted from the datepicker's position.

This fix uses code adapted from the _findPos function to find the nearest visible element and uses that to test the offset. Note that inputHeight and inputWidth are reset to 0, as that's the effective width of an invisible element (although using .outerHeight() actually returns a positive number).

The _findPos function already searches for the nearest visible element, and returns its position. This is good.

The _checkOffset function, however, just deals with the current element, which causes issues where your invisible element is placed oddly by the browser.

The problem is only evident when the datepicker's input element is invisible (e.g. if you're showing an altField instead, and have a button to show the picker) and a descendant of a fixed element, and the screen is scrolled. The conditions never matched, so the documents scroll amount was never subtracted from the datepicker's position.

This fix uses code adapted from the _findPos function to find the nearest visible element and uses that to test the offset. Note that inputHeight and inputWidth are reset to 0, as that's the effective width of an invisible element (although using .outerHeight() actually returns a positive number).
@scottgonzalez
Copy link
Member

Thanks @SpoonNZ. We'll need a ticket filed at http://bugs.jqueryui.com/newticket including a reduced test case showing the problem.

@IanSimpson
Copy link
Contributor Author

Ticket is at http://bugs.jqueryui.com/ticket/9495, and a fiddle at http://jsfiddle.net/aPrAv/

offset.left -= (this._get(inst, 'isRTL') ? (dpWidth - inputWidth) : 0);
offset.left -= (isFixed && offset.left == inst.input.offset().left) ? $(document).scrollLeft() : 0;
offset.top -= (isFixed && offset.top == (inst.input.offset().top + inputHeight)) ? $(document).scrollTop() : 0;
while (offsetTarget && !offsetTarget.is(':visible')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offsetTarget will always be truthy, since it's a jQuery object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there is not visible sibling? Even if this conditional worked, you'd end up with an error later on when trying to access the offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I adapted the code (poorly!) from _findPos(). Easy fix is offsetTarget.length.

Agree if there is no visible sibling we'll end up with an error, although I think that findPos() would have already generated an error in that instance anyway so the code probably wouldn't execute.

while (obj && (obj.type === "hidden" || obj.nodeType !== 1 || $.expr.filters.hidden(obj))) {
    obj = obj[isRTL ? "previousSibling" : "nextSibling"];
}

position = $(obj).offset();
return [position.left, position.top];

Copying/adapting the code from findPos() is probably a poor approach anyway - seems to highlight an underlying problem a bit. I wonder if we're better to create a new variable a little further up the scope for the element we use to position against. Then there just needs to be a single function which can search right, left, and then up the DOM until it finds a visible element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using common code to find the element sounds like the right approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugger, I was afraid you'd reply with something short and concise like that.

@tjvantoll
Copy link
Member

@SpoonNZ Are you still interested in moving forward with this approach?

@tjvantoll
Copy link
Member

Closing due to inactivity. @SpoonNZ if you're still interesting in moving forward with this approach let us know.

@tjvantoll tjvantoll closed this Nov 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants