351217 - getElementById return null for elements overlayed into pref-mailnews.xul

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Davide Ficano]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060729 SeaMonkey/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060729 SeaMonkey/1.0.4

I added an id-less <vbox> containing a checkbox (with id) child into pref-mailnews.xul. Then I try to obtain the checkbox element using getElementById but it is always null. Adding an onclick event to checkbox that print document.getElementById(this.id)) I obtain always null.

Reproducible: Always

Steps to Reproduce:
1. chrome://messenger/content/pref-mailnews.xul
2. open preferences click on 'Mail & Newsgroup'
3. click on 'Click me' checkbox

Actual Results:  
The alert message based on my testcase show
null --- null

The first null refers to a document.getElementById that receive the id as string
The second null refers to document.getElementById(this.id)
this.id is correctly set

Expected Results:  
The string indicating the object

Comment 1

[Davide Ficano]

Attachment: The whole xul file used to overlay chrome://messenger/content/pref-mailnews.xul

Using this file you can reproduce the bug

Comment 2

[[e-mail]]

Attachment: Proposed patch

It looks like that that code path forgets to update the element map.

Comment 3

[[e-mail]]

Comment on attachment 236689 [details] [diff] [review]
Proposed patch

>+        rv = mDocument->InsertElement(mDocument->mRootContent, mOverlay, notify);
>+        if (NS_FAILED(rv)) return eResolve_Error;
>+
>+        // Add child and any descendants to the element map
>+        rv = mDocument->AddSubtreeToDocument(target);
Whoops, this should be mOverlay of course... Fixed locally.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236689 [details] [diff] [review]
Proposed patch

>Index: nsXULDocument.cpp
> nsXULDocument::OverlayForwardReference::Resolve()
>+        // Add child and any descendants to the element map
>+        rv = mDocument->AddSubtreeToDocument(target);

Won't this call AddSubtreeToDocument twice when |notify| is true?

>     rv = mDocument->AddSubtreeToDocument(target);

For reference that looks pretty wrong to me, since it calls AddSubtreeToDocument multiple times on the same node.... Doesn't really affect this patch, except for the |notify| thing.

Comment 5

[[e-mail]]

Attachment: Fixed notify usage

Asking jst for review as he was one of the two reviewers who let the double-notify though in the first place and I'm unlikely to get a review from the original author.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 237106 [details] [diff] [review]
Fixed notify usage

r=jst, looks right to me.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237106 [details] [diff] [review]
Fixed notify usage

sr=bzbarsky

Comment 8

[[e-mail]]

Fix checked in to the trunk.