Bug 30232 - [HTML5] Add document.head
Summary: [HTML5] Add document.head
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-08 12:21 PDT by Joseph Pecoraro
Modified: 2009-10-15 00:39 PDT (History)
2 users (show)

See Also:


Attachments
[PATCH] Adds document.head and Tests (3.79 KB, patch)
2009-10-08 12:30 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Adds document.head and Tests (improved) (3.79 KB, patch)
2009-10-08 12:37 PDT, Joseph Pecoraro
eric: review-
Details | Formatted Diff | Diff
[PATCH] Add to HTMLDocument.idl and Newer Style Test (4.31 KB, patch)
2009-10-08 13:48 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] document.head in Document.idl (3.53 KB, patch)
2009-10-08 22:46 PDT, Joseph Pecoraro
abarth: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2009-10-08 12:21:11 PDT
document.head is now in the HTML5 draft. It is in the Document's IDL:

Document IDL:
http://www.whatwg.org/specs/web-apps/current-work/#documents-in-the-dom

More Info on head accessor:
http://www.whatwg.org/specs/web-apps/current-work/#dom-tree-accessors
Comment 1 Joseph Pecoraro 2009-10-08 12:30:49 PDT
Created attachment 40901 [details]
[PATCH] Adds document.head and Tests
Comment 2 Joseph Pecoraro 2009-10-08 12:37:29 PDT
Created attachment 40902 [details]
[PATCH] Adds document.head and Tests (improved)

I made the equality tests stronger and fixed a style issue in the test case.
Comment 3 Eric Seidel (no email) 2009-10-08 12:46:00 PDT
Comment on attachment 40902 [details]
[PATCH] Adds document.head and Tests (improved)

Seems wrong.  This already exists on HTMLDocument.idl.  Seems that we would either want to remove the one on HTMLDocument.idl if we did this, or that we don't want to do this at all.

The real change here is not for HTML docuemtns, but for XHTML documents! :)  Also, this would add .head for SVG documents.
Comment 4 Darin Adler 2009-10-08 13:38:50 PDT
Let me say what Eric said, a little more emphatically.

The test attached to this bug already passes. Did you try it without your patch?

Making all of the HTMLDocument interface available to XML documents is something I think HTML 5 does call for in some fashion. I don't think it's urgent to move the head property from HTMLDocument  to Document as a separate step.

