Core: provide "includeHidden" parameter in $.ui.scrollParent, needed for a draggable fix #1307
Conversation
| @@ -56,7 +56,7 @@ $.fn.extend({ | |||
| if ( excludeStaticParent && parent.css( "position" ) === "static" ) { | |||
| return false; | |||
| } | |||
| return (/(auto|scroll)/).test( parent.css( "overflow" ) + parent.css( "overflow-y" ) + parent.css( "overflow-x" ) ); | |||
| return (/(auto|scroll|hidden)/).test( parent.css( "overflow" ) + parent.css( "overflow-y" ) + parent.css( "overflow-x" ) ); | |||
There was a problem hiding this comment.
It seems a little odd to consider an overflow: hidden element as a “scrollParent”, but I think I get why you're doing this here. I worry a bit about this having adverse in other cases.
|
There's some weird behavior in the test case with this patch applied: http://jsbin.com/ralivifo/11/edit. If you drag the red box around the edges of the container the interaction gets choppy. Not sure what's up or if this is related. |
|
@tjvantoll, what's happening now is that it's scrolling inside the parent because of the |
|
OK, so the problem here is that if we treat While this may be the correct behavior, it might be counterintuitive. Should I have |
I don't like scrolling
And by hidden you mean |
|
The other way to fix the draggable bug is to not modify the scrollParent method but rather check to see if the immediate parent of the draggable is different from the scrollParent but still happened to have a non-zero scrollTop/Left, and if so, add that in. This would preserve the correct behavior without having to account for shenanigans in scroll:true with overflow hidden. |
|
That seems reasonable to me. |
Even though the user is unable to scroll via the UI, authors may have custom scrollbars that programmatically set scrollTop. Therefore, overflow:hidden can be considered a scrollParent.
Developers can programmatically set scrollTop/Left on draggable containers that are overflow:hidden. They must be considered for positioning. Fixes #10147
mikesherov
changed the title
|
@tjvantoll can you rereview. I took the approach of adding a param to Also, by providing a parameter, I can keep the existing behavior of scrollParent and not introduce any backCompat issues. The default, with no arguments, retains the old behavior. |
While it is true that overflow:hidden elements can be scrolled programatically, this breaks user expectation. Therefore, do not scroll inside an overflow:hidden container.
|
This does fix the issue (http://jsbin.com/ralivifo/17/edit), but don't like the idea of adding a boolean parameter to Note that if we do move forward with this we would have to update the API docs too (http://api.jqueryui.com/scrollParent/). |
|
Only reason I made it public was because there exists a ticket or two where this question is open for debate. Four ways to tackle this:
On Sun, Aug 10, 2014 at 1:44 PM, TJ VanToll notifications@github.com
|
|
I think it's fine to be public, and my vote would be to not document it. |
Agreed. With the perspective of wiping this all away at some point in the future, eh? |
|
@jzaefferer scrollParent is already publicly documented. It's just this parameter that wouldn't be. |
|
Sounds fine. |
|
Ok great! Then I'm going to land these changes. |
|
These are all now landed. |
Even though the user is unable to scroll via the UI, authors
may have custom scrollbars that programmatically set scrollTop.
Therefore, overflow:hidden must be considered scrollable.