212366 - implement CSS3/SVG "group opacity"

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

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

Subject line says it all. I've finally done it :-).

Comment 1

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

I'll attach the patch tomorrow, I need to sleep now. But for now I'll attach a
testcase and a screenshot.

Comment 2

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

Attachment: testcase

This testcase tests a few evil things. Of course they all work with my patch.
1) 'opacity' applied to IFRAMEs
2) group opacity (the white line in the middle of the red DIV appears pink
currently, but should be totally white with group opacity)
3) group opacity blends the entire element as a group, including 'absolute' and
'fixed' children (thus the yellow fixed DIV is in the same blending group as
the red+white DIV).

Comment 3

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

Attachment: screenshot

The Google IFRAME appears a little discoloured. I'm not sure why, but I'm
running in 16bit color so it's probably a rounding error in the blender. Other
than that it looks and feels good (a bit slow when you scroll to the bottom,
but other work is needed to fix that).

Comment 4

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

Attachment: patch

here it is

Comment 5

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

Dunno if hyatt reads any bugmail, but he might be interested in this given that
he's just done this (or something similar) for Safari

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

>Index: content/html/style/src/nsHTMLCSSStyleSheet.cpp
>   // Disable everything in the nsRuleDataDisplay struct except 'float'
>   // and 'clear'.
>   if (aData->mSID == eStyleStruct_Visibility) {
>     nsCSSValue inherit(eCSSUnit_Inherit);
>+    nsCSSValue one(1.0f, eCSSUnit_Number);
>     aData->mDisplayData->mVisibility = inherit;
>     aData->mDisplayData->mDirection = inherit;
>-    aData->mDisplayData->mOpacity = inherit;
>+    aData->mDisplayData->mOpacity = one;
>   }
> 
>   if (aData->mSID == eStyleStruct_Display) {

You need to move the opacity setting into the eStyleStruct_Display case.

Comment 7

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

OK, sure.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

the nsLayoutAtomList.h changes look unrelated to this bug
                                                                                
It looks like the semantics of RenderViews are a little interesting now
-- aRCSurface is both a pointer and a boolean indicating whether you're
double-buffering.  Could there be a comment about this (i.e., that it's
optionally null and what happens each way)?  Likewise for
CreateBlendingBuffers, although one comment could refer to the other.
                                                                                
in nsViewManager::RenderViews:
                                                                                
> +      if (!aRCSurface) {
> +        NS_WARNING("Cannot support translucent elements with doublebuffering
disabled");
> +      }
This could just be an assertion.



Is there documentation somewhere that could give me a general idea of how
painting works -- i.e., what exactly an nsIRenderingContext or nsDrawingSurface
is an abstraction of, and how those abstractions relate to the OS APIs and to
how we use the features of the video card, etc.?  I've never understood that,
and I feel like I should in order to review this patch.  Or should I just read
the code and the OS API documentation...?

Comment 9

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

> the nsLayoutAtomList.h changes look unrelated to this bug

oops, yeah

> Could there be a comment about this

yeah

> Is there documentation somewhere that could give me a general idea of how
> painting works

There isn't. There should be and I will add some detailed comments to this patch
that will hopefully help.

nsIRenderingContext and nsDrawingSurface are poorly defined abstractions. I can
do my best to describe them but ultimately there isn't going to be a very good
description because they're just not totally coherent. The best I can do is
describe how the view code is supposed to work and what I expect from
nsIRenderingContext and nsDrawingSurface. [GFX is ripe for an overhaul and this
may be my focus when my family's away for six weeks in August/September.] I
won't be able to produce this until next week though, since I'm just about to
leave town for the weekend.

Comment 10

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

BTW if you could test this on Windows, I'd much appreciate it. Probably should
be tested on the Mac too.

Comment 11

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

http://ocallahan.org/mozilla/gfx-overview.html has some text that may be
helpful. Let me know if there's anything you want fleshed out and I'll have a go.

Comment 12

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

Attachment: new patch

all as promised...

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I applied your patch on the Mac and got nothing looking anything like the
screenshot, in either cocoa (Camino) or carbon (Mozilla).

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I applied your patch on the Mac and got nothing looking anything like the
screenshot, in either cocoa (Camino) or carbon (Mozilla).

To give a little more detail:
 * nothing in the iframes showed up, except for a blinking caret and the
scrollbars for the iframes
 * most things were just white, but some areas painted gray some of the time.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It works fine on GTK1.  Nothing with opacity shows up on GTK2 (as expected --
not a regression).

On Windows the opaque things show up rather messed up -- they look like the
pixels are spread out in some way, so that things end up slanting diagonally.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: screenshot of attachment 128209's behavior on Windows

Comment 17

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

oh dear. the Windows screenshot seems to indicate some sort of stride problem.
Of course I have no clue about the Mac.

Comment 18

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

Attachment: simpler test

Can you try this patch on Windows and attach a screenshot? Maybe I can figure
it out offline. Thanks!!!

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Windows screenshot of attachment 129207

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 128209 [details] [diff] [review]
new patch

minusing since it doesn't work (and I'm trying to go through my queue)

Comment 21

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

Attachment: updated patch to trunk

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

patching file content/shared/public/nsStyleStruct.h
Hunk #1 FAILED at 699.
1 out of 2 hunks FAILED -- saving rejects to file
content/shared/public/nsStyleStruct.h.rej

Comment 23

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Screenshot of the complicated testcase on Windows

Using the updated patch.

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

roc - It works properly on both testcases now.

Comment 25

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

Excellent. Can you do a few more things for me?

1) attach a cvs diff of the changes. (cvs diff -utp6
gfx/src/nsRenderingContextWin.cpp)
2) change display depths to 16, 24, 32bits and check that it works in all of
them (you may need to restart Mozilla between changes, I'm not sure)
3) make sure that the content of the IFRAMES (Google, Yahoo) works. You should
be able to surf in there.