It's nice to have this test and we should land it, but *not* the code change.
Comment 5 Joseph Pecoraro 2009-10-08 13:47:11 PDT
(In reply to comment #4)
> Let me say what Eric said, a little more emphatically.
> 
> The test attached to this bug already passes. Did you try it without your
> patch?

Yes I did test.  It fails in the current nightly! document.head is always undefined.  Eric was mistaken, it is not in HTMLDocument.idl.

> Making all of the HTMLDocument interface available to XML documents is
> something I think HTML 5 does call for in some fashion. I don't think it's
> urgent to move the head property from HTMLDocument  to Document as a separate
> step.

I see that I should have put it on HTMLDocument and not Document.  That is fixed in the new patch.  Eric also linked me to some newer resources on creating tests and I've attempted the newer style.
Comment 6 Joseph Pecoraro 2009-10-08 13:48:36 PDT
Created attachment 40909 [details]
[PATCH] Add to HTMLDocument.idl and Newer Style Test

- HTMLDocument.idl not Document.idl
- Newer Style test with script-tests
Comment 7 Darin Adler 2009-10-08 13:51:48 PDT
(In reply to comment #5)
> I see that I should have put it on HTMLDocument and not Document.

I'm not sure that's correct. I think perhaps it should be on Document. Is there a downside to having the head property be exposed to JavaScript in XML and SVG documents? If not, then you were right to have it in Document.
Comment 8 Eric Seidel (no email) 2009-10-08 13:57:09 PDT
Comment on attachment 40909 [details]
[PATCH] Add to HTMLDocument.idl and Newer Style Test

OK, it would appear that document.head does not actually exist.  I'm surprised by this.  I thought it did.

I don't think this check is needed:
 var head = document.head;
 4 if (head)
 5     testPassed("document.head");

Given that document.body is defined in Document.idl instead of HTMLDocument.idl, I'm not sure that I was correct in telling you to move this to HTMLDocument.idl.
Comment 9 Joseph Pecoraro 2009-10-08 14:34:54 PDT
> I'm not sure that's correct. I think perhaps it should be on Document. Is there
> a downside to having the head property be exposed to JavaScript in XML and SVG
> documents? If not, then you were right to have it in Document.

In order to track this down I checked to see if there was a downside to having the body property be exposed to JavaScript in XML and SVG.

From the Internet Side of Things:

This article (2005) seems to indicate that the web developer at the time did not expect document.body to appear in an XML or SVG Document and would fail in WebKit?
http://userjs.org/help/tips_and_tricks/what-type-of-page

Someone on the XML/SVG mailing list (2008) wrote that document.body was not a part of the SVG DOM:
http://article.gmane.org/gmane.text.xml.svg.devel/43755

document.body is not in the SVG 1.1 or SVG Tiny 1.2 Specs (under ECMAScript Bindings):
http://www.w3.org/TR/SVGTiny12/ecmascript-binding.html

However, on the WebKit side of things:

document.body was moved from HTMLDocument.idl to Document.idl in ChangeSet 26078 (2007):
http://trac.webkit.org/changeset/26078

The ChangeLog states:
> Moved a bunch of methods and properties from
> HTMLDocument down into Document to make
> them available for all documents (xml, svg).

Maciej Stachowiak on bug 12628 (2007):
> We should consider supporting all HTMLDocument properties and
> methods on all XML documents (because any XML document could
> contain XHTML).


I see no harm in putting the body and head properties on XML and SVG documents. Especially if document.body has been that way for over two years in WebKit without complaints.  Anyone know how Firefox handles this?
Comment 10 Joseph Pecoraro 2009-10-08 15:08:08 PDT
> Anyone know how Firefox handles this?

On an SVG Document:
http://www.w3schools.com/svg/rect2.svg
http://grab.by/8sn

Firefox 3.5:
- document.body is undefined
- document.head is undefined (not supported)

WebKit Nightly:
- document.body is null (like the HTML5 spec says for HTML)
- document.head is undefined (as we are discussing)
Comment 11 Joseph Pecoraro 2009-10-08 20:23:48 PDT
Should I CC someone in on this for a final decision?

(In reply to comment #8)
> I don't think this check is needed:
>    var head = document.head;
>    if (head)
>         testPassed("document.head");

You're right. I'll remove that in a new patch when a decision is made.
Comment 12 Joseph Pecoraro 2009-10-08 22:27:40 PDT
Here is a clip from an IRC chat with othermaciej:

> that's a complicated question in theory, the absolutely correct thing
> is for it to go on HTMLDocument, and for us to make every single
> document implement all Document interfaces (including both
> HTMLDocument and SVGDocument) in practice, we don't have a
> way to do the "all interfaces on all documents" thing yet. thus, my
> suggestion would be that you should temporarily put it wherever
> document.body currently lives which happens to be Document
>
> though in the long term it should be on HTMLDocument and we
> should figure out how to do the HTML5 thing.

I agree with his suggestion. It would be easier to work with the attributes later if they are in the same place.
Comment 13 Joseph Pecoraro 2009-10-08 22:46:38 PDT
Created attachment 40930 [details]
[PATCH] document.head in Document.idl

- Moved back to Document.idl.
- Removed the extra check in the script-test.
Comment 14 Joseph Pecoraro 2009-10-08 22:48:20 PDT
Comment on attachment 40930 [details]
[PATCH] document.head in Document.idl

The ChangeLog's diff would make this fail in the commit-queue anyways. So it would need to be landed manually.
Comment 15 Adam Barth 2009-10-14 23:35:42 PDT
Comment on attachment 40930 [details]
[PATCH] document.head in Document.idl

Based on the discussion on this bug and Maciej's quote from IRC, this patch appears correct.  Thanks!
Comment 16 Joseph Pecoraro 2009-10-15 00:39:51 PDT
Landed in http://trac.webkit.org/changeset/49610
r49610 = a7e5c8e75b5404e71a0a7a6740f2b235f0b338f6

I had to provide a Windows Build Fix: r49611
Landed in http://trac.webkit.org/changeset/49611
Modeled after a previous Windows Build Fix for:
http://trac.webkit.org/changeset/48728

It appears as though there was an issue with Window's Code Generation.  Could someone enlighten me on why this was necessary, and if this is a problem could you point me in the right direction for getting this fixed?  Is there someone who knows a lot about Window's code generation?

The error I was received was:
http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/6292/steps/compile-webkit/logs/stdio
GEN_DOMHTMLEmbedElement.cpp
..\DOMCreateInstance.cpp(207) : error C2259: 'GEN_DOMHTMLDocument' : cannot instantiate abstract class
        due to following members:
        'HRESULT IGEN_DOMDocument::head(IGEN_DOMHTMLHeadElement **)' : is abstract
        C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Include\WebKit/WebKit.h(61160) : see declaration of 'IGEN_DOMDocument::head'

After adding the build fix Windows passed all the Layout tests, including my newly added test, which confused me, but maybe that is how it should be?

Sorry about all this for a one line fix!