Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#1965 closed defect (fixed)

[patch] [cla] getAbsolutePosition() broken for scrolled elements in Mozilla

Reported by: mikewse@… Owned by: sjmiles
Priority: high Milestone: 0.9
Component: HTML Version: 0.4
Keywords: Cc:
Blocked By: Blocking:

Description

There is a bug in

dojo.html.getAbsolutePosition()

when run on Mozilla.

What happens is that the reported position is affected by the node's internal scroll, so if I am asking about a scrollable DIV's position I get different values depending on how the DIV's content is scrolled. This doesn't happen on IE.

I traced the problem down to sumAncestorProperties() that is only used for Mozilla, and the bug is that this function is not only summing values from ancestors but also from the node itself. With the two added lines below it works correctly:

dojo.html.sumAncestorProperties = function(/* HTMLElement */node, /* string */prop){ 
  // summary 
  // Returns the sum of the passed property on all ancestors of node. 
  node = dojo.byId(node); 
  if(!node){ return 0; } // FIXME: throw an error? 
  node = node.parentNode; // *** ADD THIS 
  if(!node){ return 0; } // *** ADD THIS 
  
  var retVal = 0; 
  while(node){ 
    ...

Best regards Mike

Change History (12)

comment:1 Changed 13 years ago by guest

This solves #1621. At least for Firefox. Opera still has issues.

comment:2 Changed 13 years ago by guest

It appears this also solves #1947.

comment:3 Changed 13 years ago by bill

Summary: getAbsolutePosition() broken for scrolled elements in Mozilla[patch] [cla] getAbsolutePosition() broken for scrolled elements in Mozilla

CLA confirmed.

comment:4 Changed 13 years ago by guest

#1947 is not affected by the fix, sorry.

dojobug[at]cpusoftware.com

comment:5 Changed 13 years ago by guest

Hmm, ok. I though #1621 and #1947 were the same bug. This certainly solves #1621 in Safari.

comment:6 Changed 13 years ago by mikewse@…

Hi there,

Sorry for nagging, but why isn't anything happening with this bug? It is an easy bug to verify, and the solution is simple (and included in the report).

I'll provide a more detailed explanation below. Please let me know if there is anything more you want me to clarify.

Consider the following element tree:

DIV a (height=100)
DIV b (scrollTop=20)
  c

(The page first has one DIV that currently is 100px high, followed by a DIV with one child that is scrolled 20px)

I want to know the top position of "b" (should be 100) and call dojo.html.getAbsolutePosition(b). In Mozilla, it will do something like:

box = document.getBoxObjectFor(b);
height = box.y - dojo.html.sumAncestorProperties(node, "scrollTop");

box.y will be 100, but the call to sumAncestorProperties will return 20, thereby making the calculation return 80. The reason is because sumAncestorProperties includes element b's own scrollTop in its sum, which of course isn't relevant to b's position (it has the same position independent of how it has scrolled its own children). The function sumAncestorProperties's name suggests that it is only ancestors' properties that should be summed, and not properties from the node itself, and this is not the case with the current implementation.

I understand that you maybe are planning a major refactoring of this function (maybe due to https://bugzilla.mozilla.org/show_bug.cgi?id=340571) and have not had time to do this yet, but couldn't you check in my suggested fix in the meantime?

Best regards Mike

comment:7 Changed 13 years ago by bill

Yeah, I guess that according to the comment for function sumAncestorProperties():

//	summary
//	Returns the sum of the passed property on all ancestors of node.

is shouldn't include the node itself in the calculations (just it's ancestors), so your patch seems to make sense.

comment:8 Changed 13 years ago by dylan

Milestone: 0.9

comment:9 Changed 12 years ago by Adam Peller

Owner: changed from Bryan Forbes to sjmiles

comment:10 Changed 12 years ago by sjmiles

Status: newassigned

Sorry this languished for so long. I only became owner of this recently as you can see, and our review shows your report and fix to be spot on.

comment:11 Changed 12 years ago by sjmiles

Resolution: fixed
Status: assignedclosed

(In [10060]) Make sumAncestorProperties do what it says it will do (+test), fixes #1965.

comment:12 Changed 12 years ago by sjmiles

(In [10066]) Make sumAncestorProperties do what it says it will do (in 0.4 trunk), refs #1965.

Note: See TracTickets for help on using tickets.