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; },
The failure text: Test 15: FAIL (wrong exception raised; code = 3)
"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?
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.
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
Ian, I think it's unclear as to why this is the "wrong exception".
woops, i meant to put the comment nodes in an element, not in a document. my bad. will fix.
FYI the fail message is now: Test 15: FAIL (when trying to surround two halves of comment: wrong exception raised; code = 3)
As Andrew has discovered, this case meets conditions for both exceptions.
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.
(please do let me know if you disagree with what the test currently does)
(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?
Committed revision 30123.
This would be a WebAPI working group thing. We need a volunteer to edit the spec.
Mass moving XML DOM bugs to the "DOM" Component.