Position: New notification API #630

Merged
merged 34 commits into from Apr 24, 2012

Conversation

Projects
None yet
4 participants
Owner

jzaefferer commented Apr 10, 2012

First draft for a new notification API, via using callback, telling you were the of-element is, not just when something flipped. New test page demonstrates usage.

This will make it possible to build widgets upon position that have callouts built-in. With the previous flipped classes all you could do was to flip your callout when you know exactly where it was supposed to be in the first place. That can be done only inside an application, not a widget.

Preview here: http://view.jqueryui.com/position-notification/tests/visual/position/position_feedback.html

Preview

For review only, for now.

@jzaefferer jzaefferer Position: First draft for a new notification API, via using callback,…
… telling you were the of-element is, not just when something flipped. New test page demonstrates usage
479530b

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

tests/visual/position/position.html
my: "left top+20",
at: "left bottom",
of: this,
+ using: function( position ) {
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

I'd like to see the additional data in a separate parameter so that users can continue to cleanly pass position to .css() or .animate().

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away other props then left/top
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

"other props then" -> "props other than"

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

single var declaration at the top of the function

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away other props then left/top
+ options.using = function( props ) {
+ // can't use basePosition, as that gets modified
+ var targetOffset = target.offset(),
+ left = targetOffset.left - props.left,
+ right = (targetOffset.left + targetWidth) - (props.left + elemWidth),
+ top = targetOffset.top - props.top,
+ bottom = (targetOffset.top + targetHeight) - (props.top + elemHeight);
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

Do we have this above? Would it make sense to just $.extend() it ahead of time? At least caching the offset would probably be good.

@jzaefferer

jzaefferer Apr 10, 2012

Owner

We don't have the bottom and right values, but I've refactored it to reuse targetOffset, by cloning that once at the beginning. Leaves us with no extra DOM access, so performance overhead of this feature is very low.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away other props then left/top
+ options.using = function( props ) {
+ // can't use basePosition, as that gets modified
+ var targetOffset = target.offset(),
+ left = targetOffset.left - props.left,
+ right = (targetOffset.left + targetWidth) - (props.left + elemWidth),
+ top = targetOffset.top - props.top,
+ bottom = (targetOffset.top + targetHeight) - (props.top + elemHeight);
+ var feedback = {};

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away other props then left/top
+ options.using = function( props ) {
+ // can't use basePosition, as that gets modified
+ var targetOffset = target.offset(),
+ left = targetOffset.left - props.left,
+ right = (targetOffset.left + targetWidth) - (props.left + elemWidth),
+ top = targetOffset.top - props.top,
+ bottom = (targetOffset.top + targetHeight) - (props.top + elemHeight);
+ var feedback = {};
+ feedback.horizontal = right < 0 ? "left" : left > 0 ? "right" : "center";
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

These are small, why not just set them inline when defining feedback?

@jzaefferer

jzaefferer Apr 10, 2012

Owner

Fixed, didn't see that when refactoring into a separate object.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +216,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+ var using = options.using;
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away other props then left/top
+ options.using = function( props ) {
+ // can't use basePosition, as that gets modified
+ var targetOffset = target.offset(),
+ left = targetOffset.left - props.left,
+ right = (targetOffset.left + targetWidth) - (props.left + elemWidth),
+ top = targetOffset.top - props.top,
+ bottom = (targetOffset.top + targetHeight) - (props.top + elemHeight);
+ var feedback = {};
+ feedback.horizontal = right < 0 ? "left" : left > 0 ? "right" : "center";
+ feedback.vertical = bottom < 0 ? "top" : top > 0 ? "bottom" : "middle";
+ if (Math.max(Math.abs(left), Math.abs(right)) > Math.max(Math.abs(top), Math.abs(bottom))) {
Owner

jzaefferer commented Apr 10, 2012

Addressed all comments. Units pass, test pages look the same. Added a screenshot to the PR description.

@scottgonzalez scottgonzalez commented on an outdated diff Apr 10, 2012

ui/jquery.ui.position.js
@@ -216,6 +220,27 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
+
+ if ( using ) {
+ // we have to proxy, as jQuery.offset.setOffset throws away props other than left/top
+ options.using = function( props ) {
@scottgonzalez

scottgonzalez Apr 10, 2012

Owner

Won't this proxy itself over and over for each element? We're inside a this.each() where each iteration is using the same options object.

jzaefferer added some commits Apr 10, 2012

@jzaefferer jzaefferer Position: Mini refactoring, avoids proxying using callback multiple t…
…imes if more then one element is positioned
623e8e6
@jzaefferer jzaefferer Position: Extend feedback test page to include two mouse-positioned e…
…lements, highlights the 0px center/middle limitation

Also rename the demo file to match the variables names, 'feedback', instead of 'notification'
2602612
@jzaefferer jzaefferer Position: Improve feedback API by giving the center/middle position m…
…ore weight. Also removed themeswitcher from test pages, now load faster
d077f9b
Owner

jzaefferer commented Apr 11, 2012

The demo now includes four mouse-following elements. The issue the previous algorithm had is now gone, and the overhead ended up being really small. Just a little more inline math, nothing fancy.

Owner

jzaefferer commented Apr 12, 2012

See also https://gist.github.com/2366588

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L160
calls https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L42
which may call https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L23
repeatedly without the return value of $.scrollbarWidth() ever changing - cache the value!

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L146
offset calculation is done 4 times, enough to make a function of it?

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L158
parseInt( $.css(), …) || 0 is done 4 times, enough to make a function of it?

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L235
repeated calls to Math.abs et al may benefit from caching the reference to avoid scope lookups

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L260
depending on the number of elements to align within one .position() call, offsets, widths, etc. are calculated over and over again. maybe these can be cached / provided by the callee to avoid multiple execution? (same for $.ui.position.fit.top and $.ui.position.flip.*, obviously)

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L175
https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L177
why is one a literal string and the other a variable? consistency?

Regarding the using callback:
pass the target element so using() may calculate the angle between the two elements to rotate a pointer accordingly

Owner

jzaefferer commented Apr 12, 2012

@scottgonzalez @rodneyrehm I'm done with the refactorings, if you want to take another look

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L45
`$.position.scrollbarWidth()? is (potentially) still executed twice

https://github.com/jquery/jquery-ui/blob/position-notification/ui/jquery.ui.position.js#L83
if the scrollbarWidth is a global constant, make it one. Or does this expect different containers having differently sized scrollbars? (theoretically possible, I guess)

While we are taking flea-shit-optimiziation here, see http://jsperf.com/accessing-scopes in respect to always addressing functions' full namespace instead of "caching references".

Owner

jzaefferer commented Apr 13, 2012

@rodneyrehm while performance tuning that wasn't originally a goal of this PR, I've implemented most anyway, as long as it reduces DOM access. The accessor stuff is going a little too far.

Anyway, I've landed three more refactorings. Will now do at most one calculation of scrollbar width. getScrollInfo reuses other data that we calculate for within anyway.

Probably easier to follow along when you look at the last three commits, instead of the full diff.

looking good!

Don't worry about the coordinate calculation thing. It's a "nice to have" anyways…

Owner

jzaefferer commented Apr 13, 2012

Oh, that's actually one more thing I wanted to look at. We've got the data to calculate the center of both elements at hand, so adding a tan2 call would be easy enough.

I'm not sure the center of both elements is always the thing to go with.

Consider aligning an the "bottom left" of one element with "top right" of another. If both elements are of equal dimension, the angle would be 45°. and the center-based angle would be fine. Should the elements differ in size, though, the center-based angle would deviate from 45° - while that may still be the angle between the "top right" and "bottom left" coordinates.

With the above I argue that position() should not do any calculations of the sort. I'd rather see {top:123, left:123, width:123, height:123} returned (supplied to using). It would, in this case, also help if alignments and offsets parsed from at and my be provided to using. reconstructing "bottom right" from that data is trivial and can be done in the using callback (if and when required).

@jzaefferer jzaefferer Position: Expose target and element dimensions to allow further custo…
…mization, like calculating the angle between the two elements
a5fc9eb
Owner

jzaefferer commented Apr 13, 2012

Thanks for the feedback. The last commit added that data, allowing you to calculate whatever you want with very little overhead. I don't see a way to use dynamic transforms on before/after elements, so added this for now: http://view.jqueryui.com/position-notification/tests/visual/position/position_feedback_rotate.html

muha? AWESOME!

jzaefferer added some commits Apr 17, 2012

@jzaefferer jzaefferer Tooltip: Remove bad docs links from headers 5cf3f92
@jzaefferer jzaefferer Tooltip: Update custom-style demo, remove the ugly padding that was h…
…iding layout issues, use new position feedback API instead of position callout in more then just one place
db592b7

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 17, 2012

ui/jquery.ui.position.js
+ element: withinElement,
+ isWindow: isWindow,
+ offset: withinElement.offset(),
+ scrollLeft: withinElement.scrollLeft(),
+ scrollTop: withinElement.scrollTop(),
+ width: isWindow ? withinElement.width() : withinElement.outerWidth(),
+ height: isWindow ? withinElement.height() : withinElement.outerHeight()
+ };
+ },
+ getOffsets: function( offsets, width, height ) {
+ return [
+ parseInt( offsets[ 0 ], 10 ) * ( rpercent.test( offsets[ 0 ] ) ? width / 100 : 1 ),
+ parseInt( offsets[ 1 ], 10 ) * ( rpercent.test( offsets[ 1 ] ) ? height / 100 : 1 )
+ ];
+ },
+ parseCss: function( element, property ) {
@scottgonzalez

scottgonzalez Apr 17, 2012

Owner

There's not really any reason to expose this. We should just move it into a local function. That's probably true of all of these methods.

@jzaefferer

jzaefferer Apr 17, 2012

Owner

Made getOffsets and parseCss local methods. The others are used in unit tests.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 17, 2012

ui/jquery.ui.position.js
targetElem = target[0],
collision = ( options.collision || "flip" ).split( " " ),
offsets = {},
atOffset,
targetWidth,
targetHeight,
+ targetOffset,
basePosition;
@scottgonzalez

scottgonzalez Apr 17, 2012

Owner

All of the undefined vars should move to the first line of the var.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 17, 2012

ui/jquery.ui.position.js
position = $.extend( {}, basePosition ),
- myOffset = [
- parseInt( offsets.my[ 0 ], 10 ) *
- ( rpercent.test( offsets.my[ 0 ] ) ? elem.outerWidth() / 100 : 1 ),
- parseInt( offsets.my[ 1 ], 10 ) *
- ( rpercent.test( offsets.my[ 1 ] ) ? elem.outerHeight() / 100 : 1 )
- ],
- collisionPosition;
+ myOffset = $.position.getOffsets( offsets.my, elem.outerWidth(), elem.outerHeight() ),
+ collisionPosition,
+ using;
@scottgonzalez

scottgonzalez Apr 17, 2012

Owner

Move undefined vars to first line.

@jzaefferer

jzaefferer Apr 18, 2012

Owner

Done and done.

@scottgonzalez scottgonzalez and 1 other commented on an outdated diff Apr 17, 2012

ui/jquery.ui.position.js
var within = data.within,
- win = $( window ),
- isWindow = $.isWindow( data.within[0] ),
- withinOffset = ( isWindow ? 0 : within.offset().top ) + within.scrollTop(),
- outerHeight = isWindow ? within.height() : within.outerHeight(),
- offsetTop = isWindow ? 0 : within.offset().top,
+ withinOffset = ( within.isWindow ? 0 : within.offset.top ) + within.scrollTop,
@scottgonzalez

scottgonzalez Apr 17, 2012

Owner

Shouldn't it be safe to always access within.offset.top?

@jzaefferer

jzaefferer Apr 18, 2012

Owner

Can't, as $(window).offset() returns null. Any thoughts? Otherwise I think I've addressed everything from this latest review.

@scottgonzalez

scottgonzalez Apr 18, 2012

Owner

I meant that the abstraction is a bit broken if it's not safe. Shouldn't getWithinInfo() handle this for us? Probably just offset: withinElement.offset() || { top: 0, left: 0 }.

@scottgonzalez scottgonzalez commented on an outdated diff Apr 17, 2012

ui/jquery.ui.position.js
var within = data.within,
- win = $( window ),
- isWindow = $.isWindow( data.within[0] ),
- withinOffset = ( isWindow ? 0 : within.offset().left ) + within.scrollLeft(),
- outerWidth = isWindow ? within.width() : within.outerWidth(),
- offsetLeft = isWindow ? 0 : within.offset().left,
+ withinOffset = ( within.isWindow ? 0 : within.offset.left ) + within.scrollLeft,
@scottgonzalez

scottgonzalez Apr 17, 2012

Owner

see comment on L406

@scottgonzalez scottgonzalez commented on the diff Apr 19, 2012

ui/jquery.ui.position.js
+ target: {
+ element: target,
+ left: targetOffset.left,
+ top: targetOffset.top,
+ width: targetWidth,
+ height: targetHeight
+ },
+ element: {
+ element: elem,
+ left: props.left,
+ top: props.top,
+ width: elemWidth,
+ height: elemHeight
+ },
+ horizontal: right < 0 ? "left" : left > 0 ? "right" : "center",
+ vertical: bottom < 0 ? "top" : top > 0 ? "bottom" : "middle"
@scottgonzalez

scottgonzalez Apr 19, 2012

Owner

If you're going to cache these, you should do it at the very top of the file so it only happens once.

@scottgonzalez scottgonzalez commented on the diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -337,36 +383,25 @@ $.ui.position = {

@scottgonzalez scottgonzalez commented on an outdated diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -337,36 +383,25 @@ $.ui.position = {
if ( overLeft < 0 ) {
newOverRight = position.left + myOffset + atOffset + offset + data.collisionWidth - outerWidth - withinOffset;
if ( newOverRight < 0 || newOverRight < Math.abs( overLeft ) ) {
- data.elem
- .addClass( "ui-flipped-right" );
-
position.left += myOffset + atOffset + offset;
}
}

@scottgonzalez scottgonzalez commented on an outdated diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -385,18 +420,12 @@ $.ui.position = {
if ( overTop < 0 ) {
newOverBottom = position.top + myOffset + atOffset + offset + data.collisionHeight - outerHeight - withinOffset;
if ( ( position.top + myOffset + atOffset + offset) > overTop && ( newOverBottom < 0 || newOverBottom < Math.abs( overTop ) ) ) {
- data.elem
- .addClass( "ui-flipped-bottom" );
-
position.top += myOffset + atOffset + offset;
}
}

@scottgonzalez scottgonzalez commented on the diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -216,7 +228,50 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
- elem.offset( $.extend( position, { using: options.using } ) );
+
+ if ( options.using ) {
+ // adds feedback as second argument to using callback, if present
@scottgonzalez

scottgonzalez Apr 19, 2012

Owner

left + targetWidth - elemWidth

@scottgonzalez scottgonzalez commented on an outdated diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -216,7 +228,50 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
- elem.offset( $.extend( position, { using: options.using } ) );
+
+ if ( options.using ) {
+ // adds feedback as second argument to using callback, if present
+ using = function( props ) {
+ var left = targetOffset.left - props.left,
@scottgonzalez

scottgonzalez Apr 19, 2012

Owner

top + targetHeight - elemHeight

@scottgonzalez scottgonzalez commented on the diff Apr 19, 2012

ui/jquery.ui.position.js
} else if ( $.isWindow( targetElem ) ) {
targetWidth = target.width();
targetHeight = target.height();
- basePosition = { top: target.scrollTop(), left: target.scrollLeft() };
+ targetOffset = { top: target.scrollTop(), left: target.scrollLeft() };
} else if ( targetElem.preventDefault ) {
// force left top to allow flipping
options.at = "left top";
@scottgonzalez

scottgonzalez Apr 19, 2012

Owner

We should probably use targetElem instead of options.of here. Unrelated to the changes, but you're already doing a lot of cleanup.

@jzaefferer

jzaefferer Apr 20, 2012

Owner

If I'm mapping this comment correctly, you're referring to the option.of.pageX/Y calls, which we can't replace, as they access event properties.

@scottgonzalez

scottgonzalez Apr 20, 2012

Owner

Yeah, that's mapped properly. targetElem is the event object; target is a jQuery object, targetElem is the first item of target. $( obj )[0] === obj; there's even proof in the conditional. I probably wrote the original code, I'm not sure why I would've switched between the conditional and the assignments.

@scottgonzalez scottgonzalez commented on the diff Apr 19, 2012

ui/jquery.ui.position.js
@@ -216,7 +228,50 @@ $.fn.position = function( options ) {
if ( $.fn.bgiframe ) {
elem.bgiframe();
}
- elem.offset( $.extend( position, { using: options.using } ) );
+
+ if ( options.using ) {
@scottgonzalez

scottgonzalez Apr 19, 2012

Owner

It looks like these calculations are supposed to use offsets, but props will contain position.

Owner

jzaefferer commented Apr 21, 2012

How about a milestone release for position based on this branch? We still haven't gotten any real feedback on the actual feedback API.

@jzaefferer jzaefferer merged commit 252352e into master Apr 24, 2012

Does anyone actually know what this comparison is supposed to tell us? It's mentioned, but not explained anywhere I can find, there are no comments, and from reading the code, I can't really see how this bit of information would ever be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment