319659 - XPFE code assumes document.firstChild is the root element

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Jason Barnabe (np)]

Bug 319654 is about adding processing instructions in XUL to the DOM tree. When
that happens, any code that thinks document.firstChild is the document element
will break. We need to change it to document.documentElement.

Comment 1

[[e-mail]]

/editor/ /mailnews/ and /xpfe/ count as Suite for the purposes of this bug.

/extensions/irc/js/lib/command-manager.js, line 259 -- document.firstChild.appendChild (parentElem);
IRC may want its own bug filed for this.

/extensions/p3p/resources/content/p3pSummary.js, line 92 -- while (aResultDocument.firstChild &&
/extensions/p3p/resources/content/p3pSummary.js, line 93 -- aResultDocument.firstChild != docElement) {
/extensions/p3p/resources/content/p3pSummary.js, line 94 -- aResultDocument.removeChild(aResultDocument.firstChild);
This is an XML document, not a XUL document. Not part of the bug.

/extensions/venkman/resources/content/command-manager.js, line 235 -- document.firstChild.appendChild (parentElem);
Venkman may want its own bug filed for this.

/layout/html/tests/block/bugs/4324.html, line 15 -- var body=document.firstChild.childNodes.item(1);
/layout/html/tests/table/bugs/bug13526.html, line 11 -- var oList=document.firstChild.childNodes[1].childNodes[1];
These HTML tests should probably use document.body - the first one looks wrong, although the second one might work; again, layout will want its own bug filed.

/calendar/resources/content/importExport.js, line 1645 -- // xcsDocument.insertBefore(processingInstruction, xcsDocument.firstChild);
/calendar/sunbird/base/content/customizeToolbar.xul, line 66 -- document.insertBefore(newPI, document.firstChild);
Wow, someone using document.firstChild correctly ;-)

/parser/htmlparser/tests/html/18308.html, line 9 -- var oWhat=document.firstChild.childNodes[i];
Again, this code doesn't work, I guess someone needs to file a bug on parser.

Comment 2

[Jason Barnabe (np)]

(In reply to comment #1)
> IRC may want its own bug filed for this.
Bug 319821

> This is an XML document, not a XUL document. Not part of the bug.
Plus it looks like a valid usage.

> Venkman may want its own bug filed for this.
Bug 319822

> These HTML tests should probably use document.body - the first one looks wrong,
> although the second one might work; again, layout will want its own bug filed.
Bug 319824

> Again, this code doesn't work, I guess someone needs to file a bug on parser.
Bug 319826

Comment 3

[[e-mail]]

(In reply to comment #2)
That was quick work, two fixed already :-)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

seems unlikely that Bug 319654 will end up on any branches, why do we need this for 1.0?

Comment 5

[[e-mail]]

(In reply to comment #4)
>seems unlikely that Bug 319654 will end up on any branches, why do we need this for 1.0?
Because that bug only applies to loading documents, not to mutating existing documents, which already breaks any code making the assumption.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, good point.

Comment 7

[Ian Neal]

*** Bug 322520 has been marked as a duplicate of this bug. ***

Comment 8

[Ian Neal]

Attachment: Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

Changes document.firstChild to document.documentElement

Comment 9

[Jason Barnabe (np)]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

>-      var skipBlockQuotes = (window.document.firstChild.getAttribute("windowtype") == "msgcompose");
>+      var skipBlockQuotes = (window.document.documentElement.getAttribute("windowtype") == "msgcompose");
You could probably just drop the "window" from this.

Comment 10

[Ian Neal]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

Checking in (trunk)
editor/ui/composer/content/ComposerCommands.js;
new revision: 1.203; previous revision: 1.202
editor/ui/composer/content/editorApplicationOverlay.js;
new revision: 1.22; previous revision: 1.21
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.229; previous revision: 1.228
mailnews/extensions/smime/resources/content/msgReadSMIMEOverlay.js;
new revision: 1.4; previous revision: 1.3
xpfe/communicator/resources/content/contentAreaDD.js;
new revision: 1.36; previous revision: 1.35
xpfe/communicator/resources/content/contentAreaUtils.js;
new revision: 1.140; previous revision: 1.139
xpfe/components/sidebar/resources/sidebarOverlay.js;
new revision: 1.127; previous revision: 1.126
xpfe/global/resources/content/charsetOverlay.js;
new revision: 1.43; previous revision: 1.42
done

Comment 11

[Ian Neal]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

Checking in (branch 1.8)
editor/ui/composer/content/ComposerCommands.js;
new revision: 1.201.4.2; previous revision: 1.201.4.1
editor/ui/composer/content/editorApplicationOverlay.js;
new revision: 1.21.24.1; previous revision: 1.21
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.222.2.6; previous revision: 1.222.2.5
mailnews/extensions/smime/resources/content/msgReadSMIMEOverlay.js;
new revision: 1.3.126.1; previous revision: 1.3
xpfe/communicator/resources/content/contentAreaDD.js;
new revision: 1.35.2.1; previous revision: 1.35
xpfe/communicator/resources/content/contentAreaUtils.js;
new revision: 1.134.2.3; previous revision: 1.134.2.2
xpfe/components/sidebar/resources/sidebarOverlay.js;
new revision: 1.125.4.1; previous revision: 1.125
xpfe/global/resources/content/charsetOverlay.js;
new revision: 1.42.2.1; previous revision: 1.42
done

Comment 12

[Ian Neal]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

The two changes under editor are shared code with TB, the changes are very low risk though.

Comment 13

[Scott MacGregor]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

I'm ok with the overlapping changes to files shared by Thunderbird here. Since this change only impacts thunderbird and the suite, I'll take the liberty of approving this patch for the seamonkey 1.0 release on the 1.8.0.x branch.

Comment 14

[Ian Neal]

Comment on attachment 207670 [details] [diff] [review]
Simple patch v0.1 (Checked in trunk & branch 1.8/1.8.0)

Checking in (branch 1.8.0)
editor/ui/composer/content/ComposerCommands.js;
new revision: 1.201.4.1.4.1; previous revision: 1.201.4.1
editor/ui/composer/content/editorApplicationOverlay.js;
new revision: 1.21.32.1; previous revision: 1.21
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.222.2.3.4.3; previous revision: 1.222.2.3.4.2
mailnews/extensions/smime/resources/content/msgReadSMIMEOverlay.js;
new revision: 1.3.134.1; previous revision: 1.3
xpfe/communicator/resources/content/contentAreaDD.js;
new revision: 1.35.10.1; previous revision: 1.35
xpfe/communicator/resources/content/contentAreaUtils.js;
new revision: 1.134.2.2.2.2; previous revision: 1.134.2.2.2.1
xpfe/components/sidebar/resources/sidebarOverlay.js;
new revision: 1.125.12.1; previous revision: 1.125
xpfe/global/resources/content/charsetOverlay.js;
new revision: 1.42.10.1; previous revision: 1.42
done

Comment 15

[Ian Neal]

Closest keyword to fixed1.8.0.2 is fixed1.8.0.1