Skip to main content

Search and Top Navigation

#5936 closed bug (notabug)

Opened August 12, 2010 04:14PM UTC

Closed August 17, 2010 09:42AM UTC

Last modified October 11, 2012 09:15PM UTC

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 August 16, 2010 03:31PM UTC.

    bug in position plugin

Change History (9)

Changed August 12, 2010 05:45PM UTC by scottgonzalez comment:1

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 August 16, 2010 03:34PM UTC by T.Lindig comment:2

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.

Changed August 16, 2010 03:38PM UTC by scottgonzalez comment:3

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.

Changed August 16, 2010 04:12PM UTC by T.Lindig comment:4

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

Changed August 16, 2010 04:42PM UTC by scottgonzalez comment:5

You haven't described a single bug yet.

Changed August 17, 2010 09:04AM UTC by T.Lindig comment:6

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.

Changed August 17, 2010 09:42AM UTC by jzaefferer comment:7

resolution: → invalid
status: reopenedclosed

Changed August 17, 2010 11:05AM UTC by rdworth comment:8

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.

Changed October 11, 2012 09:15PM UTC by scottgonzalez comment:9

milestone: TBD

Milestone TBD deleted