Closed Bug 212366 Opened 21 years ago Closed 21 years ago

implement CSS3/SVG "group opacity"

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(14 files, 4 obsolete files)

(deleted), text/html
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(deleted), image/png
Details
(deleted), patch
emaijala+moz
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
(deleted), patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(deleted), image/png
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/html; charset=UTF-8
Details
(deleted), image/png
Details
(deleted), image/png
Details
Subject line says it all. I've finally done it :-).
I'll attach the patch tomorrow, I need to sleep now. But for now I'll attach a testcase and a screenshot.
Attached file testcase (deleted) —
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).
Attached image screenshot (deleted) —
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).
Attached patch patch (obsolete) (deleted) — Splinter Review
here it is
Attachment #127597 - Flags: superreview?(dbaron)
Attachment #127597 - Flags: review?(dbaron)
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
Blocks: 93156
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.
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...?
> 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.
BTW if you could test this on Windows, I'd much appreciate it. Probably should be tested on the Mac too.
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.
Attached patch new patch (obsolete) (deleted) — Splinter Review
all as promised...
Attachment #127597 - Attachment is obsolete: true
Attachment #127597 - Flags: superreview?(dbaron)
Attachment #127597 - Flags: review?(dbaron)
Attachment #128209 - Flags: superreview?(dbaron)
Attachment #128209 - Flags: review?(dbaron)
I applied your patch on the Mac and got nothing looking anything like the screenshot, in either cocoa (Camino) or carbon (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.
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.
oh dear. the Windows screenshot seems to indicate some sort of stride problem. Of course I have no clue about the Mac.
Attached file simpler test (deleted) —
Can you try this patch on Windows and attach a screenshot? Maybe I can figure it out offline. Thanks!!!
Comment on attachment 128209 [details] [diff] [review] new patch minusing since it doesn't work (and I'm trying to go through my queue)
Attachment #128209 - Flags: superreview?(dbaron)
Attachment #128209 - Flags: superreview-
Attachment #128209 - Flags: review?(dbaron)
Attachment #128209 - Flags: review-
Attached patch updated patch to trunk (deleted) — Splinter Review
Attachment #128209 - Attachment is obsolete: true
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
roc - It works properly on both testcases now.
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.
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).
I can provide those testcases. BTW Chris, I meant "cvs diff -utp6 gfx/src/win/nsDrawingSurfaceWin.cpp" of course
It did *not* work in 16 bit color, and my driver doesn't offer 24 bit color. The iframes do work properly.
Attached patch GFX/Win fix (deleted) — Splinter Review
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.
Attached patch fixing another bug (deleted) — Splinter Review
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 on attachment 133974 [details] [diff] [review] fixing another bug This particular fix is independent of the opacity work.
Attachment #133974 - Flags: superreview?(dbaron)
Attachment #133974 - Flags: review?(dbaron)
Attached image It works :-) (deleted) —
Tested in 16 & 32 bit color. The iframes seem to work properly.
Attachment #133970 - Flags: superreview?(bzbarsky)
Attachment #133970 - Flags: review?(ere)
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.
Attachment #133680 - Flags: superreview?(dbaron)
Attachment #133680 - Flags: review?(dbaron)
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....
Attachment #133970 - Flags: superreview?(bzbarsky) → superreview+
Attachment #133974 - Flags: superreview?(dbaron)
Attachment #133974 - Flags: superreview+
Attachment #133974 - Flags: review?(dbaron)
Attachment #133974 - Flags: review+
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 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
Attachment #133970 - Flags: review?(ere) → review+
Eh, I meant bottom-to-top with positive heights.
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.
*** Bug 223769 has been marked as a duplicate of this bug. ***
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 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, ...).
Attachment #133680 - Flags: superreview?(dbaron)
Attachment #133680 - Flags: superreview+
Attachment #133680 - Flags: review?(dbaron)
Attachment #133680 - Flags: review+
+ 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);
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
This seems to have increased pageload time on btek just a bit. 956 -> 971, with this being the only thing in the checkin window.
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?
I'm going to check in the other changes anyway. They're independent of how we fix the IFRAME thing.
checked in with all comments addressed.
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
Blocks: 136978
Oh, is our double-buffering always disabled on the Mac? Hmm...
Attached patch Reorg nsFrameFrame (obsolete) (deleted) — Splinter Review
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.
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.
Today's check-in could have caused bug 224478.
roc - could this have broken scrolling="no" on iframes? It broke between the 30th and 31 of october.
Sure it could have :-) bug number?
It's bug 224628, I guess.
Attached patch Decent nsFrameFrame reorg (obsolete) (deleted) — Splinter Review
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 on attachment 134756 [details] [diff] [review] Decent nsFrameFrame reorg oops, found another bug
Attachment #134756 - Attachment is obsolete: true
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.)
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.
True, maybe 5ms improvement, but not as much as the ~15ms decrease that bryner noted (and I noticed).
Attached patch nsFrameFrame reorg (deleted) — Splinter Review
Okay. This seems to work pretty well. No regression of "scrolling=no". And I actually diffed all the files this time :-).
Comment on attachment 134838 [details] [diff] [review] nsFrameFrame reorg Hello David, another big patch for you to review :-).
Attachment #134838 - Flags: superreview?(dbaron)
Attachment #134838 - Flags: review?(dbaron)
Re comment 60: the original inccrease was more like 10ms -- it went down the next cycle.
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 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 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.
Attached file iframe reflow testcase (deleted) —
Did you test printing and print preview of pages with iframes?
Blocks: 75739
Okay, that revealed some bugs :-). I'll revise the patch.
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...?
Attachment #134838 - Flags: superreview?(dbaron)
Attachment #134838 - Flags: review?(dbaron)
Marking fixed now that we've moved the rest of the work out of here.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.)
So is there a follow-up bug for whatever Mac problems we have here?
Ian, that second screenshot seems to show garbage at the top ("x-terminal emulator" etc). is that really there or am I misinterpreting?
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?
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.
It is OK, that this works correctly only in 16bit and up, but something should be done with bug 241939.
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: