Bug 8563 – [DOMHTMLDocument doctype] always returning nil.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Matt Gough]

Sometime since 418, the implementation of HTMLDocument::doctype was changed to:

DocumentType *HTMLDocument::doctype() const
{
    // According to a comment in dom_doc.cpp, doctype is null for HTML documents.
    return 0;
}


Firstly, where is dom_doc.cpp? It doesn't seem a part of WebKit/Core.

What is the comment and why is it deemed correct that doctype should be null for HTML documents?

Why do I care:

Well, I am writing some code to take a DOM (or portions thereof) and convert it back into a fairly self-contained HTML file. Without being able to reconstitute the correct <!DOCTYPE ...> I have no way of ensuring that quirks mode is set correctly when displaying the resulting html.

[Matt Gough]

Created attachment 7938 [details]
Accessing the doctype when there is one declared

The following Mac browsers 'pass' this test -Firefox 1.5.0.1, Opera 8.54.2200 and Internet Explorer 5.2.3.
WebKit-based ones fail it.

[Matt Gough]

Created attachment 7939 [details]
Accessing the doctype when there is not one declared

All browser passed this test, including WebKit-based ones

[Matt Gough]

My original assertion that this bug did not affect released versions of WebKit is wrong. It fails in 417.9.2

[Matt Gough]

Ignore the 417.9.2 version mentioned above. It should have been 418.

[Matt Gough]

Created attachment 7940 [details]
Patch for this bug

This patch:
1. Removes DocumentType *HTMLDocument::doctype() const from HTMLDocument.h and cpp.
2. Changes the comment in Document.h to say that docType() may return 0.
3. Changes LayoutTests/dom/html/level1/core/hc_documentgetdoctype.js to do a case insensitive compare of docTypeName against 'html' (Since HTML is the name specified in HTMLDocument::determineParseMode when creating the docType for HTML.) Not sure whether this is the correct approach, or to just make HTMLDocument::determineParseMode use 'html'

[Alexey Proskuryakov]

