Bug 16749

Summary: DOMRange.surroundContents throws wrong exception (Acid3 bug)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: DOMAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, ian
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 17064    
Attachments:
Description Flags
Proposed patch darin: review-

Eric Seidel (no email)
Reported 2008-01-05 14:15:14 PST
DOMRange.surroundContents throws wrong exception (Acid3 bug) This one should be easy to fix. :) function () { // test 15: Ranges and Comments var doc = getTestDocument(); var c1 = doc.createComment("aaaaa"); doc.appendChild(c1); var c2 = doc.createComment("bbbbb"); doc.appendChild(c2); var r = doc.createRange(); r.setStart(c1, 2); r.setEnd(c2, 3); var msg = 'wrong exception raised'; try { r.surroundContents(doc.createElement('a')); msg = 'no exception raised'; } catch (e) { if ('code' in e) msg += '; code = ' + e.code; if (e.code == e.BAD_BOUNDARYPOINTS_ERR) msg = ''; } assert(msg == '', msg); assert(r.toString() == "", "comments returned text"); return 1; }, function () { return 1; },
Attachments
Proposed patch (5.25 KB, patch)
2008-01-05 20:27 PST, Andrew Wellington 		darin: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2008-01-05 14:15:35 PST
The failure text: Test 15: FAIL (wrong exception raised; code = 3)
Alexey Proskuryakov
Comment 2 2008-01-05 15:56:13 PST
"BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a non-text node." I'm wondering why the spec says that - why are Text nodes different from Comment and CDATASection ones in this regard?
Andrew Wellington
Comment 3 2008-01-05 20:27:41 PST
Created attachment 18294 [details] Proposed patch Ensure that we throw for all non-text nodes not just those that aren't character indexed for offsets. Also moved the code for this exception up the method so it'll throw before HIERARCHY_REQUEST_ERR, which will let this Acid3 test pass.
Eric Seidel (no email)
Comment 4 2008-01-05 22:20:35 PST
Comment on attachment 18294 [details] Proposed patch A couple things. I'm not yet sure we really have enough test coverage for this change. Perhaps I'm just not thinking clear enough at this hour, or perhaps you can just convince me over IRC. Also, our style guidelines forbid c-style casts in c++ code: (int)m_endOffset
Eric Seidel (no email)
Comment 5 2008-01-05 22:39:58 PST
Ian, I think it's unclear as to why this is the "wrong exception".
Ian 'Hixie' Hickson
Comment 6 2008-01-05 22:56:03 PST
woops, i meant to put the comment nodes in an element, not in a document. my bad. will fix.
Eric Seidel (no email)
Comment 7 2008-01-06 00:16:16 PST
FYI the fail message is now: Test 15: FAIL (when trying to surround two halves of comment: wrong exception raised; code = 3)
Alexey Proskuryakov
Comment 8 2008-01-06 00:24:10 PST
As Andrew has discovered, this case meets conditions for both exceptions.
Darin Adler
Comment 9 2008-01-06 01:05:35 PST
Comment on attachment 18294 [details] Proposed patch Looks good! + } else if (m_startContainer->nodeType() != Node::TEXT_NODE) { I would just have used isTextNode() here rather than nodeType(). + if (m_startOffset > 0) { + ec = RangeException::BAD_BOUNDARYPOINTS_ERR; + return; + } This seems wrong. If m_startOffset is >= m_startContainer->maxCharacterOffset, I think we're fine here. review- because of that incorrect case. We need to add a test where the start container is a comment node and the start offset is the end of the comment node.
Ian 'Hixie' Hickson
Comment 10 2008-01-08 19:37:34 PST
(please do let me know if you disagree with what the test currently does)
Darin Adler
Comment 11 2008-02-10 12:10:17 PST
(In reply to comment #10) > (please do let me know if you disagree with what the test currently does) I don't think the specification is clear enough about which exception will be raised when there are multiple things wrong. It's easy to make WebKit match Acid3's expectations and we'll do that. But the specification should be fixed to be specific enough; there might be additional WebKit fixes needed then. Where would we do that? In HTML 5?
Darin Adler
Comment 12 2008-02-10 12:14:25 PST
Committed revision 30123.
Ian 'Hixie' Hickson
Comment 13 2008-02-10 13:17:37 PST
This would be a WebAPI working group thing. We need a volunteer to edit the spec.
Lucas Forschler
Comment 14 2019-02-06 09:04:09 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note You need to log in before you can comment on or make changes to this bug.