Opened 14 years ago

Closed 13 years ago

#7808 closed defect (fixed)

A subtle bug in dijit.scrollIntoView()

Reported by: Eugene Lazutkin Owned by: Douglas Hays
Priority: high Milestone: 1.3
Component: Dijit Version: 1.2beta
Keywords: Cc:
Blocked By: Blocking:


In dijit/_base/scroll.js dijit.scrollIntoView() defines an internal function: addPseudoAttrs(element) (line 28). From the context it is suppose to add some private properties to the "element" parameter. Sometimes it is not the case.

At the very beginning of the function it checks if element.offsetParent is null (line 31). If it is, it changes ... the element to scrollRoot, and goes on modifying the scrollRoot instead. The original element is not modified.

It breaks the loop in line 61:

var next = parent._parent;

As you can see addPseudoAttrs() didn't modify its parameter (as explained above), yet the code assumes that parent._parent is there. It is not. Nevertheless this is a relatively harmless mistake.

The more serious mistake occurs in the next loop on line 69:

while(element != scrollRoot){
    parent = element._parent;
    if(parent.tagName == "TD"){

Again this code assumes that "element" has "_parent" property set. It doesn't. So the next line blows up the browser.

This effect was observed in the clients code on FF3 and IE7.

Attachments (1)

dijit.scrollInView.pseudo-fix.diff (500 bytes) - added by Eugene Lazutkin 14 years ago.
My pseudo-fix for the problem: bailing out of the procedure as soon as we detect that something is wrong

Download all attachments as: .zip

Change History (4)

Changed 14 years ago by Eugene Lazutkin

My pseudo-fix for the problem: bailing out of the procedure as soon as we detect that something is wrong

comment:1 Changed 14 years ago by Douglas Hays

Milestone: tbdfuture
Owner: set to Douglas Hays

Is scrollIntoView being called on a node that is not visible or not added to the DOM? That would be bad. I was unable to recreate the problem using dijit/tests/_base/test_scrollNoDTD.html. Please include a description of the steps to recreate the problem.

comment:2 Changed 13 years ago by Douglas Hays

Milestone: future1.3
Status: newassigned

comment:3 Changed 13 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

[16405] Fixes #7808, #8284, #8249 !strict. Rewrite of scrollIntoView to support IE8. Added large automated test suite for scrollIntoView. Added fallbacks to native scrollIntoView method for unsupported browsers or exceptions.

Note: See TracTickets for help on using tickets.