Skip to content

Conversation

ngsilverman
Copy link
Contributor

Fixed #7389 - sortable: 'cursor' option didn't override CSS cursor settings

Overrides the CSS cursor styles of the page's elements by injecting a stylesheet.

…sortable: 'cursor' option didn't override CSS cursor settings
@scottgonzalez
Copy link
Member

Considering the fact that this would prevent any mouse events from firing on "real" elements during a sort, I don't think this is a good solution.

@ngsilverman
Copy link
Contributor Author

Good point, though it seems in most practical cases sortable events could be used instead. Another way would be to inject a new CSS rule into a stylesheet, as suggested by @fracmak in #315. Would that be acceptable?

@scottgonzalez
Copy link
Member

I guess if we just injected something like <style>body { cursor: whatever !important; }</style> and then removed it after that'd be ok.

@ngsilverman
Copy link
Contributor Author

Appending a stylesheet does not work with IE, but the old inline css change to the body does, so I added that back.

Tested for Google Chrome v24, Firefox 17 and 18, Safari 5.1.7, Opera 12.14 and IE 7, 8 and 9.

@@ -86,6 +86,16 @@ $.widget("ui.sortable", $.ui.mouse, {

},

_createCursorStylesheet: function( cursor ) {
$stylesheet = $("<style>");
Copy link
Member

Choose a reason for hiding this comment

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

Does $( "<style>*{ cursor: move !important; }</style>" ).appendTo( "body" ) not work in all browsers? Last I checked (a few years ago), it did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it does! Except for IE.

@scottgonzalez
Copy link
Member

We're getting close :-) There are a few changes that need to be made, but before having you change stuff that might go away, can you see if using <style type="text/css"> makes it work in IE?

@scottgonzalez
Copy link
Member

Since this is going to land soon, can you sign our CLA?

@ngsilverman
Copy link
Contributor Author

Nope, adding type="text/css" does not make it work in IE (8 or 9). I've just signed the CLA, what are the few changes needed? :-)

@scottgonzalez
Copy link
Member

Ok, thanks for checking. I didn't think it would make a difference, but figured we should try all possible tricks before having different logic for different browsers. I'll do inline comment for the changes.

this._storedCursor = $("body").css("cursor");
}
$("body").css("cursor", o.cursor);
if( o.cursor !== "auto" ) { // cursor option
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be !o.cursor || o.cursor !== "auto". "auto" is a silly default, null should've been used, but that's out of scope for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you meant o.cursor && o.cursor !== "auto" ;-)

@ngsilverman
Copy link
Contributor Author

All done.

@scottgonzalez
Copy link
Member

Thanks, landed in a692bf9.

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.

2 participants