Last Comment Bug 664127 - Allow to set opacity on moz-tree-image
: Allow to set opacity on moz-tree-image
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Marco Bonardo [::mak]
Depends on:
Blocks: 416459
  Show dependency treegraph
Reported: 2011-06-14 07:02 PDT by Marco Bonardo [::mak]
Modified: 2011-08-31 05:25 PDT (History)
10 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1.0 (3.75 KB, patch)
2011-06-15 03:35 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (2.15 KB, patch)
2011-06-15 05:12 PDT, Marco Bonardo [::mak]
roc: review+
Details | Diff | Splinter Review
reftest v1.0 (2.25 KB, patch)
2011-06-16 06:54 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
reftest v1.1 (2.55 KB, patch)
2011-06-17 03:45 PDT, Marco Bonardo [::mak]
ehsan: review-
Details | Diff | Splinter Review
reftest v1.2 (2.53 KB, patch)
2011-06-18 04:35 PDT, Marco Bonardo [::mak]
roc: review+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-06-14 07:02:49 PDT
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 ( )
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) 2011-06-14 16:47:30 PDT
Comment 2 Marco Bonardo [::mak] 2011-06-15 03:30:24 PDT
Comment 3 Marco Bonardo [::mak] 2011-06-15 03:35:41 PDT
Created attachment 539469 [details] [diff] [review]
patch v1.0

Something like this?
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 03:44:05 PDT
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] 2011-06-15 05:12:20 PDT
Created attachment 539479 [details] [diff] [review]
patch v1.1

Good idea, the change is much more restricted.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 17:08:29 PDT
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] 2011-06-16 06:54:19 PDT
Created attachment 539787 [details] [diff] [review]
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) 2011-06-16 20:19:03 PDT
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] 2011-06-17 01:53:46 PDT
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] 2011-06-17 01:57:56 PDT
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] 2011-06-17 03:45:31 PDT
Created attachment 540023 [details] [diff] [review]
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 2011-06-17 04:17:40 PDT
(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] 2011-06-17 06:15:23 PDT
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] 2011-06-17 07:48:14 PDT
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 :Ehsan Akhgari 2011-06-17 11:38:06 PDT
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] 2011-06-17 12:05:55 PDT
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] 2011-06-17 12:41:42 PDT
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] 2011-06-17 13:10:41 PDT
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] 2011-06-18 04:35:57 PDT
Created attachment 540229 [details] [diff] [review]
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 (
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-18 06:02:38 PDT
Comment on attachment 540229 [details] [diff] [review]
reftest v1.2

Review of attachment 540229 [details] [diff] [review]:
Comment 23 Vlad [QA] 2011-08-31 05:25:48 PDT
Is there a way to verify this?

Note You need to log in before you can comment on or make changes to this bug.