Thanks so much.

Comment 26

[David Hyatt]

Just a random plea... if you could add -khtml-opacity to your testcases as well, then I could do side-
by-side comparisons in Mozilla and Safari (on Panther).

Comment 27

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

I can provide those testcases. BTW Chris, I meant "cvs diff -utp6
gfx/src/win/nsDrawingSurfaceWin.cpp" of course

Comment 28

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

It did *not* work in 16 bit color, and my driver doesn't offer 24 bit color. 
The iframes do work properly.

Comment 29

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

Attachment: GFX/Win fix

This should fix a couple of bugs in nsRenderingSurfaceWin. Chris, can you apply
the patch by hand and test it out when you have time? thanks.

Comment 30

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

Attachment: fixing another bug

While working with Chris on the GFX/Win bug, I noticed another problem. Both
the outerframe and the innerframe were getting opacity 0.7, so we were blending
twice. (And of course the effect was of opacity 0.49.) This patch gives the
innerframe a psuedoelement style context so that style rules on the outerframe
don't apply to it.

Comment 31

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

Comment on attachment 133974 [details] [diff] [review]
fixing another bug

This particular fix is independent of the opacity work.

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: It works :-)

Tested in 16 & 32 bit color.  The iframes seem to work properly.

Comment 33

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

Comment on attachment 133680 [details] [diff] [review]
updated patch to trunk

I'm pretty confident that this code is correct, since getting it working on
Win32 only required bug fixes to nsRenderingContextWin. It would be easiest to
just land this and if it still doesn't work on Mac, have a Mac person fix any
Mac Gfx bugs.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133970 [details] [diff] [review]
GFX/Win fix

sr=bzbarsky, assuming those comments are correct and all... but I know little
of win32 gfx, so please make sure that whoever reviews this knows something
about it....

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133680 [details] [diff] [review]
updated patch to trunk

I'm happy with the content/layout changes here.  I still need to look at the
view changes some more.

Comment 36

[Ere Maijala (slow)]

Comment on attachment 133970 [details] [diff] [review]
GFX/Win fix

I believe it's top to bottom with positive bitmap heights and vice versa.
Anyway,
r=ere

Comment 37

[Ere Maijala (slow)]

Eh, I meant bottom-to-top with positive heights.

Comment 38

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

I verified, with Chris's help, that on this path, mBitmapInfo.biHeight is indeed
negative but the scanlines were being interpreted bottom-to-top. He's running
WinXP. dbaron's screenshots indicate his build was behaving the same. I don't
know how to square this with the documentation. It may be because on the code
path modified by this patch, we're dealing with a bitmap that is not a
DIBSection (the bitmap bits are only initialized in Lock() if the drawing
surface was created without FOR_PIXEL_ACCESS.) I wish the GDI bitmap API didn't
suck so much.

Comment 39

[Boris Zbarsky [:bzbarsky]]

*** Bug 223769 has been marked as a duplicate of this bug. ***

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I find the variable naming convention for display list elements (e prefix) a
little odd, since we tend to use that for the values of enumerations.

In the new code in nsViewManager::OptimizeDisplayList, I'd prefer prefix
increment/decrement to postfix.

In SortByZOrder, it's worth commenting that you don't need to duplicate filters
like you duplicate clips because filter implies non-auto z-index.

(more comments later)

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133680 [details] [diff] [review]
updated patch to trunk

+static PRInt32 PrintDisplayListElement

+    PRInt32 zindex = view ? view->GetZIndex() : nsnull;

0 is better than nsnull here.

However, it might be cleaner to just have separate |printf|s for the
view and non-view cases.


+    memset(nest, ' ', sizeof(nest));

