Bug 16833 - Acid3 expects createElementNS to throw exception for localname = ":div" (affects Acid3 test 23)
Summary: Acid3 expects createElementNS to throw exception for localname = ":div" (affe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 16693 Acid3 76143
  Show dependency treegraph
 
Reported: 2008-01-10 20:00 PST by Eric Seidel (no email)
Modified: 2019-02-06 09:04 PST (History)
5 users (show)

See Also:


Attachments
First pass at throwing additional exceptions (3.08 KB, patch)
2008-01-12 17:33 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
A more complete fix (7.45 KB, patch)
2008-01-12 17:33 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Final fixes and adding test cases (20.28 KB, patch)
2008-01-12 17:33 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
full patch, including test cases (23.51 KB, patch)
2008-01-12 17:55 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Gecko tests, some with explanations (4.71 KB, text/html)
2008-01-13 16:19 PST, Jeff Walden (remove +bwo to email)
no flags Details
Gecko tests, with bugs fixed (5.71 KB, text/html)
2008-01-13 16:59 PST, Jeff Walden (remove +bwo to email)
no flags Details
Address Darin's comments and update tests to match mozilla (35.77 KB, patch)
2008-02-01 18:39 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Squashed patch for final review (29.47 KB, patch)
2008-02-03 05:13 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
patch rebased with addition of more tests from Gecko tree (33.44 KB, patch)
2008-03-15 18:28 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Fix xml/xhtml handling of createElement and add tests (36.15 KB, patch)
2008-03-21 14:45 PDT, Eric Seidel (no email)
eric: review-
Details | Formatted Diff | Diff
additional patch needed to fix the test and code (3.90 KB, patch)
2008-03-21 15:38 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix xml/xhtml handling of createElement and add tests (36.71 KB, patch)
2008-03-21 15:39 PDT, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-10 20:00:06 PST
Acid3 expects createElementNS to throw exception for localname = ":div"

We fail to throw exception 14 in the ":div" case.

    function () {
      // test 23: createElementNS() with invalid tag names
      var test = function (name, ns, code) {
        var result;
        try {
          var div = document.createElementNS(ns, name);
        } catch (e) {
          result = e;
        }
        return result && result.code == code;
      }
      assert(test('<div>', null, 5), "no, or wrong, exception for: <div>");
      assert(test('0div', null, 5), "no, or wrong, exception for: 0div");
      assert(test('di v', null, 5), "no, or wrong, exception for: di v");
      assert(test('di<v', null, 5), "no, or wrong, exception for: di<v");
      assert(test('-div', null, 5), "no, or wrong, exception for: -div");
      assert(test('.div', null, 5), "no, or wrong, exception for: .div");
      assert(test('<div>', "http://example.com/", 5), "no, or wrong, exception for: <div>");
      assert(test('0div', "http://example.com/", 5), "no, or wrong, exception for: 0div");
      assert(test('di<v', "http://example.com/", 5), "no, or wrong, exception for: di<v");
      assert(test('-div', "http://example.com/", 5), "no, or wrong, exception for: -div");
      assert(test('.div', "http://example.com/", 5), "no, or wrong, exception for: .div");
      assert(test(':div', null, 14), "no, or wrong, exception for: :div");
      assert(test(':div', "http://example.com/", 14), "no, or wrong, exception for: :div");
      assert(test('d:iv', null, 14), "no, or wrong, exception for: d:iv");
      assert(test('xml:test', "http://example.com/", 14), "no, or wrong, exception for: xml:test");
      assert(test('xmlns:test', "http://example.com/", 14), "no, or wrong, exception for: xmlns:test");
      assert(test('x:test', "http://www.w3.org/2000/xmlns/", 14), "no, or wrong, exception for: x:test");
      document.createElementNS('xmlns:test', "http://www.w3.org/2000/xmlns/");
      return 2;
    },
Comment 1 Eric Seidel (no email) 2008-01-12 15:14:26 PST
Ok, a few things were wrong with his test.  I've fixed them and sent him a diff.  I've written some additional tests for us, and have prepared a patch, which I will attach shortly.
Comment 2 Eric Seidel (no email) 2008-01-12 17:32:35 PST
About to attach patches.  They don't fix the test.  But they do make us agree with Moz more or less.  I still need to check IE to make sure all 3 of us agree.  Hixie's test had several problems which I'm still working on communicating to him.
Comment 3 Eric Seidel (no email) 2008-01-12 17:33:03 PST
Created attachment 18413 [details]
First pass at throwing additional exceptions

 WebCore/dom/Document.cpp |   42 ++++++++++++++++++++++++++++++------------
 1 files changed, 30 insertions(+), 12 deletions(-)
Comment 4 Eric Seidel (no email) 2008-01-12 17:33:05 PST
Created attachment 18414 [details]
A more complete fix

 WebCore/dom/Document.cpp |   63 +++++++++++++++++++++++++++++++++++-----------
 WebCore/dom/Document.idl |    4 +-
 2 files changed, 50 insertions(+), 17 deletions(-)
Comment 5 Eric Seidel (no email) 2008-01-12 17:33:07 PST
Created attachment 18415 [details]
Final fixes and adding test cases

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   38 ++++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 +++
 .../createElementNS-namespace-err-expected.txt     |   37 ++++++++
 .../Document/createElementNS-namespace-err.html    |   13 +++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 +++
 .../resources/createAttributeNS-namespace-err.js   |   89 ++++++++++++++++++++
 .../resources/createElementNS-namespace-err.js     |   88 +++++++++++++++++++
 WebCore/dom/Document.cpp                           |   42 ++++------
 9 files changed, 307 insertions(+), 29 deletions(-)
Comment 6 Eric Seidel (no email) 2008-01-12 17:55:06 PST
Created attachment 18416 [details]
full patch, including test cases

This is a combined version of the previous 3 patches.
Comment 7 Eric Seidel (no email) 2008-01-12 17:59:12 PST
I didn't include a changelog in the patch, but here's the gist:

This patch fixes the interesting test cases in Acid3 test23.  This makes us agree with the W3C as well as Mozilla wrt throwing exceptions from create*NS.  This cannot be a compat issue with IE since IE does not support either of these methods.  Mozilla is also moving to match our/the spec's behavior.

Most interesting was ":div" which we didn't throw about due to not checking prefix.isEmpty(), there were also a few obscure cases from the spec, where DOM Core Level 2 requires that you disallow "xmlns" and "xml" as prefixes when not matched with their correct namespace.
Comment 8 Darin Adler 2008-01-13 09:04:15 PST
Comment on attachment 18416 [details]
full patch, including test cases

Looks like great work!

 3 function quoteString(s) {

Please format JavaScript code so that function braces are on a new line, as in our C code.

 36     shouldThrowWithCode("document.createAttributeNS(" + quoteString(namespace) + ", " + quoteString(name) + ")", code);

Can you use the standard shouldThrow instead of writing your own?

 39 // We use numbers instead of names, because it's slightly easier to read

I think the names are clearer. I found the numbers confusing when looking at the test output. If you find the long qualified names and full exception strings too wordy maybe you could still write your own harness, but make it so only the short part of the names show up: "NAMESPACE_ERR". That's definitely clearer than "5".

 49 shouldBeEqualToString("document.createElementNS(null, undefined).toString()", "[object Element]");

I normally just use shouldBe here and put two sets of quotes around the expected string. Is this style better?

 703     // FIXME: We define these more than once in WebCore, we could use an XMLNSNames.cpp someday

While this is true, I personally don't think it's helpful to have a FIXME for this.

Also, I don't think we want to add a file named XMLNSNames.cpp. I'd suggest we add these XML namespace constants to XMLNames.h.

 704     static const AtomicString xmlnsURI = "http://www.w3.org/2000/xmlns/";

I think it's confusing to have this named xmlnsURL and another one named xmlNamespaceURI meaning something different. The new one should probably be xmlnsNamespaceURI.

 701 static bool checkForNamespaceError(const QualifiedName& qName)

A function that returns a bool and is named "check" confused me. I couldn't figure out whether true meant "had an error" or "OK". It would be better to name this something like hasNamespaceError.

 710     if (qName.prefix() == emptyAtom) // createElementNS(null, ":div")
 714     if (!qName.prefix().isEmpty() && qName.namespaceURI().isNull()) // createElementNS(null, "html:div")

One of these checks uses isEmpty() and the other uses emptyAtom. Why the difference?

 744     // FIXME: the element factories should be fixed to not ignore qName.prefix()

This comment is not clear enough. The comment should state what the factories should do with the prefix rather than just saying "not ignore".

 746     // Then function can stop taking ExceptionCode& as well.

What function? You mean setPrefix? Please name the function and also say why.

 757 PassRefPtr<Element> Document::createElementNS(const String& _namespaceURI, const String& qualifiedName, ExceptionCode& ec)

Given the other changes you made, I'm surprised you didn't take the spurious underscore off of _namespaceURI here.

 761         // qualifiedName.isEmpty() also hits this path (not NAMESPACE_ERR), see:
 762         // dom/html/level1/core/hc_documentinvalidcharacterexceptioncreateattribute1.html
 763         // and http://www.w3.org/Bugs/Public/show_bug.cgi?id=525

I don't think this is a helpful comment. It's a great code change! But we already have a test case, and I don't really think we need a pointer to that bug. If we do want that bug URL somewhere, I suggest putting it in the test case rather than putting it in two places in the code.

 3447     // Spec: DOM Level 2 Core:  createAttributeNS(null, "xmlns") should throw
 3448     // http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-DocCrAttrN
 3449     if (qName.localName() == "xmlns" && qName.namespaceURI() != "http://www.w3.org/2000/xmlns/") {

You left an "S" off the URL here, so the URL didn't work. Please fix that.

I also don't think the example in the comment is clear enough since it's a specific case involving null and doesn't match the much broader test in the code, although the code does match the spec. It'd probably be better to just omit the example.

The code you added is only for xmlns, but the section you site in the DOM Level 2 document also mentions a couple other rules that don't seem to be implemented here. The one for xml and the one about a prefix without a namespace. The test cases seem to cover these, but I don't see why it works. Shouldn't you use the whole hasNamespaceError function here?

Even though everything looks good, there are enough comments here that I'll say review-.
Comment 9 Eric Seidel (no email) 2008-01-13 15:41:11 PST
The FireFox dudes have created another test case:
http://web.mit.edu/jwalden/www/ce.html
Comment 10 Jeff Walden (remove +bwo to email) 2008-01-13 16:19:50 PST
Created attachment 18427 [details]
Gecko tests, some with explanations
Comment 11 Jeff Walden (remove +bwo to email) 2008-01-13 16:59:29 PST
Created attachment 18428 [details]
Gecko tests, with bugs fixed

Sorry, I had a misunderstanding about colons in XML names -- this is better, especially since passing the previous would have meant failing acid3.  :-)
Comment 12 Eric Seidel (no email) 2008-01-29 00:18:29 PST
I still have this patch in my tree.  I'll see if I can get this polished off an submitted this week.
Comment 13 Eric Seidel (no email) 2008-02-01 18:39:37 PST
Created attachment 18866 [details]
Address Darin's comments and update tests to match mozilla

 .../createAttributeNS-namespace-err-expected.txt   |   70 +++++---
 .../createElementNS-namespace-err-expected.txt     |   68 +++++---
 .../resources/createAttributeNS-namespace-err.js   |  190 ++++++++++++--------
 .../resources/createElementNS-namespace-err.js     |  188 ++++++++++++--------
 WebCore/dom/Document.cpp                           |   32 ++--
 5 files changed, 331 insertions(+), 217 deletions(-)
Comment 14 Eric Seidel (no email) 2008-02-01 18:40:48 PST
Comment on attachment 18866 [details]
Address Darin's comments and update tests to match mozilla

This includes darin's suggested fixes, as well as updated tests.  We don't pass all of the updated moz tests (and I haven't spent any serious effort to find out why).  I'm not sure passing the moz tests is worth much effort however, given how obscure a section of the spec this is.
Comment 15 Eric Seidel (no email) 2008-02-03 05:12:22 PST
"blocking" bug 16693, since some of the functions added by this patch should be reused there.  I'd like to land this (even though we "fail" some of moz's new tests -- we throw different exception numbers in a couple cases).  I can file a follow-up bug for those cases.  It would be hard to imagine any number of websites depending on such an obscure section of the DOM as "which exception is thrown for this particular flavor of invalid qualified name" -- hence my desire to get this landed and move on.
Comment 16 Eric Seidel (no email) 2008-02-03 05:13:54 PST
Created attachment 18882 [details]
Squashed patch for final review

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |   51 ++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 ++++++++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  134 +++++++++++++++++++
 WebCore/dom/Document.cpp                           |   77 ++++++++---
 WebCore/dom/Document.idl                           |    4 +-
 10 files changed, 470 insertions(+), 25 deletions(-)
Comment 17 Darin Adler 2008-02-03 13:21:34 PST
Comment on attachment 18882 [details]
Squashed patch for final review

+description("createAttirbuteNS tests adapted from createElementNS tests attached to webkit bug 16833");

Typo. Should be "createAttributeNS".

r=me
Comment 18 Eric Seidel (no email) 2008-02-03 15:50:01 PST
Fixed and landed in r29952.
Comment 19 Kevin McCullough 2008-02-06 12:28:28 PST
Unfortunately this fix caused a regression in Yahoo! mail.  Passing Acid 3 is important but we need to not break a real-world high-priority site, so we are going to roll this change out.

1. Login to the Yahoo! mail beta.
2. Click the New button on the top left to make a new message.
3. Fill in the To field and Subject field.
4. From the Subject field hit Tab.  In previous revisions the focus and cursor would go to the Body.
5. Fill in the body and hit the Send button.

It will take a long time to send and then fail saying "Oops! We are having a problem sending your message."
Comment 20 Jeff Walden (remove +bwo to email) 2008-02-06 13:20:37 PST
At a glance the problem is one of these three, assuming I didn't mess up line numbers or anything:

this.A.setAttribute("xmlns:xsi",yC.O+"-instance");
this.A.setAttribute("xmlns:xsd",yC.O);
this.A.setAttribute(yC.L+"encodingStyle",yC.q+"/encoding/");
this.n.createElement(yC.L+"Body");

yC looks a few lines up to be defined (with reformatting, natch) as:

  {
    q: "http://schemas.xmlsoap.org/soap",
    O: "http://www.w3.org/1999/XMLSchema",
    L: "SOAP-ENV:",
    B: "srcWindow",
    s: "urn:yahoo:ymws",
    n: function(){},
    u: function(){}
  }

So I'm guessing it's most likely the last one, although looking at the patch I don't quite know why.
Comment 21 Eric Seidel (no email) 2008-02-06 14:21:27 PST
OK, this is a real regression.

The problem is this check:

    if (!qName.prefix().isEmpty() && qName.namespaceURI().isNull()) // createElementNS(null, "html:div")
        return true;

fails for document.createElement("foo:bar"), because we send createElement down the createElementNS path (which is wrong).

So this patch is wrong (due to existing code being "wrong").
Comment 22 Ian 'Hixie' Hickson 2008-02-06 17:40:08 PST
Good lord what is Yahoo doing here. That's insane code.
Comment 23 Darin Adler 2008-02-06 17:47:15 PST
Comment on attachment 18882 [details]
Squashed patch for final review

Clearing review flag since this bug was reopened.
Comment 24 Eric Seidel (no email) 2008-02-08 01:08:15 PST
I do not wish to hold the mutex on this bug, I'm not working on it actively at the moment.

What remains to be done:
1.  create HTML and XHTML test cases for createElement (and test against other browsers to see what exceptions we should throw)
2.  make createElement stop calling createElementNS under the covers.
Comment 25 Darin Adler 2008-02-10 13:35:21 PST
Comment on attachment 18882 [details]
Squashed patch for final review

Marking this review- to make it clear that we need a new patch that doesn't break Yahoo. I didn't want to mark this obsolete since it's the latest and greatest we have so far.
Comment 26 Jeff Walden (remove +bwo to email) 2008-02-12 06:22:48 PST
I added a bunch of tests for createElement to complement the createElementNS ones:

http://mxr.mozilla.org/mozilla/source/dom/tests/mochitest/bugs/test_bug411103.html?raw=1
Comment 27 Darin Adler 2008-03-15 18:26:27 PDT
I merged in Jeff's additional test, but since none of those tests fails with the patch applied, clearly none exactly covers the problem that affected Yahoo.
Comment 28 Darin Adler 2008-03-15 18:28:02 PDT
Created attachment 19785 [details]
patch rebased with addition of more tests from Gecko tree
Comment 29 Eric Seidel (no email) 2008-03-21 14:45:43 PDT
Created attachment 19938 [details]
Fix xml/xhtml handling of createElement and add tests

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |  138 +++++++++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 +++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  180 ++++++++++++++++++++
 WebCore/dom/Document.cpp                           |   89 +++++++---
 WebCore/dom/Document.idl                           |    4 +-
 10 files changed, 609 insertions(+), 31 deletions(-)
Comment 30 Eric Seidel (no email) 2008-03-21 15:37:48 PDT
Comment on attachment 19938 [details]
Fix xml/xhtml handling of createElement and add tests

Oops, turns out there was an error in the test harness.
Comment 31 Eric Seidel (no email) 2008-03-21 15:38:21 PDT
Created attachment 19940 [details]
additional patch needed to fix the test and code
Comment 32 Eric Seidel (no email) 2008-03-21 15:39:33 PDT
Created attachment 19941 [details]
Fix xml/xhtml handling of createElement and add tests

xml/xhtml createElement fixed, now with test harness fixed too
---
 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |  138 +++++++++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 +++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  180 ++++++++++++++++++++
 WebCore/dom/Document.cpp                           |   89 +++++++---
 WebCore/dom/Document.idl                           |    6 +-
 10 files changed, 610 insertions(+), 32 deletions(-)
Comment 33 Maciej Stachowiak 2008-03-21 22:20:02 PDT
Comment on attachment 19941 [details]
Fix xml/xhtml handling of createElement and add tests

r=me
Comment 34 Eric Seidel (no email) 2008-03-22 02:51:25 PDT
r31230
Comment 35 Lucas Forschler 2019-02-06 09:04:01 PST
Mass moving XML DOM bugs to the "DOM" Component.