Bug 16693

Summary: Acid3 expects WebKit to raise exceptions for invalid qualified names (affects Acid3 test 25)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: DOMAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/REC-xml-names/#ns-qualnames
Bug Depends on: 16833    
Bug Blocks: 17064    
Attachments:
Description Flags
Test case for createDocumentType
none
first attempt
eric: review-
Unify handling of NAMESPACE_ERR and fix Acid3 test 25 mjs: review+

Eric Seidel (no email)
Reported 2008-01-01 01:32:34 PST
Acid3 expects WebKit to raise exceptions for invalid qualified names At least qualified names passed to: document.implementation.createDocumentType The bug is in DOMImplementation.cpp: // FIXME: An implementation of this is still waiting for me to understand the distinction between // a "malformed" qualified name and one with bad characters in it. For example, is a second colon // an illegal character or a malformed qualified name? This will determine both what parameters // this function needs to take and exactly what it will do. Should also be exported so that // Element can use it too. static bool qualifiedNameIsMalformed(const String&) { return false; } The qualified name parsing rules are actually pretty easy: http://www.w3.org/TR/REC-xml-names/#ns-qualnames I think we can fix this to make it spec-compliant w/o raising any compatibility concerns.
Attachments
Test case for createDocumentType (2.45 KB, patch)
2008-01-01 18:07 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
first attempt (9.59 KB, patch)
2008-02-09 12:01 PST, Rob Buis 		eric: review-
DetailsFormatted DiffDiff
Unify handling of NAMESPACE_ERR and fix Acid3 test 25 (16.51 KB, patch)
2008-03-22 02:20 PDT, Eric Seidel (no email) 		mjs: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2008-01-01 18:07:54 PST
Created attachment 18230 [details] Test case for createDocumentType .../dom/DOMImplementation/createDocumentType.html | 13 +++++++++++++ .../dom/DOMImplementation/resources/TEMPLATE.html | 13 +++++++++++++ .../resources/createDocumentType.js | 14 ++++++++++++++ 3 files changed, 40 insertions(+), 0 deletions(-)
Eric Seidel (no email)
Comment 2 2008-01-01 18:09:03 PST
A more complicated test case could be created. But the one I attached at least shows the bug. We should probably actually check the type of exception thrown, but I didn't see a good should* function to check that. Firefox passes all of the subtests in this test case.
Eric Seidel (no email)
Comment 3 2008-01-29 14:56:58 PST
*** Bug 17074 has been marked as a duplicate of this bug. ***
Rob Buis
Comment 4 2008-02-09 12:01:00 PST
Created attachment 19020 [details] first attempt This patch incorporates and passes Erics testcase as well as the acid3 test. Note that in most cases the check could be done inside parseQualifiedName too, though createAttributeNS is a bit different here. To pass the tests this suffices though, and there are no regressions in the current layout tests. Cheers, Rob.
Eric Seidel (no email)
Comment 5 2008-02-09 13:26:49 PST
Comment on attachment 19020 [details] first attempt Looks like you have tabs in Document.cpp Document:: functions don't need to use Document:: to reference static methods in the same class. Since you're changing method behavior in Element, you will need to add tests for the changed behavior. (Similar to what was added by bug 16833.) One probably which will probably stop this bug as well is that createElement calls createElementNS (caused the regression that caused bug 16833 to get rolled out). Likewise setAttribute might be calling setAtributeNS. That means that this patch would not only need to test createElementNS and setAttributeNS, but also createElement and setAttribute (it's the lack of testing of those two which is currently blocking bug 16833 from completion). r- w/o more test cases to prove this is correct.
Eric Seidel (no email)
Comment 6 2008-02-09 13:27:18 PST
Comment on attachment 19020 [details] first attempt (Thank you for the patch btw.) It's always great to see you pop out of the woodwork and throw up a patch here and there. :)
Eric Seidel (no email)
Comment 7 2008-03-22 02:20:11 PDT
Created attachment 19964 [details] Unify handling of NAMESPACE_ERR and fix Acid3 test 25 .../createAttributeNS-namespace-err-expected.txt | 8 +- .../createElementNS-namespace-err-expected.txt | 8 +- WebCore/dom/DOMImplementation.cpp | 77 ++++---------------- WebCore/dom/DOMImplementation.idl | 6 +- WebCore/dom/Document.cpp | 51 ++++++++------ WebCore/dom/Document.h | 9 ++- WebCore/dom/Element.cpp | 8 +- WebCore/editing/FormatBlockCommand.cpp | 9 ++- 8 files changed, 72 insertions(+), 104 deletions(-)
Maciej Stachowiak
Comment 8 2008-03-22 02:21:48 PDT
Comment on attachment 19964 [details] Unify handling of NAMESPACE_ERR and fix Acid3 test 25 r=me
Eric Seidel (no email)
Comment 9 2008-03-22 02:52:11 PDT
r31231
Eric Seidel (no email)
Comment 10 2008-03-22 02:52:33 PDT
r31231
Lucas Forschler
Comment 11 2019-02-06 09:04:04 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.