664127 - Allow to set opacity on moz-tree-image

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Marco Bonardo [:mak]]

I'm trying to reproduce the classic "cut" effect on a tree, where the icon for a certain row gets some transparency. Unfortunately ::moz-tree-image opacity is ignored.

I see most of the drawing is done in nsTreeBodyFrame::PaintImage ( http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#3380 )
I can most likely get opacity from imageContext->GetStyleDisplay()->mOpacity, but I have no idea how to apply it to the drawing code.

Any hint or example I could look at?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

gfxContext::PushGroup/gfxContext::PopGroupToSource/gfxContext::Paint.

Comment 2

[Marco Bonardo [:mak]]

OMGITWORKS

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Something like this?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539469 [details] [diff] [review]
patch v1.0

Review of attachment 539469 [details] [diff] [review]:
-----------------------------------------------------------------

I'd prefer you to not add this to nsLayoutUtils::DrawImage/DrawImageInternal. Instead, just do the push/pop in nsTreeBodyFrame, using aRenderingContext->ThebesContext().

::: layout/base/nsLayoutUtils.cpp
@@ +3290,5 @@
>                 aImageFlags);
> +
> +  if (aOpacity != 1.0f) {
> +    ctx->PopGroupToSource();
> +    ctx->SetOperator(gfxContext::OPERATOR_OVER);

You don't need to set the operator here, it should already be OVER.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Good idea, the change is much more restricted.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539479 [details] [diff] [review]
patch v1.1

Review of attachment 539479 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

A reftest for this would be appreciated.

Comment 7

[Marco Bonardo [:mak]]

Attachment: reftest v1.0

This one should work, one tree has a fully opaque black icon with opacity: 0.5, the other one has a half opaque black icon with no opacity.
Locally I get:
REFTEST TEST-PASS | file:///c:/mozilla/places/layout/reftests/bugs/664127-1.xul | image comparison (==)
sounds like working, but being my first reftest I don't believe it :)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is probably a little too fragile, there's no guarantee that the opacity calculations on the platform match the values you calculated in your image.

I'd just use a solid black image in both files and put opacity:0.5 on the <tree> in your reference file.

Comment 9

[Marco Bonardo [:mak]]

OK I'll try, btw the previous test (the one with black icon and opacity: 0.5) for some reason was not showing the icon at all in the test on tryserver, while it works here locally...

Comment 10

[Marco Bonardo [:mak]]

Well, not completely true, it worked fine on linux opt. Maybe it doesn't wait enough for the icon to show.

Comment 11

[Marco Bonardo [:mak]]

Attachment: reftest v1.1

I was able to reproduce the failure locally and it was indeed a snapshot timing thing. I added a 50ms delay and now it works, will push to try.

Comment 12

[[e-mail]]

(In reply to comment #11)
> I added a 50ms delay and now it works
Just giving you a chance to avoid filing another orangefactor bug...

Comment 13

[Marco Bonardo [:mak]]

fwiw, it still fails on Mac try (still waiting other platforms), I thought reftests were easier to get right!  In this case there is absolutely nothing I could listen at to be sure it is ready for the comparison, so I don't see how I could handle it differently than with a reasonable timeout (it is used in other reftests as well), unless you have suggestions.  Painting a simple icon can't be that much slow!

Comment 14

[Marco Bonardo [:mak]]

So, it is permagreen on all platforms but Mac, where it's permaorange. The reference image (<tree> with opacity: 0.5) is completely blank there, while the reftest is correct. If I open the reftest on my Mac it looks correct though (I still have to run the reftest harness though). I don't think this Mac thing is due to the timeout though, it failed at all tried and respins.

Comment 15

[(no longer active)]

Comment on attachment 540023 [details] [diff] [review]
reftest v1.1

You're not running any js code, so you shouldn't need the timeout at all.  By default the reftest framework waits until a test is loaded and then paints the page.  What rendering do you get if you remove the reftest-wait class and the onload handlers?

Comment 16

[Marco Bonardo [:mak]]

the tree icon is painted asynchronously, the usual waiting is not good enough for it. If I remove the timeout the icon is not painted.

Comment 17

[Marco Bonardo [:mak]]

hm, looks like my reftest was fine, and it just found a Mac regression, on Firefox 4.0.1 I can open my reftest and I see a half-opaque tree, on current Firefox 7 I see an empty page :(

Comment 18

[Marco Bonardo [:mak]]

To clarify:
- After a brief irc discussion with Ehsan, painting is synchronous, but most likely the asynchronous part is fetching the image from the channel.  I'll try adding a second image to the page to see if this fetching can become cached so I can get rid of the timeout.
- The Mac regression is a problem apart, I'm looking for a regression range and will file a separate bug for it (we may be able to cover 2 bugs with a single reftest!)

Comment 19

[Marco Bonardo [:mak]]

Attachment: reftest v1.2

This doesn't need a timeout, the <img> preloads the image so that the tree has it in cache, to allow for this I need an external stylesheet. Using a real image also bypasses bug 665133, so that one will need its own reftest.
Note I can't preload a datauri since datauris are not cached.
Already passed try (http://tbpl.mozilla.org/?tree=Try&rev=329996f60c77)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540229 [details] [diff] [review]
reftest v1.2

Review of attachment 540229 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 21

[Marco Bonardo [:mak]]

baking on Places:
http://hg.mozilla.org/projects/places/rev/ed60c3abdb71
http://hg.mozilla.org/projects/places/rev/26bda006334c

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ed60c3abdb71
http://hg.mozilla.org/mozilla-central/rev/26bda006334c

Comment 23

[Vlad [QA]]

Hi.
Is there a way to verify this?
Thanks