602794 - nsPresContext::IsRootContentDocument is wrong for the embedding situation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 481767 [details] [diff] [review]
patch

This actually causes svg-as-image reftest failures.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do you know why yet?

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: breaktest

Here's a reduced version of the reftest failures.  This has an <img> with "src" pointing to a transparent (empty) SVG file and with a dashed border, positioned inside of a div with a light green background.

m-c rendering:         <img> region is lightgreen
m-c + patch rendering: <img> region is white

So, it looks like this somehow makes transparent regions of the SVG paint as if they were white.

Comment 5

[Daniel Holbert [:dholbert]]

I think the code in play might be PresShell::UpdateCanvasBackground(), which calls

which has:
>    if (GetPresContext()->IsRootContentDocument() &&
>        !IsTransparentContainerElement(mPresContext)) {
>      mCanvasBackgroundColor =
>        NS_ComposeColors(mPresContext->DefaultBackgroundColor(), mCanvasBackgroundColor);
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5920

Without this patch, we skip that clause (for our helper SVG document).  With the patch, we enter it. (The patch makes us return true from IsRootContentDocument(), via the first clause that the patch touches, labeled "anonymous inner view")

Perhaps we'd like to return TRUE from IsTransparentContainerElement...?  Right now that method returns false from its first return statement:
>   nsCOMPtr<nsPIDOMWindow> pwin(do_GetInterface(docShellItem));
>   if (!pwin)
>     return PR_FALSE;
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5891... but we could add a check like:
> if (aPresContext->Document()->IsBeingUsedAsImage())
>   return PR_TRUE;
... at the beginning of that method, and I think that should get us to skip the "draw default background color" clause from the beginning of this bugzilla-comment.  It looks like that's the only use of IsTransparentContainerElement(), so I don't think there would be any unexpected side-effects.

Does that sound reasonable?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> I think the code in play might be PresShell::UpdateCanvasBackground(), which
> calls
> 
> which has:
sorry -- s/which calls\n\n which has/which has/

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: followup

(In reply to comment #5)
> > if (aPresContext->Document()->IsBeingUsedAsImage())
> >   return PR_TRUE;

Here's that as a patch.  I confirmed that this fixes the breaktest here & the svg-as-image reftests.

tn, feel free to merge this into this bug's patch if that makes things easier.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: followup

Comment 9

[Boris Zbarsky [:bzbarsky]]

It might make more sense to do that check in IsRootContentDocument...

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 481868 [details] [diff] [review]
followup

Ah, right -- I'd misunderstood what IsRootContentDocument was achieving.

So tn's patch just needs an mDocument->IsBeingUsedAsImage() check near the beginning of the IsRootContentDocument() impl.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Or just check for a resource doc in general?

Comment 12

[Daniel Holbert [:dholbert]]

Yeah, that makes sense correctness-wise. (I'm not sure if we could ever hit that for an ExternalResource document -- could we? If so, we should add tests for that -- but if we did, it clearly wouldn't be the root content document.)

So, s/IsBeingUsedAsImage/IsResourceDoc/ in comment 10.

Comment 13

[Timothy Nikkel (:tnikkel)]

I actually knew why last night, but I just wanted to cancel the review request and fix it tomorrow. I didn't expect someone else to come along and pick it up!

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481930 [details] [diff] [review]
patch v2

r=me

Comment 16

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/a0861f461354