Closed
Bug 212366
Opened 21 years ago
Closed 21 years ago
implement CSS3/SVG "group opacity"
Categories
(Core :: Web Painting, enhancement)
Core
Web Painting
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 :-).
Assignee | ||
Comment 1•21 years ago
|
||
I'll attach the patch tomorrow, I need to sleep now. But for now I'll attach a
testcase and a screenshot.
Assignee | ||
Comment 2•21 years ago
|
||
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).
Assignee | ||
Comment 3•21 years ago
|
||
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).
Assignee | ||
Comment 4•21 years ago
|
||
here it is
Assignee | ||
Updated•21 years ago
|
Attachment #127597 -
Flags: superreview?(dbaron)
Attachment #127597 -
Flags: review?(dbaron)
Assignee | ||
Comment 5•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
OK, sure.
Comment 8•21 years ago
|
||
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...?
Assignee | ||
Comment 9•21 years ago
|
||
> 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.
Assignee | ||
Comment 10•21 years ago
|
||
BTW if you could test this on Windows, I'd much appreciate it. Probably should
be tested on the Mac too.
Assignee | ||
Comment 11•21 years ago
|
||
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.
Assignee | ||
Comment 12•21 years ago
|
||
all as promised...
Attachment #127597 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #127597 -
Flags: superreview?(dbaron)
Attachment #127597 -
Flags: review?(dbaron)
Assignee | ||
Updated•21 years ago
|
Attachment #128209 -
Flags: superreview?(dbaron)
Attachment #128209 -
Flags: review?(dbaron)
Comment 13•21 years ago
|
||
I applied your patch on the Mac and got nothing looking anything like the
screenshot, in either cocoa (Camino) or carbon (Mozilla).
Comment 14•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Assignee | ||
Comment 17•21 years ago
|
||
oh dear. the Windows screenshot seems to indicate some sort of stride problem.
Of course I have no clue about the Mac.
Assignee | ||
Comment 18•21 years ago
|
||
Can you try this patch on Windows and attach a screenshot? Maybe I can figure
it out offline. Thanks!!!
Comment 19•21 years ago
|
||
Comment 20•21 years ago
|
||
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-
Assignee | ||
Comment 21•21 years ago
|
||
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
Using the updated patch.
roc - It works properly on both testcases now.
Assignee | ||
Comment 25•21 years ago
|
||
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•21 years ago
|
||
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).
Assignee | ||
Comment 27•21 years ago
|
||
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.
Assignee | ||
Comment 29•21 years ago
|
||
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.
Assignee | ||
Comment 30•21 years ago
|
||
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.
Assignee | ||
Comment 31•21 years ago
|
||
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)
Tested in 16 & 32 bit color. The iframes seem to work properly.
Assignee | ||
Updated•21 years ago
|
Attachment #133970 -
Flags: superreview?(bzbarsky)
Attachment #133970 -
Flags: review?(ere)
Assignee | ||
Comment 33•21 years ago
|
||
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 34•21 years ago
|
||
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+
Updated•21 years ago
|
Attachment #133974 -
Flags: superreview?(dbaron)
Attachment #133974 -
Flags: superreview+
Attachment #133974 -
Flags: review?(dbaron)
Attachment #133974 -
Flags: review+
Comment 35•21 years ago
|
||
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•21 years ago
|
||
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+
Comment 37•21 years ago
|
||
Eh, I meant bottom-to-top with positive heights.
Assignee | ||
Comment 38•21 years ago
|
||
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•21 years ago
|
||
*** Bug 223769 has been marked as a duplicate of this bug. ***
Comment 40•21 years ago
|
||
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•21 years ago
|
||
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+
Comment 42•21 years ago
|
||
+ 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•21 years ago
|
||
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•21 years ago
|
||
This seems to have increased pageload time on btek just a bit. 956 -> 971, with
this being the only thing in the checkin window.
Assignee | ||
Comment 45•21 years ago
|
||
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?
Assignee | ||
Comment 46•21 years ago
|
||
I'm going to check in the other changes anyway. They're independent of how we
fix the IFRAME thing.
Assignee | ||
Comment 47•21 years ago
|
||
checked in with all comments addressed.
Comment 48•21 years ago
|
||
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
Assignee | ||
Comment 49•21 years ago
|
||
Oh, is our double-buffering always disabled on the Mac? Hmm...
Assignee | ||
Comment 50•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Today's check-in could have caused bug 224478.
Comment 53•21 years ago
|
||
roc - could this have broken scrolling="no" on iframes? It broke between the
30th and 31 of october.
Assignee | ||
Comment 54•21 years ago
|
||
Sure it could have :-) bug number?
Comment 55•21 years ago
|
||
It's bug 224628, I guess.
Assignee | ||
Comment 56•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #134638 -
Attachment is obsolete: true
Assignee | ||
Comment 57•21 years ago
|
||
Comment on attachment 134756 [details] [diff] [review]
Decent nsFrameFrame reorg
oops, found another bug
Attachment #134756 -
Attachment is obsolete: true
Assignee | ||
Comment 58•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 60•21 years ago
|
||
True, maybe 5ms improvement, but not as much as the ~15ms decrease that bryner
noted (and I noticed).
Assignee | ||
Comment 61•21 years ago
|
||
Okay. This seems to work pretty well. No regression of "scrolling=no". And I
actually diffed all the files this time :-).
Assignee | ||
Comment 62•21 years ago
|
||
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)
Comment 63•21 years ago
|
||
Re comment 60: the original inccrease was more like 10ms -- it went down the
next cycle.
Assignee | ||
Comment 64•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Comment 67•21 years ago
|
||
Comment 68•21 years ago
|
||
Did you test printing and print preview of pages with iframes?
Assignee | ||
Comment 69•21 years ago
|
||
Okay, that revealed some bugs :-). I'll revise the patch.
Comment 70•21 years ago
|
||
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...?
Assignee | ||
Comment 71•21 years ago
|
||
Good idea. Filed bug 225820
Assignee | ||
Updated•21 years ago
|
Attachment #134838 -
Flags: superreview?(dbaron)
Attachment #134838 -
Flags: review?(dbaron)
Assignee | ||
Comment 72•21 years ago
|
||
Marking fixed now that we've moved the rest of the work out of here.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 73•21 years ago
|
||
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•21 years ago
|
||
So is there a follow-up bug for whatever Mac problems we have here?
Assignee | ||
Comment 75•21 years ago
|
||
Ian: screenshot?
Comment 76•21 years ago
|
||
Comment 77•21 years ago
|
||
Assignee | ||
Comment 78•21 years ago
|
||
Bug 228441 is the Mac problem.
Assignee | ||
Comment 79•21 years ago
|
||
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•21 years ago
|
||
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?
Assignee | ||
Comment 81•21 years ago
|
||
I filed it as bug 228861. Thanks!
Comment 82•21 years ago
|
||
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•21 years ago
|
||
It is OK, that this works correctly only in 16bit and up, but something should
be done with bug 241939.
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•