Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#5936 closed bug (notabug)

options.of is used as jQuery object, but it could be a selector string

Reported by: T.Lindig Owned by:
Priority: minor Milestone:
Component: ui.position Version: 1.8.4
Keywords: Cc:
Blocked by: Blocking:

Description

in the position-plugin the flowing is written:

if ( options.of.nodeType === 9 ) {
	targetWidth = target.width();
	targetHeight = target.height();
	basePosition = { top: 0, left: 0 };
} else if ( options.of.scrollTo && options.of.document ) {
	targetWidth = target.width();
	targetHeight = target.height();
	basePosition = { top: target.scrollTop(), left: target.scrollLeft() };
} else if ( options.of.preventDefault ) {
	// force left top to allow flipping
	options.at = "left top";
	targetWidth = targetHeight = 0;
	basePosition = { top: options.of.pageY, left: options.of.pageX };
} else {
	targetWidth = target.outerWidth();
	targetHeight = target.outerHeight();
	basePosition = target.offset();
}

But options.of could be a string. so options.of.nodeType is always undefined and never === 9.

Instead of options.of you should use the var target. May be so:

if ( target[0].nodeType === 9 ) {
	targetWidth = target.width();
	targetHeight = target.height();
	basePosition = { top: 0, left: 0 };

Attachments (1)

default_copy.html (3.9 KB) - added by T.Lindig 9 years ago.
bug in position plugin

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by Scott González

Resolution: invalid
Status: newclosed

Just because options.of can be a string doesn't mean it is. It could be a string, a node, a document, a window, a jQuery object or an event.

Changed 9 years ago by T.Lindig

Attachment: default_copy.html added

bug in position plugin

comment:2 Changed 9 years ago by T.Lindig

Resolution: invalid
Status: closedreopened

ok, you misunderstood me.

the problem is, that you use direct options.of instead of the normalized object target.

you access options.of as it would be ever a DOM-object, but that is not save because it could be a string, a node, a document, a window, a jQuery object or an event. (see your answer)

here is the code how i think it should look like:

21 $.fn.position = function( options ) {
22 if ( !options || !options.of ) {
23 return _position.apply( this, arguments );
24 }
25
26 // make a copy, we don't want to modify arguments
27 options = $.extend( {}, options );
28
29 var target = $( options.of ),
30 collision = ( options.collision || "flip" ).split( " " ),
31 offset = options.offset ? options.offset.split( " " ) : [ 0, 0 ],
32 targetWidth,
33 targetHeight,
34 basePosition;
35
36 if ( target[0].nodeType === 9 ) {
37 targetWidth = target.width();
38 targetHeight = target.height();
39 basePosition = { top: 0, left: 0 };
40 } else if ( target[0].scrollTo && target[0].document ) {
41 targetWidth = target.width();
42 targetHeight = target.height();
43 basePosition = { top: target.scrollTop(), left: target.scrollLeft() };
44 } else if ( options.of.preventDefault ) {
45 // force left top to allow flipping
46 options.at = "left top";
47 targetWidth = targetHeight = 0;
48 basePosition = { top: options.of.pageY, left: options.of.pageX };
49 } else {
50 targetWidth = target.outerWidth();
51 targetHeight = target.outerHeight();
52 basePosition = target.offset();
53 } 

In line 44 and 48 I am not sure, how it would be correct. It now works only, if you got a jQuery-Event object because preventDefault, pageY and pageX are properties by jQuery, isn't it? So if you got a regular javascript event-object it will not work.

However, i add also the default_copy.html from the position example and modified the of-option. If run that, you will get exceptions.

comment:3 Changed 9 years ago by Scott González

Resolution: invalid
Status: reopenedclosed

The "normalized object" isn't always valid, as you've obviously noticed since you didn't use it for all of your conditionals. Please stop re-opening this bug.

comment:4 Changed 9 years ago by T.Lindig

but it is still a open bug? why you closed it?

comment:5 Changed 9 years ago by Scott González

You haven't described a single bug yet.

comment:6 Changed 9 years ago by T.Lindig

Resolution: invalid
Status: closedreopened

I have described a bug. I have made and add a test case, where the bug appear. See the attached file line 47. The current implementation did not work with $(document) and $(window) as option.of, but it should.

46	                        $('.positionable').position({
47	                                of: $(document), //$(window) did also not work
48	                                my: $('#my_horizontal').val() + ' ' + $('#my_vertical').val(),
49	                                at: $('#at_horizontal').val() + ' '+ $('#at_vertical').val(),
50	                                offset: $('#offset').val(),
51	                                using: using,
52	                                collision: $("#collision_horizontal").val() + ' ' + $("#collision_vertical").val()

So don't close that ticket before the bug is fixed.

I have described a solution and explained it, this solution works fine and is better, than the current implementation. If you think, it is not the best solution, show us a better one.

comment:7 Changed 9 years ago by Jörn Zaefferer

Resolution: invalid
Status: reopenedclosed

comment:8 Changed 9 years ago by rdworth

There are two bugs presented here. For clarity I have opened them separately. See:

  • #5962 - Position: option 'of' can be other than type selector, undocumented
  • #5963 - Position: option 'of' accepts jQuery object unless it wraps document

T.Lindig: In the future, please take care to:

  • Present an issue by pointing out a behavior that doesn't match the documented expected behavior. Per #5962, using { of: document } is not documented as supported, but it works. On top of that undocumented feature you were trying a variation, { of: $(document) }. This means what you found is not so much a bug as an undocumented behavior that could be possibly be improved for consistency and convenience.
  • Explain clearly what you expected to see and what you saw instead. This is not done by looking at the source code, and certainly not by jumping straight into what line should be changed how. This is done by
  1. a simplified test case
  2. a list of steps to reproduce
  3. the expected behavior (per documention)
  4. the actual behavior (the bug you see clearly in the test case)
  • Even though you had a test case sooner, steps 2-4 were not done until your very last comment

The current implementation did not work with $(document) and $(window) as option.of, but it should.

  • Create an accurate and relevant summary. The summary 'options.of is used as jQuery object, but it could be a selector string' does not describe a present bug because options.of is documented as being a selector string or an event ( see #5962 )
  • Keep discussions on the Developing jQuery UI Forum. This is a bug tracking system. While it does have a comment capability, it is not designed for back-and-forth meta discourse, and certainly not by way of reopening a ticket again and again in order to further such discussion. Comments and state changes should be related to the summary and description of the ticket. If you read the summary and initial description of this ticket, you'll find that's why it was closed invalid by Scott and Joern. All meta discussion (why was this closed? isn't this a bug? can't options.of sometimes be a string? why don't you use the target? etc.) should occur in a forum appropriate for that.

comment:9 Changed 7 years ago by Scott González

Milestone: TBD

Milestone TBD deleted

Note: See TracTickets for help on using tickets.