Direct children of <overlay> not created correctly

RESOLVED FIXED in mozilla1.9alpha2

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: asqueella, Assigned: asqueella)

Tracking

Trunk
mozilla1.9alpha2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

12.06 KB, patch
asqueella
: review+
sicking
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.741#3590

3590 nsXULDocument::CreateOverlayElement(nsXULPrototypeElement* aPrototype,
3591                                     nsIContent** aResult)

3592 {
3593     nsresult rv;
3594 
3595     // This doesn't really do anything except create a placeholder
3596     // element. I'd use an XML element, but it gets its knickers in a
3597     // knot with DOM ranges when you try to remove its children.
3598     nsCOMPtr<nsIContent> element;

3599     rv = nsXULElement::Create(aPrototype, this, PR_FALSE,
3600                               getter_AddRefs(element));

3601     if (NS_FAILED(rv)) return rv;

3602 
3603     OverlayForwardReference* fwdref =
3604         new OverlayForwardReference(this, element);

3605     if (! fwdref)
3606         return NS_ERROR_OUT_OF_MEMORY;
3607 
3608     // transferring ownership to ya...
3609     rv = AddForwardReference(fwdref);
3610     if (NS_FAILED(rv)) return rv;


http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.741#3814

3800 nsForwardReference::Result
3801 nsXULDocument::OverlayForwardReference::Resolve()
...
3814 if (id.IsEmpty()) {
3815     // overlay had no id, use the root element

3816     if (!mDocument->mRootContent) {
3817         return eResolve_Error;
3818     }
3819     

3820     rv = mDocument->InsertElement(mDocument->mRootContent, mOverlay, notify);
3821     if (NS_FAILED(rv)) return eResolve_Error;
3822 
3823     if (!notify) {
3824         // Add child and any descendants to the element map
3825         rv = mDocument->AddSubtreeToDocument(mOverlay);
3826         if (NS_FAILED(rv)) return eResolve_Error;
3827     }

This causes the "placeholder" element to be inserted in the document. This breaks non-XUL elements directly underneath <overlay>, for example.
(Assignee)

Updated

12 years ago
Blocks: 123118
(Assignee)

Comment 1

12 years ago
Created attachment 249439 [details] [diff] [review]
patch
Attachment #249439 - Flags: superreview?(bugmail)
Attachment #249439 - Flags: review?(bugmail)
(Assignee)

Comment 2

12 years ago
The actual change is in nsXULDocument::CreateOverlayElement.

In nsXULDocument::OverlayForwardReference::Resolve I shuffle code a bit to avoid duplicated AddSubtreeToDocument call, and the rest of the patch is comments tweaks and tests.

Note to self: need to include the changes to reftest.list in the next diff.
Status: NEW → ASSIGNED
Attachment #249439 - Flags: review?(bugmail) → review?(Olli.Pettay)
Comment on attachment 249439 [details] [diff] [review]
patch


>+        // If we can't find the element in the document, defer the hookup
>+        // until later.
>+        if (! domtarget)
>+            return eResolve_Later;
>+
>+        target = do_QueryInterface(domtarget);
>+        NS_ASSERTION(target != nsnull, "not an nsIContent");
>+        if (! target)
>+            return eResolve_Error;


No need for the space after !

I'd write this something like

// If we can't find the element in the document, defer the hookup
// until later.
target = do_QueryInterface(domtarget);
NS_ASSERTION(!domtarget || target, "not an nsIContent");
if (!target)
    return eResolve_Later;

So removing if (! domtarget) because if domtarget but !target
something is really badly wrong and the NS_ASSERTION should be enough.

With that, r=me
Attachment #249439 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 4

12 years ago
Created attachment 251482 [details] [diff] [review]
patch v2

With review comments fixed and the reftest.list changes included (the extra entries are the tests that were forgotten earlier).

Carrying over r+ from smaug.
Attachment #249439 - Attachment is obsolete: true
Attachment #251482 - Flags: superreview?(bugmail)
Attachment #251482 - Flags: review+
Attachment #249439 - Flags: superreview?(bugmail)
Comment on attachment 251482 [details] [diff] [review]
patch v2

>Index: content/xul/document/src/nsXULDocument.cpp

> nsXULDocument::OverlayForwardReference::Resolve()
...
>+    } else {

Please follow the whitespace style of the file:

}
else {

sr=sicking with that.
Attachment #251482 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 6

12 years ago
content/xul/document/src/nsXULDocument.cpp  1.746
content/xul/document/src/nsXULDocument.h    1.190
layout/reftests/reftest.list                1.30
layout/reftests/xul-document-load/readme.txt 1.3
layout/reftests/xul-document-load/test016-overlay.xul 1.1
layout/reftests/xul-document-load/test016.xul         1.1
layout/reftests/xul-document-load/test021-overlay.xul 1.1
layout/reftests/xul-document-load/test021.xul         1.1
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9alpha2
You need to log in before you can comment on or make changes to this bug.