WinIE 6 fails the test :-(

"FAILED - doctype is null even though there is a <!DOCTYPE ...> declaration 

[Darin Adler]

Comment on attachment 7940 [details]
Patch for this bug

We normally do not make changes to tests from the DOM working group without making comments describing what the change is and why we made it, and also reporting the bug in the W3C Bugzilla. So I'm a bit uncomfortable with the change in hc_documentgetdoctype.js.

[Matt Gough]

(In reply to comment #7)
> So I'm a bit uncomfortable with the change in hc_documentgetdoctype.js.
> 

As it stood before my patch that code (in the test) would never have been called anyway (in WebKit)

The problem is caused by the test html file using 'html' but WebKit hard-coding a 'HTML' for the docType. Conceptually, the test is consistent with itself and so should probably not have been changed.

Do you have any objections to passing in a lowercase 'html' to new DocumentType in HTMLDocument::determineParseMode instead:
     setDocType(new DocumentType(this, "html", publicID, systemID));

An even better fix, but one which is far more trouble than it is worth, would be to pass the same version of the name as read in from the <!Doctype ...>, but WebKit has already lost that information by the time the setDocType call is made.

Since html is inherently case-insensitive anyway I see no issue with using 'html' instead and not changing the test.

[Darin Adler]

Comment on attachment 7940 [details]
Patch for this bug

(In reply to comment #8)
> Do you have any objections to passing in a lowercase 'html' to new DocumentType
> in HTMLDocument::determineParseMode instead:
>      setDocType(new DocumentType(this, "html", publicID, systemID));

Sounds fine. We should do this.

> An even better fix, but one which is far more trouble than it is worth, would
> be to pass the same version of the name as read in from the <!Doctype ...>, but
> WebKit has already lost that information by the time the setDocType call is
> made.

I'm not sure why you say this is more trouble than it's worth, though.

We could easily add a fourth return value to the function "parseDocTypeDeclaration" in HTMLDocument.cpp to complement the resultFlags, publicID, and systemID return values.

I think the real issue is not the difficulty of implementation, but the behavior of other browsers. Do those browsers always return the "html" in the same case it was in the <!doctype> or do they always return lowercase "html"? We should match them. If they always return lowercase "html", then this is not a better fix after all.

[Matt Gough]

Created attachment 7981 [details]
Improved Patch

FireFox and Opera both return the docType.name it as it is in the source (i,e HTML, html, Html, hTMl etc). As such this new patch incorporates Darin's suggestion of returning the name out of parseDocTypeDeclaration. This name is then passed in when creating the DocumentType in determineParseMode.

[Darin Adler]

Comment on attachment 7981 [details]
Improved Patch

Looks great! Just needs a change log and a test case, and then it's ready to land.

Marking review- because of the lack of change log and test case, otherwise would be review+.

[Matt Gough]

> Marking review- because of the lack of change log and test case, otherwise
> would be review+.

I couldn't decide if the existing doctype test (hc_documentgetdoctype.html) was enough or not. Should I do one along the lines of the first attachment above?

Dumb as it sounds, but how do I provide a change log? Am I supposed to actually edit the Changelog file and create a patch from that, or just add a comment here? Unfortunately http://webkit.opendarwin.org/coding/contributing.html is a little ambiguous: 'Make sure that your patch includes a ChangeLog entry'

[Alexey Proskuryakov]

When the behavior changes, some test should change from failure to success - if I understood correctly, hc_documentgetdoctype.html doesn't catch this change? I think it would also be useful to test for different capitalizations that you have mentioned (hTMl etc).

Yes, ideally, a patch to ChangeLog should be included in the patch itself. There's a prepare-Changelog script that creates a template (automatically mentioning all modified files and functions!). I run it once in each modified top-level subdirectory, that is, WebCore, LayoutTests etc.

[Matt Gough]

Created attachment 8063 [details]
Patch

Now with ChangeLog entries.

Test files to appear shorty
(Note - I would have though that svn-create-patch would also add my tests to the patch but it didn't. Is it supposed to?)

[Matt Gough]

Created attachment 8064 [details]
Layout test

into fast/doctypes/

[Matt Gough]

Created attachment 8065 [details]
Layout test expected result

into fast/doctypes

(In reply to comment #14)
> (Note - I would have though that svn-create-patch would also add my tests to
> the patch but it didn't. Is it supposed to?)

Yes, but you have to "svn add" them.

[Matt Gough]

> > (Note - I would have though that svn-create-patch would also add my tests to
> > the patch but it didn't. Is it supposed to?)
> 
> Yes, but you have to "svn add" them.

I was assuming that since I am not a committer that I would not be allowed to touch svn.

http://webkit.opendarwin.org/coding/contributing.html doesn't mention what to do with new files.

[Darin Adler]

Comment on attachment 8063 [details]
Patch

r=me.

Note that the new test case is not in the patch and must be "svn add"-ed separately.

[Darin Adler]

(In reply to comment #18)
> http://webkit.opendarwin.org/coding/contributing.html doesn't mention what to
> do with new files.

A patch for the website to correct this would be great!

[Matt Gough]

Created attachment 8090 [details]
Patch including test files

[Eric Seidel (no email)]

Comment on attachment 8090 [details]
Patch including test files

This looks great.  I don't know why the old code worked that way.  Assuming you tested in FireFox and IE to make sure that our behavior agrees with theirs, r=me.

Also, we would need to clean up the changelog conflict markers before landing.

[Matt Gough]

> Assuming you
> tested in FireFox and IE to make sure that our behavior agrees with theirs,
> r=me.

I tested on Opera and Firefox on Mac. No PC so can't check IE

> Also, we would need to clean up the changelog conflict markers before landing.

Does 'we' mean someone else, or do I have to go through the whole patch making rigmarole again :(

[Alexey Proskuryakov]

(In reply to comment #23)
> > Assuming you
> > tested in FireFox and IE to make sure that our behavior agrees with theirs,
> > r=me.

  To reiterate comment 6, WinIE behavior is different. Not sure if this can be considered already OK'd by Darin.

> > Also, we would need to clean up the changelog conflict markers before landing.
> 
> Does 'we' mean someone else, or do I have to go through the whole patch making
> rigmarole again :(

  I'm pretty sure "we" means "the committer" :)

[Eric Seidel (no email)]

Comment on attachment 8090 [details]
Patch including test files

I didn't read the whole history, sorry.  I'm going to leave this one to darin, since he's been working with you on this one.

[Darin Adler]

Comment on attachment 8090 [details]
Patch including test files

Patch looks fine. Yes the committer will have to remove the conflict marker from LayoutTests/ChangeLog.

r=me

[Matt Gough]

(In reply to comment #17)
> (In reply to comment #14)
> > (Note - I would have though that svn-create-patch would also add my tests to
> > the patch but it didn't. Is it supposed to?)
> 
> Yes, but you have to "svn add" them.
> 
OK, so now that my patch has landed update-webkit failed with:
svn: Failed to add file 'LayoutTests/fast/doctypes/005-case-preserving.html': object of the same name already exists

so I removed it and I get:

svn: Failed to add file 'LayoutTests/fast/doctypes/005-case-preserving.html': object of the same name is already scheduled for addition

Once I know what to do I'll be able to update the 'Contributing Code' part of the Website with all the info that I wished I'd known at the start.

[Alexey Proskuryakov]

(In reply to comment #27)

You can either use the svn-unapply script to rollback your patch locally before updating, or just revert to the base revision (if you have no other local modifications):

svn revert LayoutTests -R