Why not use |aIndent| instead of |sizeof(nest)|?
(And maybe assert that |aIndent < sizeof(nest)|?

+      } else if (child->mFlags & PUSH_FILTER) {
+	 sortStartIndex++;
+	 sortEndIndex--;
+      } else {

It might make sense to add an NS_ASSERTION(!autoZIndex, ...).

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

+    char nest[400];
+    memset(nest, ' ', sizeof(nest));
+    nest[aIndent] = 0;
+
+    printf("%sDisplayZTreeNode@%p\n", nest, (void*)aNode);

can't the printf be written as:
printf("%*cDisplay...", ' ', " ", (void*)aNode);

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, could you fix the shadowing variables:

/builds/counters/mozilla/view/src/nsViewManager.cpp: In function `void
SortByZOrder(DisplayZTreeNode*, nsVoidArray&, nsVoidArray&, int)':
/builds/counters/mozilla/view/src/nsViewManager.cpp:1088: warning: declaration
of `child' shadows a previous local
/builds/counters/mozilla/view/src/nsViewManager.cpp:1073: warning: shadowed
declaration is here
/builds/counters/mozilla/view/src/nsViewManager.cpp: In member function `void
nsViewManager::RenderViews(nsView*, nsIRenderingContext&, const nsRegion&, void*)':
/builds/counters/mozilla/view/src/nsViewManager.cpp:1311: warning: declaration
of `i' shadows a previous local
/builds/counters/mozilla/view/src/nsViewManager.cpp:1307: warning: shadowed
declaration is here

Comment 44

[Brian Ryner (not reading)]

This seems to have increased pageload time on btek just a bit.  956 -> 971, with
this being the only thing in the checkin window.

Comment 45

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

Specifically, that was the change to the nsFrameInnerFrame style context. I have
no idea why it would have such an effect on Tp. Could the extra style resolution
really be that costly?

Comment 46

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

I'm going to check in the other changes anyway. They're independent of how we
fix the IFRAME thing.

Comment 47

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

checked in with all comments addressed.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On mac, nothing with opacity shows up, and I see the following repeatedly:

###!!! ASSERTION: Cannot support translucent elements with doublebuffering
disabled: 'aRCSurface', file /builds/dbaron/mozilla/view/src/nsViewManager.cpp,
line 1317
Break: at file /builds/dbaron/mozilla/view/src/nsViewManager.cpp, line 1317
###!!! ASSERTION: nested lock attempt: '0', file
/builds/dbaron/mozilla/gfx/src/mac/nsDrawingSurfaceMac.cpp, line 158

Comment 49

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

Oh, is our double-buffering always disabled on the Mac? Hmm...

Comment 50

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

Attachment: Reorg nsFrameFrame

Here's a VERY LIGHTLY TESTED reorg of nsFrameFrame.cpp. It eliminates the
innerframe, gives the outerframe a better name, merges all the functionality
that used to be in the innerframe into the outerframe except for a few methods
that weren't being used, and deCOMtaminates internal methods. The basic idea is
that in lieu of the innerframe we have an anonymous child view (corresponding
to no frame) with a widget for the subdocument to live in. There are a few more
changes I need to make, like fixing the NS_New... function name and removing
the unused InnerFrame type atom and its uses, and fixing a few comments outside
nsFrameFrame.

Comment 51

[Zibi Braniecki [:zbraniecki][:gandalf]]

With todays checkins testcase 127505 works OK, but there is still problem with
speed. part of a yellow box with "Hello Kitty" which moves behind "google" frame
is not synchronized with rest of this box.
Also when I move vertical scroll of "google" frame, part of this frame which is
over yellow box is not repainted until I stop scrolling.

Comment 52

[Jacek Piskozub]

Today's check-in could have caused bug 224478.

Comment 53

[Doron Rosenberg (IBM)]

roc - could this have broken scrolling="no" on iframes? It broke between the
30th and 31 of october.

Comment 54

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

Sure it could have :-) bug number?

Comment 55

[Jacek Piskozub]

It's bug 224628, I guess.

Comment 56

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

Attachment: Decent nsFrameFrame reorg

OK, this has been somewhat tested and I've fixed a few bugs. I think it's now
ready for review.

I found a bug in the view system where repositioning a view which does not have
a widget but which has views with child widgets did not update the locations of
the child widgets. We seemed to be working around this in
nsContainerFrame::PositionChildViews by repositioning all descendant views,
whereas logically we should be able to stop descending into the child frames of
a frame with a view. I fixed this in the view system so that
nsViewManager::MoveViewTo actually works properly in all cases. I also removed
the workaround in PositionChildViews. It's a bit risky but it's the right thing
to do.

Comment 57

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

Comment on attachment 134756 [details] [diff] [review]
Decent nsFrameFrame reorg

oops, found another bug

Comment 58

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

