Canvas.drawWindow() + transparent root element no longer transparent in 3.1

RESOLVED FIXED

Status

()

Core
Canvas: 2D
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: telega, Assigned: zwol)

Tracking

({fixed1.9.1, regression, testcase})

Trunk
x86
Windows XP
fixed1.9.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2

In Firefox 3.1b2 canvas.drawWindow() function works incorrectly:

    var context = canvas.getContext("2d");
    context.clearRect(0, 0, canvas.width, canvas.height);
    context.drawWindow(wnd, 0, 0, width, height, "rgba(0,0,0,0)");
    var dataURL = canvas.toDataURL("image/png");

This code produces image on WHITE background in Firfox 3.1b2. But in Firefox 3.0.3 the same code produces TRANSPARENT background as it should.

Reproducible: Always

Steps to Reproduce:
1.Execute sample code in Fx 3.1b2.
2.Resulting image has white background.
3.Execute the same code in Fx 3.0.3.
4.Image has transparent backgroung as expected.
Actual Results:  
Background is white.

Expected Results:  
Background should be transparent.

This functionality is cruical for Fast Dial addon, please fix this ASAP.

Updated

9 years ago
Component: General → Layout: Canvas
Product: Firefox → Core
QA Contact: general → layout.canvas
can you attach a basic testcase to the bugreport please?
and, does it work correctly in beta1?
Severity: major → normal
Version: unspecified → 1.9.1 Branch
Potentially caused by bug 469170

Comment 3

