Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9648 closed enhancement (fixed)

dojo.coords() is using a mixed box model

Reported by: Douglas Hays Owned by: Douglas Hays
Priority: high Milestone: 1.4
Component: Core Version: 1.3.2
Keywords: Cc: James Burke
Blocked By: Blocking:

Description

dojo.coords is returning x/y using the border-box and w/h using the margin-box. This causes confusion and inadvertent bugs. In contrast, _abs returns the border-box x/y, but not the border-box w/h which is has but ignores. To maintian backward compatibility, I suggest changing _abs to return the w/h as well, and make it public (position). Existing uses of coords() should probably be changed to either dojo.position() or dojo.marginBox(). The docs should be disambiguated as well to explain the different box types.

Attachments (1)

9648.patch (53.3 KB) - added by Douglas Hays 10 years ago.
(updated) Add dojo.position to base. Change _abs() to position(). Change coords() to position() if equivalent.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by Douglas Hays

need jburke OK (at least for dojo base changes) before committing

comment:2 Changed 10 years ago by James Burke

The _base part of the patch looks good to me, although it looks like all the tests/_base/html.html tests were changed from dojo.coords to dojo.position? There should be some dojo.coords tests in there so we can catch regressions.

The patch also includes changes to demos/beer, maybe that is just an indenting or formatting fix, but the patch seemed larger than just some dojo.coords changes.

comment:3 Changed 10 years ago by dante

Yah, it looks like a global search/replace for coords/marginBox. This can't be right:

-				var p = e._leftTop || dojo.coords(e.node,true);
+				var p = e._leftTop || dojo.marginBox(e.node,true);

the patch touches a lot of demos/ (I see the skew/ demo is changed to use marginBox where it was coords() previously, and that bit of code is trying to restore actual page-level position later on)

comment:4 Changed 10 years ago by James Burke

Thinking more about it, I think it is fine not to patch other files that use dojo.coords, and instead notify the contributors list that dojo.position is now available, and that if they are using dojo.coords, they will likely see a benefit to switching to dojo.position or dojo.marginBox.

However, I do like the idea of cleaning up the dojo.coords references in at least Core so it sets a good model for de-emphasizing dojo.coords. As long as the changes do not break anything. :)

comment:5 Changed 10 years ago by Douglas Hays

Updated patch. coords() DOH test added back to html[_quirks].html. Typo in Dialog.js [ marginBox(e.node,true) ] corrected.

comment:6 Changed 10 years ago by James Burke

With the dojo.coords DOH test in place, I am OK with the _base changes and the rest of the Core changes.

Bill can speak better of the dijit changes, and maybe Adam or Tom can speak to the dojox changes, with dante already commenting on one of the demos changes. Although, I think it is fine to patch _base, Core and perhaps dijit and leave the other dojo.coords-to-position changes as exercises for the other module maintainers if they want to get the dojo.position benefits.

comment:7 Changed 10 years ago by bill

Hi Doug, thanks for working on this.

Should we add a dojo.deprecated() call (rather than just writing "deprecated" in the summary) to dojo.coords()?

BTW, includeScroll is a confusing name for a parameter... since dojo.position() returns the position in the viewport (by default) or in the document (when the second parameter is true) maybe it should be called inDocument.

Anyway, I checked over the dijit changes. You changed a lot of calls to dojo.coords() into dojo.marginBox(). Admittedly a lot of the time dojo.position() and dojo.marginBox() are interchangeable, because either the node has no margin or because it is absolutely positioned, in which case margin is meaningless. But, I think I'd prefer that most of the dojo.marginBox() calls you added to dijit be dojo.position() calls, because the dojo.coords() call was written w/the expectation that it was giving border-box. Unless of course that breaks something. (I think most of the calls to dojo.coords() in dijit now are to get the position, not the size.)

Other than that, happy to see the changes go into dijit.

comment:8 Changed 10 years ago by James Burke

I prefer not having the dojo.deprecated call in _base mainly because of space concerns, so I'm fine with the deprecation being in the documentation/inline comments.

I'm fine with includeScroll, that seemed to make sense to me -- "do I want to account for the position that considers the scrollbars". But I am not tied to it.

Changed 10 years ago by Douglas Hays

Attachment: 9648.patch added

(updated) Add dojo.position to base. Change _abs() to position(). Change coords() to position() if equivalent.

comment:9 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

(In [19516]) Fixes #9648 !strict. Add dojo.position (replacing _abs) to core that returns the x/y start of the node's border-box, and also w/h for the border-to-border size. Most uses of coords() should be changed by component/file owners to use either position() for border-box calculations or marginBox() if the margin-box is needed. The mixed box type of coords is probably a bad programming model for users to adopt. Also added some automated tests for the new position method. I'll update the docs to describe this change.

comment:10 Changed 10 years ago by bill

(In [19522]) Fix placement problems for menu on iframe when iframe is in a div w/position:relative (or position:absolute), and convert code to use dojo.position() rather than dojo.coords(). Refs #9606, #9648 !strict.

comment:11 Changed 10 years ago by bill

(In [19523]) Correctly account for page scroll when testing if the drop down menu has appeared over the drop down button, and the user has moused-up w/out moving off of the button. (BTW this can be tested on test_Button.html by making the browser window very short and then scrolling down to the first row of buttons)

Also switching from dojo.coords() to dojo.position().

Refs #9356, #9648 !strict

comment:12 Changed 10 years ago by bill

(In [19524]) Use dojo.position() instead of dojo.coords(), refs #9648.

comment:14 Changed 10 years ago by Douglas Hays

(In [19529]) References #9648. Add new position() to NodeList? processing along with DOH testcase.

comment:15 Changed 10 years ago by Douglas Hays

(In [19530]) References #9648. Add new position() pseudo-doc to NodeList?.

comment:16 Changed 10 years ago by dante

(In [19532]) tiny nit refs #9648

comment:17 Changed 10 years ago by bill

(In [19533]) Use dojo.position() in dijit tests, instead of dojo.coords(). The one strange thing is the tooltip test, where the height is reported as a fractional value, but apparently that's expected because the height of some node w/in Tooltip is specified as em/%.

Refs #9648 !strict.

comment:18 Changed 10 years ago by Eugene Lazutkin

(In [19937]) Small style change to eliminate "argument redefinition" warning, !strict, refs #9648.

comment:19 Changed 10 years ago by Eugene Lazutkin

(In [19938]) Minor speed improvements: eliminating "with", chaining "if"s, !strict, refs #9648.

Note: See TracTickets for help on using tickets.