Regarding comment 44, backing out that change seems not to have affected Tp at
all :-(. (The change was backed out because it caused a regression in bug
224628. The nsFrameFrame reorg obsoletes it anyway.)

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks to me like it helped Tp.  The 4 runs before were 968,970,968,967, and
the 4 runs after were 964,966,964,965.

Comment 60

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

True, maybe 5ms improvement, but not as much as the ~15ms decrease that bryner
noted (and I noticed).

Comment 61

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

Attachment: nsFrameFrame reorg

Okay. This seems to work pretty well. No regression of "scrolling=no". And I
actually diffed all the files this time :-).

Comment 62

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

Comment on attachment 134838 [details] [diff] [review]
nsFrameFrame reorg

Hello David, another big patch for you to review :-).

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Re comment 60: the original inccrease was more like 10ms -- it went down the
next cycle.

Comment 64

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

Ok. It's probably a mistake to probe too deeply into the mysteries of btek.
Anyway, Axel's parser string patch got us back into the 950's so I'm happy :-)

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134838 [details] [diff] [review]
nsFrameFrame reorg

>Index: view/src/nsScrollPortView.cpp
>-  if (prefs) {
>+  if( prefs) {

yuck!

>Index: layout/html/base/src/nsContainerFrame.cpp
>@@ -995,24 +995,27 @@ nsContainerFrame::PositionChildViews(nsI
> 
>   do {
>     // Recursively walk aFrame's child frames
>     nsIFrame* childFrame;
>     aFrame->FirstChild(aPresContext, childListName, &childFrame);
>     while (childFrame) {
>-      // Position the frame's view (if it has one) and recursively
>+      // Position the frame's view (if it has one) otherwise recursively
>       // process its children
>-      PositionFrameView(aPresContext, childFrame);
>-      PositionChildViews(aPresContext, childFrame);
>+      if (childFrame->HasView()) {
>+        PositionFrameView(aPresContext, childFrame);
>+      } else {
>+        PositionChildViews(aPresContext, childFrame);
>+      }
> 

Can you make the same change to PlaceFrameView in nsBlockFrame.cpp?  Also, I'd
recommend retesting the testcases for bug 79315, bug 205165, etc.  I remember
seeing some rather ugly cases while working on one of those bugs (or maybe bug
174149), but I don't remember what made them ugly.  (Maybe they aren't
anymore.)

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134838 [details] [diff] [review]
nsFrameFrame reorg

The changes to the Paint methods make me wonder:
 * have you tested bug 75737 / bug 75739
 * does this change what 'visibility: hidden' on a frame or iframe does?  It
seems like this would break them.

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: iframe reflow testcase

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you test printing and print preview of pages with iframes?

Comment 69

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

Okay, that revealed some bugs :-). I'll revise the patch.

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How about moving the nsFrameFrame reorg into a different bug, so we can say
"regressions from bug NNNNN" and have some idea of what happened...?

Comment 71

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

Good idea. Filed bug 225820

Comment 72

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

Marking fixed now that we've moved the rest of the work out of here.

Comment 73

[Hixie (not reading bugmail)]

Using attachment 127505 [details], if I scroll down so that the red div bars go off the
top of the screen, the fixed positioned yellow div suddenly changes opacity. Is
that a known bug, or should I file one? (Windows 2000, Seamonkey, today's
nightly, with SVG support enabled.)

Comment 74

[Boris Zbarsky [:bzbarsky]]

So is there a follow-up bug for whatever Mac problems we have here?

Comment 75

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

Ian: screenshot?

Comment 76

[Hixie (not reading bugmail)]

Attachment: What attachment 127505 looks like at the top...

Comment 77

[Hixie (not reading bugmail)]

Attachment: ...and what it looks like at the bottom.

Comment 78

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

Bug 228441 is the Mac problem.

Comment 79

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

Ian, that second screenshot seems to show garbage at the top ("x-terminal
emulator" etc). is that really there or am I misinterpreting?

Comment 80

[Hixie (not reading bugmail)]

roc: That's just the updating being ridiculously slow on my machine and my
taking the screenshot too early. The bug I was attempting to show is the fact
that the yellow Hello Kitty box has different opacity if you scroll further
down. I can reproduce that on both Win32 and X using both Firebird and
Seamonkey. As soon as the Hello Kitty's block's containing block is outside the
viewport, the opacity changes. (The opacity of the Hello Kitty block is
determined by its container.)

I assume this is not a known bug. Do you want me to file a new one?

Comment 81

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

I filed it as bug 228861. Thanks!

Comment 82

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Additional to comment 80 i also see this slow update when i scroll the page. It
only happens when the yellow box is behind the invisible one or what it is. 

But this would depend on bug 64401 i think.

Comment 83

[:aceman]

It is OK, that this works correctly only in 16bit and up, but something should
be done with bug 241939.