9 years ago
(In reply to comment #2)
> Potentially caused by bug 469170
Bug 453566, perhaps.
Created attachment 354446 [details]
testcase (uses enhanced privileges)

You should see a green box on this page.

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
Keywords: regression, testcase

Comment 6

9 years ago
This testcase is in quirks mode... is that intentional?
Does that matter? I don't think it matters, afaik.
(Assignee)

Comment 8

9 years ago
I would describe this as an intentional change (because of bug 453566) to the way HTML pages are drawn, rather than as a regression in drawWindow.  You may be able to work around it by setting opacity: 0 on the outermost CSS box being displayed in the window, but that's just a guess.
(Reporter)

Comment 9

9 years ago
As far as I know setting opacity: 0 on a div makes all its contents transparent, not just the background.
Hmm, can we just revert bug 453566 without breaking anything? PresShell::Paint should compute a solid color to be passed in for aBackground to nsLayoutUtils::PaintFrame, so things should work fine with a transparent background on the root element.
(In reply to comment #10)
> Hmm, can we just revert bug 453566 without breaking anything? PresShell::Paint
> should compute a solid color to be passed in for aBackground to
> nsLayoutUtils::PaintFrame, so things should work fine with a transparent
> background on the root element.

Not sure, but if we can that would be the easiest fix.  We really shouldn't break this, given that we have the ability to specify a background color for drawWindow in the first place..
(Assignee)

Comment 12

9 years ago
Three things:

 - Why wouldn't the PresShell::Paint code trigger in the drawWindow case?
 - Reftests use drawWindow, and bug 454349 is about making sure that the regular paint path, as called from drawWindow, draws opaque color over the entire viewport even when the HTML doesn't specify an opaque background.
 - The background color argument to drawWindow is (as I see it) about ensuring that the entire *canvas rectangle* is painted even when the viewport isn't that big, so it's not useless even though in normal operation almost all of that area will be painted over with opaque colors from the regular paint path.

I think the burden needs to be on the OP to flag their unusual use case here.
(Assignee)

Comment 13

9 years ago
[ I would suggest that the background-color argument to drawWindow should override the color normally computed by PresShell::Paint - that would cover both the reftest use case and the OP's, I think - but it won't work, because the viewport isn't guaranteed to cover the entire canvas rectangle, as you can see in gory detail in bug 454349. ]
(In reply to comment #12)
>  - The background color argument to drawWindow is (as I see it) about ensuring
> that the entire *canvas rectangle* is painted even when the viewport isn't that
> big, so it's not useless even though in normal operation almost all of that
> area will be painted over with opaque colors from the regular paint path.

It's not -- if someone wanted to ensure that, they can just call fillRect() beforehand with a fill color.  The default background is 'transparent'.  The user agent just happens to ensure a solid color gets painted beforehand (white, by default) when rendering html content.
Ok, this needs to get fixed -- at this point I'm just going to revert bug 453566.
Blocks: 453566
Flags: blocking1.9.1? → blocking1.9.1+
Version: 1.9.1 Branch → Trunk
(Assignee)

Comment 16

9 years ago
I remain of the opinion that this is not a bug.
(Assignee)

Comment 17

9 years ago
#if 0'ing the code that bug 453566 added to nsCSSRendering::PaintBackground does not regress the test case in that bug (I assume this is because roc was right in comment #10; I might send in a patch later to take that code out, just for tidiness).  However, it does not help the test case in this bug.

I don't think we can have it both ways.  Either HTML documents are forced to have an opaque background, and the OP's extension is broken; or they aren't, and with moderately evil CSS you can get garbage in the background.  I prefer option one.
I don't really understand that bug; HTML documents could be forced to have a fully transparent black background by default if the destination surface supports alpha, or to opaque white if it doesn't.
(Assignee)

Comment 19

9 years ago
How might we tell, from within either nsCSSRendering::PaintBackground or PresShell::Paint?  Bad rendering with OS windows was observed on at least Linux and Windows.
surface->GetContentType(); if CONTENT_COLOR, then there's no alpha, otherwise there is.
(Assignee)

Comment 21

9 years ago
I'm not aware of any gfxASurface object accessible to PresShell::Paint -- maybe there's one hiding in one of the many context objects, but I don't know where to look for it, or how to tell which one is relevant right then.  And, tracing surface->GetContentType() down into Cairo, I do not believe that will correctly distinguish the case where bad rendering will occur (painting into a native OS window) from the case where it won't (painting into a canvas).  And, tricks of this nature will vitiate what we're trying to do in bug 454349.
You can get a surface from the current gfxContext by asking for its current target.  A native OS window almost never has alpha, though we're also basically never going to be painting into one.  We always paint into double buffered surfaces, which get created correctly with or without alpha.  A canvas's backbuffer has alpha (except for the moz-opaque hack).

I still wish there was another way to fix this, but I don't fully understand what the problem is.  It seems like we should just paint the solid colored background below <html> for content <browser>s.
(Assignee)

Comment 23

9 years ago
As far as I'm concerned, anything that makes drawWindow behave differently than normal painting that will end up in an OS window is unacceptable, because we rely on it being the same for testing.  So I'm not interested in solutions that boil down to "deduce in PresShell::Paint whether or not this is drawWindow".

I'm going to look at the option I mentioned in comment 13, though; thinking about it more, that may actually work for both use cases.
Summary: Canvas.drawWindow() bug in Fx 3.1b2 → Canvas.drawWindow() + transparent root element no longer transparent in 3.1
(In reply to comment #23)
> As far as I'm concerned, anything that makes drawWindow behave differently than
> normal painting that will end up in an OS window is unacceptable, because we
> rely on it being the same for testing.  So I'm not interested in solutions that
> boil down to "deduce in PresShell::Paint whether or not this is drawWindow".

And as far as I'm concerned, anything that regresses working functionality is unacceptable as well.  Your idea in comment #13 could work.
(Assignee)

Comment 25

9 years ago
I'll take responsibility for working out what is to be done here, at least - but I'm still considering the reftests' needs as laid out in 454349 a higher priority than whatever it was the original extension was trying to do.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Priority: P2 → --
(In reply to comment #25)
> I'll take responsibility for working out what is to be done here, at least -
> but I'm still considering the reftests' needs as laid out in 454349 a higher
> priority than whatever it was the original extension was trying to do.

Sigh; reftests' needs do not override previously-working functionality that was exposed to extension developers, regardless of what those developers are trying to do with it.  If reftests specifically have special needs, then we should do a workaround or special mode for reftests.
(Assignee)

Comment 27

9 years ago
Created attachment 356805 [details] [diff] [review]
rev 0: attempt to fix (background color not reliable yet)

I will be out the rest of the afternoon, so here is what I've got so far.  You'll need to apply the patch in bug 470916 first.  As you may see from the spurious PASS in the test case (which is deliberately drawing with background "red", which should make it fail) the drawWindow background color is not honored reliably, if all we do is set it as the prescontext default.  I would welcome suggestions.
Sorry, I got confused somehow in bug 453566. That fix is just wrong. We shouldn't be altering the background color in nsCSSRendering::PaintBackground to ensure the root background is opaque. Comment #10 is correct. Bug 467459 had the correct fix, which was to compute an opaque background color to be used as the backstop whenever we paint an opaque widget. So we should just back out 453566 immediately, no matter what.

I sympathise with Zack that it would be nice for drawWindow to use the same backstop as normal rendering. On the third hard, I can see that it's useful for it to not use a backstop, and it kinda makes sense for drawWindow to depend only on the document, and ignore the browser's default background color.

So I suggest we add another drawWindow flag to force it to use the same backstop color as PresShell::Paint. We'd use that from reftests; when that flag's set, drawWindow's background color parameter would effectively be ignored.
Bug 473398 (landed on trunk) fixes this bug by mostly reverting bug 453566.

Someone (Zack?) should file a bug for the last part of comment #28.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Depends on: 473398
Resolution: --- → FIXED
(Assignee)

Comment 30

9 years ago
Created attachment 357921 [details] [diff] [review]
rev 1: just a test case

This patch adds a mochitest and does nothing else, the bug itself having been fixed by the patch in bug 473398.  I'm reopening this bug just to get the test case committed.
Attachment #356805 - Attachment is obsolete: true
Attachment #357921 - Flags: superreview?(roc)
Attachment #357921 - Flags: review?(roc)
Attachment #357921 - Flags: approval1.9.1?
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #357921 - Flags: superreview?(roc)
Attachment #357921 - Flags: review?(roc)
Attachment #357921 - Flags: review+
I don't think we should reopen the bug --- it's fixed.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Pushed test http://hg.mozilla.org/mozilla-central/rev/3c241d029ca2
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Comment 33

9 years ago
This test case doesn't seem to have made it onto the 1.9.1 branch yet.
(Assignee)

Updated

9 years ago
Whiteboard: [needs 1.9.1 approval]
Comment on attachment 357921 [details] [diff] [review]
rev 1: just a test case

Looks like this is already approved per bug 478784 comment 19, but marking it here as well, although what's there is probably what should be landed.
Attachment #357921 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 1.9.1 approval] → [needs 1.9.1 landing]
Whiteboard: [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.