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

Tooltip: Accept HTMLElement and jQuery objects for the content option #1421

Closed
wants to merge 2 commits into from
Closed

Tooltip: Accept HTMLElement and jQuery objects for the content option #1421

wants to merge 2 commits into from

Conversation

jzaefferer
Copy link
Member

Fixes #9278
Closes #983

This replaces #983, which hasn't been updated for a while. I've fixed the known issues and rebased, then noticed a new issue. The first test, "content: element" is failing. I've tracked it down to the _open method, which first sets the given element as the content:

tooltip.find( ".ui-tooltip-content" ).html( content );

Then it later reuses the same element to set it for the live region:

// HTMLElement has no clone() method
if ( content.clone ) {
    a11yContent = content.clone();
    a11yContent.removeAttr( "id" ).find( "[id]" ).removeAttr( "id" );
} else {
    // So we end up using the element directly
    a11yContent = content;
}
$( "<div>" ).html( a11yContent ).appendTo( this.liveRegion );

When passing an element to .html(), it behaves the same as calling append, so the element is removed from the tooltip and added to the live region instead.

To fix this we could just read the HTML from the tooltip element:

a11yContent = $( "<div>" ).html( tooltip.find( ".ui-tooltip-content" ).html() );
a11yContent.removeAttr( "id" ).find( "[id]" ).removeAttr( "id" );
a11yContent.appendTo( this.liveRegion );

That works, in that it passes all existing tests.

@scottgonzalez
Copy link
Member

That sounds like a reasonable approach.

@jzaefferer
Copy link
Member Author

Added a commit with my suggested change and updated tests to also pass in IE8 (verified using browserstack-runner running locally). The .html().toLowerCase() calls are ugly, but I don't see how domEqual() could help here and using .text() would require duplicating the content variable with HTML stripped.

@scottgonzalez
Copy link
Member

Seems good to me.

@jzaefferer jzaefferer deleted the extend-tooltips-content-option branch January 12, 2015 17:50
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.

None yet

2 participants