Closed Bug 357509 Opened 18 years ago Closed 18 years ago

Problem setting/using nsIDOMRange.setStartBefore

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: stephend, Assigned: WeirdAl)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

Build ID: 2006-10-21-09 of SeaMonkey trunk under Windows XP.

This worked in last night's nightly build; I suspect bug 357445.

Steps to Reproduce:

1. Using Facebook.com, navigate to http://iusb.facebook.com/friends.php? and click [Edit Details]

Expected Results:
Link works

Actual Results:
Link fails, throwing:

Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMRange.setStartBefore]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: http://static.facebook.com/js/socialmap.js?26159 :: ajaxLoadContent :: line 300"  data: no]
Source File: http://static.facebook.com/js/socialmap.js?26159
Line: 300

The code in question does:

function ajaxLoadContent(elem, content){
  if (navigator.appName == 'Netscape' && document.createRange) {
    rng = document.createRange();
    rng.setStartBefore(elem);
    htmlFrag = rng.createContextualFragment(content);
    while (elem.hasChildNodes()) {
      elem.removeChild(elem.lastChild); 
    }
    elem.appendChild(htmlFrag);
  } else {
    elem.innerHTML = content;
  }
}

the line in question is:

rng.setStartBefore(elem);
Blocks: 357445
Attached file testcase
So with document.createRange(), both end points are (document, 0).  mIsPositioned becomes true.

In SetStart(), we skip over http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRange.cpp#718 because mIsPositioned is true, and the element is obviously in the same document as the document itself.  So DoSetRange doesn't get called.

Then we hit the ComparePoints check (nsRange.cpp#724), which will tell you the elem is after the document, so it returns NS_ERROR_ILLEGAL_VALUE.

DOM 2 Range Recommendation, section 2.4, states:

"The start position of a Range is guaranteed to never be after the end position. To enforce this restriction, if the start is set to be at a position after the end, the Range is collapsed to that position. Similarly, if the end is set to be at a position before the start, the Range is collapsed to that position."

Likewise, setStart* according to the spec doesn't require throwing an exception for a start node after the end node.  Errata page for the spec shows no changes to these lines.

In short, I believe this is a valid bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
*** Bug 357510 has been marked as a duplicate of this bug. ***
I have a patch, but I want to test it locally first.
Attached patch patch, v1Splinter Review
Assignee: traversal-range → ajvincent
Status: NEW → ASSIGNED
Attachment #243033 - Flags: superreview?
Attachment #243033 - Flags: review?
Attachment #243033 - Flags: superreview?(bugmail)
Attachment #243033 - Flags: superreview?
Attachment #243033 - Flags: review?(bugmail)
Attachment #243033 - Flags: review?
Comment on attachment 243033 [details] [diff] [review]
patch, v1

Ah, the reason this worked in the old code is because the old nsRange::IsIncreasing was busted when one of the compared nodes were a document.

>@@ -715,20 +715,21 @@ nsresult nsRange::SetStart(nsIDOMNode* a
> 
>   // Collapse if not positioned yet, or if positioned in another doc.
>   if (!mIsPositioned || !InSameDoc(parent, mEndParent)) {
>     DoSetRange(parent, aOffset, parent, aOffset);
> 
>     return NS_OK;
>   }
> 
>-  // the start must be before the end
>+  // the start must be before or equal to the end
>   if (nsContentUtils::ComparePoints(parent, aOffset,
>                                     mEndParent, mEndOffset) == 1) {

Move the test into the above if-statement instead.

with that, r/sr=sicking
Attachment #243033 - Flags: superreview?(bugmail)
Attachment #243033 - Flags: superreview+
Attachment #243033 - Flags: review?(bugmail)
Attachment #243033 - Flags: review+
Attached patch patch with r/sr nit answered (obsolete) — Splinter Review
sicking, I moved the comment into the if statement as well; I figured it was important enough to keep.  Sorry for the awkwardness...
Comment on attachment 243065 [details] [diff] [review]
patch with r/sr nit answered

Don't put the comment inline in the ifstatement. Put it with rest of the comment above.
and a typo fix, and the comment moved properly.
Attachment #243065 - Attachment is obsolete: true
fix checked in by sicking, please retest with Sunday's nightly builds.
we have coverage on this in bug 357523. WFM on a trunk checkout after sicking's checkin.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061021 Minefield/3.0a1 ID:2006102123 [cairo]

Bug 357510 is WFM again after this landed
per comments, marking FIXED.  Thanks, guys!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 357558 has been marked as a duplicate of this bug. ***
Blocks: 357550
Verified FIXED using build 2006-10-22-07 of SeaMonkey trunk under Windows XP; I appreciate the quick turnaround time from both Alex and Jonas on this: thanks, guys!
Status: RESOLVED → VERIFIED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: