#5935 closed bug (notabug)

Opened August 12, 2010 03:45PM UTC

Closed August 17, 2010 10:26AM UTC

Last modified October 11, 2012 09:15PM UTC

wrong comparison for center: >>... === verticalDefault << and >> ... === horizontalDefault << instead of >> ... === "center" <<

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

in all places, where the offset option should normalize and checked if it should be centered, are wrong check like this:

if ( options.at[0] === "right" ) {
	basePosition.left += targetWidth;
} else if (options.at[0] === horizontalDefault ) {
	basePosition.left += targetWidth / 2;
}

But this is wrong code. The code only doing the right thing, because ''horizontalDefault'' is initialized with ''"center"''.

The correct code would be:

if ( options.at[0] === "right" ) {
	basePosition.left += targetWidth;
} else if (options.at[0] === "center" ) {
	basePosition.left += targetWidth / 2;
}

I found this wrong comparison for ''horizontalDefault'' and ''verticalDefault'' instead of ''"center"'' 4 times.

Attachments (0)
Change History (9)

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

resolution: → invalid
status: new → closed

How is that wrong? They all have the same values.

Changed August 16, 2010 02:13PM UTC by T.Lindig comment:2

resolution: invalid
status: closed → reopened

It is wrong, because you don't want know if ''options.at[0] === horizontalDefault'',

you want know it if ''options.at[0] === "center"''.

''horizontalDefault'' is a default value, how the name say it. It could be "left", "right" or "center". The check for that is at a other place.

in that place of code you want not know what the default-value is, you want know if you have to half the targetWidth. So you have to check for "center".

May be you should make a test, than you will see what i am talking about.

make ''horizontalDefault = "left";'' and try to get a position centered trough the option ''at:"center"''. I bet with you, it will not work.

Changed August 16, 2010 02:21PM UTC by scottgonzalez comment:3

resolution: → invalid
status: reopened → closed

But horizontalDefault is not left, and never will be. This isn't user configurable. Would you feel better if I changed the variable name to center?

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

If horizontalDefault never be other than "center", for what is that var good for?

If you think it would be only to store the string "center", why did you not store the strings "right" and "bottom" too?

The var ''horizontalDefault'' and ''verticalDefault'' makes sense as default value. So it would be easy to change the default behavior and the code is better to read and understand. But in the 4 places i point out in my first message, it should be replaced by "center". because ' / 2' is what have be done ever for centering, regardless of defaultValue.

Changed August 16, 2010 04:07PM UTC by T.Lindig comment:5

resolution: invalid
status: closed → reopened

Changed August 16, 2010 04:41PM UTC by scottgonzalez comment:6

resolution: → invalid
status: reopened → closed

Changed August 17, 2010 09:52AM UTC by T.Lindig comment:7

resolution: invalid
status: closed → reopened

Scott, i don't understand your ignorance.

You should be grateful for help to improving the code quality of jquery ui.

If you don't want help, you should not work in a open source project.

I show here a bug you can only find by code review and I show you a way to correct it. To see this bug you have to understand the code and what it does. The argumentations, that it "isn't user configurable" is very perfunctory and do not help the project.

Do not waste my time by closing this bug before it is fixed or disproved.

Changed August 17, 2010 10:26AM UTC by jzaefferer comment:8

resolution: → invalid
status: reopened → closed

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

milestone: TBD

Milestone TBD deleted