Skip to content

Tooltip: Remove name and id from tooltip content #1525

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.

Sign up for GitHub

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

Closed
ghost wants to merge 1 commit into from
Closed

Tooltip: Remove name and id from tooltip content #1525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions tests/unit/tooltip/tooltip_core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,3 +220,35 @@ asyncTest( "multiple active delegated tooltips", function() {
});

}( jQuery ) );

// http://bugs.jqueryui.com/ticket/11272
test ( "remove radio button conflict with live Region and tooltip content", function () {
expect( 3 );
var content = "<div id='content'>" +
"<input type='radio' name='hobby' checked='checked'><label>option 1</label>" +
"<input type='radio' name='hobby'><label>option 2</label>" +
"</div>",
stripped_content = "<div>" +
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

camelCase variable names.

"<input checked=\"checked\" type=\"radio\"><label>option 1</label>" +
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Why are you using single quotes above and escaped double quotes here? Be consistent.

"<input type=\"radio\"><label>option 2</label>" +
"</div>",
element = $( content );
$( "#tooltipped1" ).tooltip({
content: element,
open: function( event, ui ) {
equal( ui.tooltip.children().html().toLowerCase(), stripped_content );
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

This seems a bit brittle. Is it even consistent across all browsers?

Copy link
Author

@ghost ghost Apr 1, 2015

Choose a reason for hiding this comment

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

You are definitely right this is probably too brittle I was thinking it might be better to assert that the content does not contain "name" and "id" with a string search? Rather than trying to match the exact output of .html()

Copy link
Member

@scottgonzalez scottgonzalez Apr 1, 2015

Choose a reason for hiding this comment

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

You should probably just search for the DOM structure you care about and then make assertions based on the elements' attributes.

}
}).tooltip( "open" ).tooltip( "destroy" );
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Missing line breaks and indentation. Please see the style guide.

$( "#tooltipped1" ).tooltip({
content: element[0],
open: function( event, ui ) {
equal( ui.tooltip.children().html().toLowerCase(), stripped_content );
}
}).tooltip( "open" ).tooltip( "destroy" );
$( "#tooltipped1" ).tooltip({
content: content,
open: function( event, ui ) {
equal( ui.tooltip.children().html().toLowerCase(), stripped_content );
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Add messages to make it more obvious what's failing if we regress.

}
}).tooltip( "open" );
});
18 changes: 16 additions & 2 deletions ui/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,28 @@ $.widget( "ui.tooltip", {
tooltipData = this._tooltip( target );
tooltip = tooltipData.tooltip;
this._addDescribedBy( target, tooltip.attr( "id" ) );
// Parse content into DOM remove attributes id and name
if ( typeof content === "string" || content.nodeType ) {
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Please don't remove existing blank lines; this violates our style guide.

if ( typeof content === "string" ) {
tooltip_content_div = document.createElement("div");
tooltip_content_div.innerHTML = content;
dom_content = $(tooltip_content_div);
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

This is silly, use jQuery.

} else {
dom_content = $("<div>").html( content );
}
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Splitting these is wasteful, they're the exact same logic in jQuery.

dom_content.find( "[id]" ).removeAttr( "id" );
dom_content.find( "[name]" ).removeAttr( "name" );
content = dom_content.html();
} else if ( content.jquery ) {
content.removeAttr( "id" ).find( "[id]" ).removeAttr( "id" );
content.removeAttr( "name" ).find( "[name]" ).removeAttr( "name" );
}
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

Splitting these is also wasteful.

tooltip.find( ".ui-tooltip-content" ).html( content );
Copy link
Member

@scottgonzalez scottgonzalez Mar 31, 2015

Choose a reason for hiding this comment

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

You can't do all of this stripping before putting the content into the tooltip. Then the content won't work. You have to follow the previous logic of only stripping for the live region.

Copy link
Author

@ghost ghost Apr 1, 2015

Choose a reason for hiding this comment

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

Hey scott I am working on all the other fixes mentioned here and I read the jshint output. Could you please further explain this comment. Based on my tests if I don't strip before putting the content into the tooltip then it will have already interfered with the other radio buttons on the page. What problems does the stripping cause with the content?

Copy link
Member

@scottgonzalez scottgonzalez Apr 1, 2015

Choose a reason for hiding this comment

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

What other radios on the page? Can you please provide the use case you're considering?

Copy link
Author

@ghost ghost Apr 1, 2015

Choose a reason for hiding this comment

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

I based this jsfiddle off the issue https://jsfiddle.net/o2wk14tq/ .
I think this use-case as shown with his code is selecting some div from the DOM which contains radio inputs and displaying them in a tooltip. If you hover over test, notice that the original radio input is unchecked. By the way thanks for taking the time to review and reply =D

Copy link
Member

@scottgonzalez scottgonzalez Apr 1, 2015

Choose a reason for hiding this comment

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

You're effectively cloning existing content. Can you explain why you would do that for a tooltip?

Copy link
Author

@ghost ghost Apr 1, 2015

Choose a reason for hiding this comment

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

I would say
Firstly essentially it is bad practice to do this and the user could definitely find a way around this problem them self.
Secondly the reason I wanted to fix this issue other than for the user who reported it was that there are only a few things you can put in a tooltip that are going to have external effects. name and id do cause those effects, why not ensure they don't?
Finally I agree if you think this may not be a good fix as its not immediately apparent why a users id would disappear if they were attempting to select inside the tooltip. In which case it would be fixing one niche use-case and breaking another.

Copy link
Author

@ghost ghost Apr 1, 2015

Choose a reason for hiding this comment

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

I finished all the revisions, so thanks so much for the help with the style guide issues, it was a little hard to understand everything =D

Copy link
Member

@scottgonzalez scottgonzalez Apr 1, 2015

Choose a reason for hiding this comment

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

name and id do cause those effects, why not ensure they don't?

Because you're ensuring that they break for other valid uses. Content supplied as the tooltip should be used as is. Only problems created by the live region are considered bugs here.

Copy link
Author

@ghost ghost Apr 2, 2015

Choose a reason for hiding this comment

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

Thanks, ok then we agree, I will make a note in the issue with an explanation that this stripping has to be done by the user if they wish to clone existing content into a tooltip.


// Support: Voiceover on OS X, JAWS on IE <= 9
// JAWS announces deletions even when aria-relevant="additions"
// Voiceover will sometimes re-read the entire log region's contents from the beginning
this.liveRegion.children().hide();
a11yContent = $( "<div>" ).html( tooltip.find( ".ui-tooltip-content" ).html() );
a11yContent.removeAttr( "id" ).find( "[id]" ).removeAttr( "id" );
a11yContent.appendTo( this.liveRegion );

function position( event ) {
Expand Down