Closed Bug 39621 Opened 24 years ago Closed 23 years ago

Need to turn on nsViewManager by default

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: andrew.ng, Assigned: roc)

References

Details

(Keywords: css2, perf, testcase, Whiteboard: (py8ieh:need testcase) WG @ 2001-01-14 10:45)

Attachments

(11 files)

Overhaul of nsViewManager2
119.41 KB, patch
Details | Diff | Splinter Review
Overhaul of nsViewManager2: revision 2
121.20 KB, patch
Details | Diff | Splinter Review
Infrastructure patch; interface changes only, no behaviour changes expected
35.46 KB, patch
Details | Diff | Splinter Review
Translucent fixed element is not clipped correctly
2.72 KB, text/html
Details
Infrastructure patch v2: small changes to nsHTMLContainerFrame.cpp
37.32 KB, patch
Details | Diff | Splinter Review
Infrastructure patch v3: Additional comment
37.64 KB, patch
Details | Diff | Splinter Review
Patch to nsViewManager.cpp/nsViewManager.h; spiffy new code for z-ordering/clipping/etc
181.72 KB, patch
Details | Diff | Splinter Review
Patch to build and use nsViewManager instead of nsViewManager2
1.76 KB, patch
Details | Diff | Splinter Review
Patch to build nsViewManager and enable it via a pref
2.57 KB, patch
Details | Diff | Splinter Review
Patch to turn on new view manager by default
578 bytes, patch
Details | Diff | Splinter Review
add ifdef DEBUG
823 bytes, patch
Details | Diff | Splinter Review 
Whenever overflow attribute is used within a <DIV>, z-index doesn't work.

Example: z-index works w/o overflow
<html>
<body>
<DIV style="background-color:red; HEIGHT:28px; WIDTH:100px; position:absolute;
top:10px; left:10px; z-index:1"></DIV>
<DIV style="background-color:blue; HEIGHT:14px; WIDTH:100px; position:absolute;
top:10px; left:10px; z-index:5"></DIV>
</html>

Example: z-index doesn't work with overflow
<html>
<body>
<DIV style="background-color:red; HEIGHT:28px; WIDTH:100px; position:absolute;
top:10px; left:10px; z-index:1"></DIV>
<DIV style="background-color:blue; HEIGHT:14px; WIDTH:100px; position:absolute;
top:10px; left:10px; z-index:5; overflow:auto"></DIV>
</html>
*** Bug 39652 has been marked as a duplicate of this bug. ***
Changing component to Layout, Steve, do you know who this belongs to?
Assignee: jst → buster
Component: DOM Level 2 → Layout
me  :(
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Should this be linked to tracking bug 38639?  Does it happen only with GFX
scrollbars?
What would be the possible target milestone for the fix of this bug?  Sorry for 
being impatient :)
This looks like a combination of kevin's task to remove native widgets from 
scroll views, and eric's work to make gfx scrollbars play nice with positioned 
elements.  I'll add it to 38639 as well.
kevin, milestone?
Assignee: buster → kmcclusk
Status: ASSIGNED → NEW
Blocks: 38639
This can not be solved unless we remove the widget used to clip ScrollFrames. I 
have changes which will allow z-index and clipping to work with html form 
elements, but I do not have a general solution for removing the widget used by 
all ScrollFrames. The problem is that we would still need to create clip widgets  
if the content of the scrollframe contained a widget. Applets and plugins always 
create widgets. We also create widgets for fixed positioned elements. We also 
have to add code to bitblit the content of the ScrollFrame when scrolling. 
Currently it repositions the view and repaints when a clip widget is not 
present.

This problem will not be easy to solve, so It will probably not be fixed for the 
first release.
Status: NEW → ASSIGNED
This bug has been marked "future" because at this time it has been determined 
that it is not absolutely critical for RTM (Release To Manufacturing). If the 
reporter and anyone else believe it is necessary to fix this before shipping 
Seamonkey 1.0, please describe your issue in the bug.
Target Milestone: --- → Future
Kevin, totally understand about your being swamped. I'm concerned about this
one, however. If we leave things as is, we've rendered the use of z-index and
overflow mutually exclusive for a long, long time on the Web in content that's
being made Gecko-friendly. That's a classic example of rendering a combination
of features unusable on the web because one browser behaves badly.

If we can't get the fundamental issue fixed for RTM, it would be better to hack
RTM Gecko so that if z-index is used, overflow is silently ignored. That way,
we'd have a bug in failing to respect overflow on elements with z-index set, but
at least we wouldn't prevent web content developers from setting both properties
on the same element in their content.
 If we have to live with bugs for FCS, so be it, but we want to avoid rendering
the properties unusable on the web if at all possible.

How hard would my proposed temporary tourniquet be to implement?

Nominating nsbeta2,nsbeta3 for tracking; recc. nsbeta2 [some lenient date-] and
ditto for nsbeta3. cc:ing dbaron for any further thoughts.
Keywords: nsbeta2, nsbeta3
Putting on [nsbeta2-] radar. Will consider for nsbeta3.
Whiteboard: [nsbeta2-]
Eric: I like your suggestion.. We could treat overflow:auto as overflow:clip if 
z-index has been applied. (That way the author would visually get what they 
expect.. it just wouldn't scroll) This would need to be done in the style 
system.

Mark, how difficult would this be?

This would be pretty easy for the static case (simple addition of logic to 
MapDeclarationInto in nsCSSStyleSheet) however I am not sure if DOM updates 
would be covered there - probably not. That might be a much harder problem to 
solve; I'd have to dig in to even know what happens there. If that is not 
important, however, we can fixup the overflow value when zindex is set in a few 
minutes (noting that this is a temporary fix with profuse documentation, of 
course).
A better approach may be to suppress the creation of the clip widget for 
scrolling frames when z-index has been set. This should allow z-index to work 
with overflow:auto with the limitation that any content within the scrolling DIV 
will not be clipped if the content's frame create's a widget. This would happen 
if the DIV contained applets, plugins, iframes or fixed position elements.

This would not require any changes to the style system. 
Only problem is IFrames always have widgets. We really need to fix this problem. 
What is the status of getting the widgets out of iframes? If that happened we 
could punt on clipping applets and plugins or we could put some hack just to 
clip them in the html content area.
I think Eric Krock's proposed workaround would make the behavior worse rather 
than better.  There is some level of acceptance of problems caused by widgets - 
they exist in other browsers too (but others are working to get rid of them 
too).  On the other hand, making content that should be scrollable not 
scrollable would prevent that content from being accessible.
I think this can be solved without dewidgetifying the scroll frames. Instead,
I've rewritten the nsViewManager2 display list code so that whenever a view is
repainted, we paint all visible content even when it comes from views in
unrelated parts of the view tree. This also addresses bug 27180 (problems with
transparent views) and a lot of the z-index bugs. So far things are proceeding
remarkably well. Hopefully I will have a patch ready soon.

[One of the hardest things was getting "position: fixed" views into the correct
Z-order; they need to get their Z-order context from a different view than their
geometric view parent. They also need to be clipped by that parent instead of
the geometric parent. To get this to work I had to extend the view interfaces a
little bit, so that layout can tell the view manager about this alternative
parentage.]
Robert: Have you read the CSS spec's z-index rules? (Notably the business about
how z-index:auto does *not* start a new stacking context and so on.)

vidur: Per meeting with ChrisD, taking QA, since this is a z-index CSS bug.
However, if you want to be QA contact that is fine by me!
Keywords: correctness
QA Contact: vidur → py8ieh=bugzilla
Whiteboard: [nsbeta2-] → [nsbeta2-] (py8ieh:need testcase)
Of course, "z-index: auto" is what makes it all interesting. I pretty much have 
it solved, although the real problem is that the possibility of weirdly 
z-indexed children lurking at the bottom of the view tree means we have to be 
rather conservative when we create the display list, and when we request 
repaints of widget-bearing views.
Cool. I just thought I would check, because I wouldn't want your hard work to
be for nothing just because I forgot to point out the spec's madness! ;-)
I have a question, actually. In the Viewer example "test11.html", isn't the 
fixed-position DIV out of the flow? And if it is, shouldn't the rest of the 
content be laid out right over it, giving a rather odd-looking rendering where 
the first line of text and the heading "Introduction" are z-positioned under the 
fixed-position element, and the rest of the text is z-positioned over it?

That is what I'm aiming for, but if this is what the spec really means, then 
it's going to break a lot of pages.
Implementing the spec on z-index will break a lot of pages.  I've said before
that it needed to be discussed by the WG -- I thought Troy was going to bring it
up but I don't think he did.
Whiteboard: [nsbeta2-] (py8ieh:need testcase) → [nsbeta2-] (py8ieh:need testcase) Must contact WG.
Well, I'll implement the spec and we can see how bad it is. The results might 
interest the WG. It would not be hard to add in alternative behaviours (perhaps 
controlled by quirks), such as putting positioned frames on top of normal frames 
when they both have the same z-order, and/or putting fixed-position frames on 
top of all others.

The spec'ed behaviour is quite cool though. It enables some nifty effects.
Yes, most of the strange spec behaviours are great for special effects. But 
Microsoft (and NS to a lesser extent) went and ruined that by implementing their
own thing, and guess which we will probably end up supporting... :-/

The suggestion of having absolutely-positioned elements treat "auto" as "0" 
sounds good, and is easy to implement. But fixed DIVs without an explicit 
z-index are still going to be rendered underneath non-fixed content (unless the 
author happened to put the fixed DIVs as the last elements, which does not 
always seem to be the case).

I'm really looking forward to seeing what this looks like, actually :-).
Robert: See bug 7774.

Also, since you are working on this, and Kevin is not going to be working on
this until at least after we ship NS6, would you like to assign the bug to
yourself? 

Sure.
Assignee: kmcclusk → roc+moz
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
An unforseen evil: if a frame has a view, then all its siblings also need views, 
to make sure that the implicit z-order between sibling content nodes is obeyed.

I think a very good suggestion to the WG would be to relax the z-order rules so 
that sibling content nodes with the same z-index can be z-ordered arbitrarily by 
the user agent (rather than in content order as is currently required). This 
relaxation gives the user agent a lot of flexibility in the common case, but 
authors can still assign the z-order of everything explicitly if they want to. 
It also means that the common behaviour of absolutely-positioned elements 
floating over their siblings (in the absence of z-index directives) would become 
conformant.

If this isn't relaxed, we're in a world of pain. Apart from needing zillions of 
views all over the place, it's a huge burden to figure out how the views should 
be ordered based on their originating content. Frame sibling order would often 
be right, but I see no reason why it should always be right, and therefore it 
surely won't be...
Well, apart from the content-implicit z-order, which I'm not going to touch, 
the rest is in pretty good shape. Everything seems to work, scrolling and 
updates too. There are a few bugs that need to be ironed out and other 
polishing to do, but I haven't found any serious regressions against content 
that doesn't use z-index or transparency. So I think this approach is sound.
I've done some major hacking on nsViewManager2 and it seems to work well. It 
fixes this bug and a number of others. The highlights are
-- "z-index: auto" works according to spec
-- "position: fixed" elements are clipped and z-ordered according to their 
parent in the content tree, not the viewport
-- Translucent positioned elements are translucent
-- Translucent elements are properly clipped
-- Transparent and translucent elements update properly when content underneath 
them is scrolled or modified

Basically, translucency and transparency work now whereas before they didn't 
:-).

Various other little bugs have been fixed ... and some introduced, no doubt. I 
think I've cleaned up nsViewManager2 significantly. I got rid of a fair bit of 
cruft (e.g. I got rid of 1 of the 2 nsRects in DisplayListElement2 ... I 
couldn't figure out what the difference was, so I just made them the same and 
everything worked).

The bad news is that I've changed a lot of code so there are probably some 
regressions. Also, there may be platform issues: I only changed XP code, but 
there may be platform-specific issues regarding widgets and rendering contexts. 

Also, there will be some performance impact, different by platform: for me on 
Windows, it doesn't feel any slower, but I don't have numbers. Fixing these bugs 
does mean doing more work, even in the general case --- for example, you never 
know a priori when some view way down in the view tree, backed by some other 
widget, has its z-index set so that it needs to draw on top of your widget. On 
the other hand, I also implemented some display list optimizations that weren't 
there before.

I'll attach the patch shortly. Let me know if there's anything else I can do to 
help get it in.
Robert -- many kudos for your hard work! Only catch: new regressions are the one 
thing we *don't* need right now. Is there any way you and folks on this bug 
could recruit volunteers to run with your patch to test it and evaluate how 
many/whether there are regressions? Feel free to recruit volunteers on the 
mozilla newsgroups. We need bulletproof patches at this point to avoid slipping 
the product ship date.
I understand. I agree that this should be thoroughly tested before it goes in.

How about I split the patch into two parts: one part with all the changes except 
for the changes to nsViewManager2, and the rest. The first part is basically 
just a few new functions in nsIView.h/nsIViewManager.h, do-nothing stubs for 
their implementations, and calls to those APIs. This part should be low risk and 
easy to review. If we can check that in, then the rest of the patch --- the real 
changes to nsViewManager2 --- would be much easier to keep up to date with the 
tip, for me and for anyone who's testing it.

I will try to recruit some volunteers to test this.
See attachment
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=12074
Scrolling works, and it's quite fast: I use fast native scrolling for the main 
window and only repaint the child widgets. There's still visible tearing while 
scrolling, but I don't think that can be solved without excising native widgets, 
which would be much more difficult and severe than what I've done.
I tryed this on linux:

In editor, mousing over menubar is not higlighting all menus, just some, and
some menus stick hilight.

Tooltips on bottom of window are not working, they are not drawn at all, or
they are drawn with content from previous tooltip. Tooltips on personal
toolbar works ok.
There was a bug related to the fact that, unlike all other views, views on 
popups are not necesssarily within the bounds of their parent view. Popups such 
as menus and tooltips would be clipped to the bounds of the main window. I've 
fixed the bug and will attach a new patch.

I can't reproduce any problems with menu highlighting on mouseover. Which skin 
are you using, modern or classic?
The previous patch 12225 contains the interface change part of my nsViewManager2 
overhaul. It touches several files, but it's not large and it should be 
harmless. If this could be checked in, it would be much easier to maintain my 
nsViewManager2 changes, and it would also be easier for people to test my 
changes.

The main change here is that you can now tell a view to get its Z-index and 
clipping parentage from some view other than its geometric parent. I modify 
layout to do that for fixed position views. The trunk nsViewManager2 just 
ignores this information. I also add a method whereby a view can notify its view 
manager that it has just been scrolled, so the view manager can update any 
transparent/translucent covering widgets. Trunk nsViewManager2 just ignores that 
too.

Pleeeeease :-).
Keywords: patch
Robert: Do you have r= ?
No, I haven't heard from anyone.

Ian, I'm wondering about the intended behaviour of "position: fixed" elements. I 
have them taking their Z-order and clipping from the in-flow parent. Do you 
think that is correct, or should they be taking those from the viewport? I 
scanned the CSS2 spec and I couldn't see any language to indicate that they got 
anything but their geometry from the viewport ... but I'm not as familiar with 
the spec as some other people :-).
Blocks: 33593
Blocks: 14691
Blocks: 4209
Blocks: 27180
Blocks: 7774
Kevin: Could you please review the two patches -- first:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=12225
...which is the infrastructure patch, which we'd like to check in first, and
then the second, complete patch:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=12172
...which we hope to check in assuming the infrastructure patch causes no bugs
of its own.

Thanks.


Chris Waterson: Assuming Kevin gives an r= and, as module owner, okays the 
checkin, could you look these patches over and give a=? We'd like to check this
in in two phases, first the infrastructure, and then the new bits. (Otherwise
keeping the patches in sync with the tip will be a lot harder.) Note that this
is a big change, but that is because it actually fixes at least four major bugs
and lays the groundwork for a fifth. These are the CSS2 compliance bugs that
we briefly discussed this afternoon -- we'd like to get these fixed, Netscape
does not have the resources, and Robert has done the work already.

If this is checked in it should go in as soon as possible so that QA can run all
the regression tests. This _is_ quite a major change, which implies risk, so it 
would be a bad idea to check this in at the end of the beta3 cycle...
Keywords: approval, css2, review, testcase
OS: Windows NT → All
Hardware: PC → All
Target Milestone: Future → M18
I found some more bugs related to fixed positioning that may depend on this 
(i.e. they're unresolved in Bugzilla but work in my tree).

There are other problems fixed by this patch that I can't find in Bugzilla. In 
particular, the trunk nsViewManager2 clips and Z-orders fixed-position elements 
using the viewport as their parent view, which is (according to Ian) incorrect. 
Also, the trunk nsViewManager2 completely fails to clip translucent views in any 
reasonable way. (All views are rendered and composited into the translucent area 
without any clipping, and then the resulting pixels are copied to the real 
rendering context, clipped to the clipping region of the last translucent view.) 
I'll attach a testcase covering these issues. The catch is, of course, that the 
bugs already filed tend to screw up the rendering so no-one can see that these 
other bugs actually exist too :-).
With rev2 patch tooltips works ok, and i dont see problem with menus anymore.
Menuproblem hapened when openned mozilla and then clicked taskbar to open
editor, then editor menus was broken. This was on modern skin.
Is someone reviewing Robert's infrastructure patch? Would be nice, really :)

If some people are really scared of possible regressions, what about the
following procedure to minimize risks:
1. Get the infrastructure patch reviewed, approved, and checked in.
2. Wait two days or so watching the incoming bugs to be sure there are no    
   regressions from this.
3. One day, produce two nearly identical versions of "nighlies": One version 
   that has the real patch in it and one without. E.g. make the first builds
   as usual at 8 am without the real patch, then check in the real patch, make
   a second series of builds at 10 am with the real patch, then back out the 
   real patch and open the tree.
4. Announce these versions on the newsgroups, mozillazine, irc etc. to get as 
   many testers as possible to compare these two nightlies.
5. If no serious bugs show up within a week, check the real fix into the tree.

Well, just an idea...
This is not that big a deal. All we need are a few people to test it out and 
make sure that nothing obvious breaks on their platform. So far me and Tomi are 
saying that's true for Windows and Linux. It would be nice to check on the Mac 
too.

Few people can write as much code as I did and get it 100% correct, so to be 
honest there probably are a few new bugs there, but the fact is that it fixes a 
number of serious problems. So far two people have tested it and we've found 
just one new bug, which I've already fixed, and we haven't found any new bugs in 
the revised patch. So I'll be very disappointed if it ends up causing more 
problems than it solves.

I don't really know why this is being treated so gingerly. Maybe I was too 
honest in admitting that the code may not be 100% correct :-).
I think nscp people are just swamped with nsbeta3 worries. I'll try running with 
your patch, and have poked kmmclusk to take a look at it.

With respect to the "infrastructure patch" to nsHTMLContainerFrame, the only 
thing that frightens me is this:

+          if (nsnull != aContentParentFrame) {
+            nsIView* zParentView = parentView;
+            
+            aContentParentFrame->GetView(aPresContext, &zParentView);

nsFrame::GetView() *always* initializes the out parameter to null,

http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrame.cpp#1805

So I'm guessing that the initial assignment to zParentView is effectively going 
to be a no-op (because GetView() will always clobber it). Is that what you 
expected? Or is there something more clever going on here? (In which case, a 
comment would be nice... ;-)
I understand "swamped" :-). I just don't want the checkin procedure for this fix 
to get more complicated than it needs to be.

As for that snippet of code: I'm just being conservative. For any method that 
could fail (i.e. returns an nsresult), I either check the result or I assume the 
output parameter may not be initialized. I deliberately ignore the 
implementation of the method. Isn't that the whole point of modular programming?
Now I see that just below that piece of code, I failed to initialize 
zParentFrame. That's because I copied that block from existing code which has 
the same problem:

      nsIFrame* parent;
      aFrame->GetParentWithView(aPresContext, &parent);
      NS_ASSERTION(parent, "GetParentWithView failed");

I'll attach a revised patch.

I also noticed recently that "z-index" can only apply to positioned elements, 
for which we always create views anyway, so I'll take out the code that 
explicitly creates a view for any element with "z-index".
Hmm. If this is supposed to be defensive, then initialize zParentView to null, 
not some other random view.

If it's actually doing optimistic initialization (under the assumption that the 
implementation of GetView() will leave the out parameter untouched), then your 
code is buggy, because at least one frame (nsFrame) will always clobber 
the out parameter's value.

If it's doing something else, explain with a comment in the code. ("GetView() 
will leave its 'out' parameter alone at the witching hour of the first tuesday 
of each month. In that case, we're ready to go with this other view that fell 
from space.")

Robert thanks for all your hard work on this, but I'm afraid these patches are 
so extensive I can't give thumbs up. The risk is too high. I think we should try 
and get the changes in after RTM.  This is basically a rewrite of most of the 
viewmanager. 
I think if we have enough people testing these changes in the next few weeks I
really hope we could get these in for RTM.  These changes fix a large number of
the existing bugs in our CSS2 support (bugs, not missing features).

If this is really a rewrite of the view manager, should it be checked in as
nsViewManager3.cpp?  I would really like to be able to test this code without
making significant modifications to existing files in my tree -- I have enough
trouble keeping track of the stuff I have modified...
Whether this gets checked in as nsViewManager2, nsViewManager3, or not at all, 
please at least check in the infrastructure patch. Then it will be possible to 
do the nsViewManager3 thing, easier for people to test without losing track of 
all the changes in their tree, and easier for me to maintain if I have to keep 
it alive in my tree till beyond RTM.

PS, it's about time someone came up with a version control system that didn't 
suck quite so much as CVS. But that's another topic.
The infrastrastructure patch looks fine, and I agree that it should go in. 
r,a=waterson

And I like dbaron's suggestion of making nsViewManager3.cpp, if this really is a 
complete rewrite. kmcclusk, how do you feel about that?
Thanks. I'd appreciate it if someone else could check in the infrastructure 
patch, I don't want to take the risk of checking in any of the wrong changes.
Just an idea, how about #ifdef NEW_VIEWCODE  or something like
that around new code and then check it in? Would be much easyer to
test and maintain new patches.

Also patch seems to give bit more speed to menus on linux, i think this
is becouse backingstore is used.

The infrastructure patch looks fine.

I don't think creating a viewmanager3 will work. The changes involve many more 
files than just viewmnager2. nsCssFrameConstructor, View, Scrolling view + lots 
of frame classes.

Once the infrastructure patch is applied, all the remaining changes are to 
nsViewManager2.cpp and nsViewManager2.h. (The "overhaul" patches included the 
infrastructure changes.) So we could indeed create an nsViewManager3.

(OK, there is some dead code in nsView.cpp that can be removed, but we can leave 
that in indefinitely if it's a concern.)
I would place the code in nsViewManager.cpp,.h instead of nsViewManager3.cpp, 
Since we would end up loosing all of the CSS log information when nsViewManager3 
was created anyways. nsViewManager is obsolete has been removed from the build 
on WIN32, Linux, and Mac.

You can go ahead and checkin the code in nsViewManager.cpp.
OK, that sounds like a good plan. If waterson approves, I will move my changes 
to nsViewManager and check them in along with the infrastructure patch. Then 
enabling the new code in someone's tree will only require a small patch to the 
makefiles and nsViewFactory.cpp.
Or we could just hook up the ViewManager checkbox in the debug pane again...
Robert: Have you got anywhere with checking in the new nsViewManager ? I wouldn't
want your patch to bitrot in your tree! ;-)
Right now, I am working on getting the infrastructure changes checked in 
for which I already have approval. Should be RSN.

Once that's done, I will submit a new patch to nsViewManager.cpp/nsViewManager.h 
to get the real changes into the tree. I guess this will have to be re-reviewed 
and approved but I guess that won't be a big deal since the files aren't build 
now anyway.

Once that's in, I will produce another patch that enables building and using 
this new view manager for those who wish to test it out.
The infrastructure patch is in.

I will attach two more patches. One is a set of changes to 
nsViewManager.cpp/nsViewManager.h. I hope this can be checked into the tree, for 
which I'll need review and approval, please :-).

The other patch is to build and use nsViewManager instead of nsViewManager2. 
This is for people who want to test out the new code. This should not, of 
course, be checked in.

If necessary I can produce an alternative second patch that introduces a pref to 
choose which implementation to use, but then we'd have to commit to building my 
nsViewManager on the trunk.
Hey, can I have review and approval for patch 12852 please?

Actually, it would be convenient if I could get blanket approval to stomp all
over nsViewManager.h and nsViewManager.cpp until they're part of the build
again. Then I could more easily track changes to nsViewManager2.

PS, did any of you guys get around to trying this code yet?
Guys, can I have some cycles here? Especially from those of you who actually 
want this patch :-).
Robert: you can go ahead and replace nsViewManager.cpp and nsViewManager.h with 
your new viewmanager implementation
Thanks. I believe I also need mozilla.org approval. Waterson?
a=waterson
Thanks. Unless otherwise advised, I will interpret that review/approval to mean 
I can continue updating nsViewManager to keep it in sync with nsViewManager2 
:-).
Yay, yet another rewrite. I agree with the suggestion that it go in as 
nsViewManager3.cpp.

When the tree opens, I will check in changes that bring nsViewManager up to 
speed with the latest changes to nsViewManager2. These changes also include some 
"last gasp" backstop rendering code that is hardly ever invoked, but prevents us 
from rendering garbage if someone manages to construct a document that doesn't 
actually cover its area with opaque pixels.

Patch 12853 is still the patch you want to use to turn on the new nsViewManager, 
for now.
Update checked in. Give it a try :-).
There is one small problem with this. Now when there is some bugs in modern skin
that makes default "Bookmars" -menu to grow when mousing over it ( "AAA Urls
with redirection" gets longer and whole menu grows). After closing  Bookmarks
menu, area under it appears grey and its not repainted properly.



It works OK on Windows.

Does this actually work OK with the old nsViewManager2? What if the popup is 
hanging over some other application's window?
Summary: problems with z-index and overflow:auto → problems with z-index and overflow:auto [POS]
I don't think my fix can possibly make NS6. The soonest it can land is after 
Netscape branches, post-beta3. So, you guys had better figure out which parts of 
CSS2 you're going to disable for NS6.
I think this should be in, at least on linux. Mailnews sucks bigtime without
this (3-panel repaint all wrong when resizing panels).

I have run with this long time, and havent seen any problems for a while and
this fixes so many problems.
Marking with perf keyword because of previous comment that this improves 
performance of 3-pane mail window on Linux. cc:ing kmurray.
Keywords: perf
FWIW, I spent a few hours browsing with this patch enabled on a PC/Linux debug
build from 9/2, and all the bugs I encountered turned out to be in the regular
Mozilla version as well.
Anyway, Mozilla nightlies will probably contain this patch before NS6 is out...

BTW, I suspect that the perf thing is a misunderstanding. Tomi?

This patch gives performance boos on menus, becouse it know when use backing
store and not redraw those areas.  I am not sure is there performance boost
on 3-panel, but at least it gets drawn right.
Rubber stamping [nsbeta3-], since Robert's fix is slated for the next release
do to high risk considerations.

Robert: Since you know what you fixed with this bug, could you file a new bug
on disabling the broken bits for beta3/rtm? Thanks!
Whiteboard: [nsbeta2-] (py8ieh:need testcase) Must contact WG. → [nsbeta2-] (py8ieh:need testcase) Must contact WG. [nsbeta3-]
Blocks: 55104
I've been trying this patch out on a debug build and it seems excellent.
Scrolling is zippy and I haven't had any rendering problems with the popups in
the mail/news compose window like I usually do.

Has anyone else tried this on the mac? ( adding scc )
Blizzard, if you can see that this fixes specific bugs, please add dependencies.

I would like to get the preference patch checked in on the trunk. Kevin, can 
you r=?
You might want to remove the annoying printf() that's checked into the view
manager code.  It's not ifdef'ed or anything.  I've been running this in an
optimized build now for a while and it's working great.
i'll try this out on mac once my trunk build finishes.
ok, it builds and runs on mac, but i don't notice any improvements. How do i
test that this patch actually changes anything?
Patch to enable new viewmanager via pref looks good.
r=kmcclusk@netscape.com
sr=blizzard on the pref patch.

roc, we need someone with a mac to check this in since we have to have view.mcp
changed.  Find me on irc and we'll talk about trying to find someone.
patch and mac project checked in
roc, since this is checked in do you want to close this bug or do you want to
hold it open until after the new view manager code is on by default?  We should
probably start filing bugs on specific problems that we have with the new view
manager code.
My $.02: it's right to hold the bug open until the code is turned on by default.
 Having the code checked in is nice, but the bug isn't fixed until folks
downloading the nightlies see the benefit without having to jump through hoops
(set a magic pref.)
Playing with it on a trunk build right now -- first impressions are good!

Thanks scc.

Let's leave this bug open until nsViewManager is turned on by default. People 
can add "status whiteboard" junk to indicate "fixed by nsViewManager", and file 
new bugs against me if they find problems.

There are major pieces of work that still need to be done in the view manager:
-- Allow views to extend above and to the left of their associated frames (bug 
13213)
-- Fix opacity model to conform to SVG spec, making opacity actually usable for 
complex content
-- Rip event handling out of nsView and into nsViewManager, make mouse event 
handling obey z-index, hand all frame events off to nsIViewManagerObserver all 
in one go to avoid problems caused by the view manager being destroyed in the 
event handler

I plan to address these. I would like to keep the new view manager turned off by 
default until these big changes are all in and working, so we can land and test 
these changes without too much pain.
roc, do you want to open bugs on all those issues so that we can track them?
ViewManager3 does indeed seem to fix the test cases.  :)

Unfortunately I have seen numerous redraw problems with
this enabled though -- mostly problems redrawing the toolbars
(the areas of toolbar exposed when drop-downs disappear are
not redrawn; not constantly reproducable, but happens on
Linux and Solaris), and one instance of viewing an image
where the image would repaint but the remaining 'blank'
background of the viewing area would not (no scrollbar
either, when there should have been).

Really?  The problem where areas under a menu aren't painted goes away for me
with the new view manager code.
mm, yeah, really really...  :(

I've not ever (well, not for six months or so)
seen this problem until recently when I switched to the
new view manager.  When I reverted back to vanilla ViewManager2
again the problem went away.

Same thing on Solaris.

You're using the pref in all.js, right?
Yes indeed, with the stdout-spam to prove it.  :)

I think I saw some problems like the ones Adam mentioned when I was running with
ViewManager3 on my old machine, but I don't see them on my new one.  (Perhaps
it's related to GTK+ version or something?  I have 1.2.8 here.  I think I had
1.2.6 on my old machine.)
On my Linux box I have GTK 1.2.9 here (ie. the latest
from the GTK 1.2.x CVS tree).  On the Solaris box at
work I have a pretty old GTK 1.2, but since I'm using
the nightly builds on that machine instead of rolling
my own I think those are statically linked anyway.

When popups are hidden (e.g. on dismissal), the old view manager would 
explicitly repaint the area underneath the popup. I removed this code because 
the window system itself should refresh the area covered by the popup. (It can 
often to do much more efficiently than we can, e.g. using save-under buffers, 
and it has to do the right thing anyway in the case where we don't own the 
content under the popup.)

It sounds like this may have exposed some other bug.
Well then, that's what it's doing -- exposing another
bug.  I didn't know there used to be code in there to
explicitly redraw the underlying area -- how horrid!

Nominating for 1.0
Adding self to CC
Keywords: mozilla1.0
I'm now running Linux and I can see the redrawing bug. It's hard to reproduce
though.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
roc, can you reproduce the problem that generates all of the "XXX: Using final
transparent rect, x=0, y=9870, width=13396, height=1" messages?
Yes I can. I'll try to track it down.
Oops, how did I accidentally set it to RESOLVED?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
roc, any luck in tracking down that problem?
I think we should have this turned on by default before 1.0, so I'm adding
mozilla0.8 keyword.
As we are at keywords, I don't want to touch that, but roc or Hixie, wouldn't it
be good to clean up status whiteboard and keywords a bit?
Keywords: mozilla0.8
I think the WG issues is cleared up (spec doesn't say anything about clipping
or z-ordering w.r.t. viewport, do it w.r.t. parents).
Keywords: nsbeta2, nsbeta3 → nsbeta1
Whiteboard: [nsbeta2-] (py8ieh:need testcase) Must contact WG. [nsbeta3-] → (py8ieh:need testcase)
I think there is still a WG issue: I think the spec should not contain 
the requirement that, in the absence of "z-index", all content must be z-ordered 
in document order. This seems very difficult to implement (makes a lot of 
optimizations difficult), conflicts with other requirements (e.g. text-shadow), 
and doesn't seem very useful to authors (who can put in explicit z-index 
declarations wherever they need to).
ok (which way have you implemented it now, btw?)
Whiteboard: (py8ieh:need testcase) → (py8ieh:need testcase) WG @ 2001-01-14 10:45
My patch deals with z-index but doesn't change anything in the absence of 
z-index. Right now, in the absence of z-index, we generally do thing in document 
order, but not always. In particular, positioned elements are always drawn after 
in-flow elements, and I think floats are always drawn after in-flow elements 
too.
Well, I'd think we could fix 'auto' by not creating views for positioned
elements with 'z-index: auto'.  I'm not sure what that would break, though
(both in Mozilla and in existing Web content).
Target Milestone: M18 → ---
I would like to get this turned on soon. Changing summary and setting milestone.

I have added depends-on bugs for the known regressions introduced by the new
view manager. I believe that the painting problems in Linux discussed here are
covered in bug 69346.
No longer blocks: 68499
Depends on: 69146, 69346
Keywords: mozilla0.8, mozilla1.0, nsbeta1
Summary: problems with z-index and overflow:auto [POS] → Need to turn on nsViewManager by default
Target Milestone: --- → mozilla0.9
OK, I have fixes for both of the depends-on bugs, so we're looking good.
roc, what do you need to get those fixes in?
Review and super-review from kmmclusk and attinasi. But I only just put the 
patches there so don't start breaking any kneecaps.
I ran this with the page loading test on win98, linux rh6.1 and macos9.0
on [reasonably fast machines with lots of memory]. I was testing the
commercial optimized build for 20010220 with 

  user_pref("nglayout.debug.enable_scary_view_manager", true);

I checked that I had the debug printf's coming out on windows and linux, but
I ran the test with no console on win32, and '> /dev/null' on linux. The skin
was modern, sidebar was collapsed, new profile, app had been restarted 3 times.

The results on win98 and linux are basically 'no change' with this pref 
enabled. On Mac, there may be a small performance hit of ~5%, but I really
need to run this again to see if this is reproducible. Will run tomorrow 
evening.

Who else has been running with this on? I plan to spend a few days running with
the new view manager (after applying the related patches), but I'd be interested
in knowing how much excercise it has gotten so far... I will need through this
week to check it out.
I run with this on from time to time and I've never seen any bad results from it.
I've been using it for all of my browsing for at least a month or two (on
Linux).
I've been running this every day for about 4 months now... no
noticable problems remaining.

which patch should i apply to run this with jrgm's tests? the 5% mac slowdown 
scares the bejesus out of me.

No patch, just
user_pref("nglayout.debug.enable_scary_view_manager", true);
I've been using it every day on linux for about 3.5 months (since Nov 7, to be
exact).  No noticeable problems, and no noticeable slowdowns compared to the old
viewmanager.
I haven't seen any slow downs with the new view manager.  Are you sure that you
aren't getting slowed down by the debugging messages that should be fixed soonish?
I only saw a difference on Mac, of ~5%. At that level, I know "something" 
happened. But it may have been something that is not related to the single
change in test setup (setting the 'scary' pref). In particular, Mac, and 
the way that Mac is configured (VM on, File Sharing on) has been prone to 
throwing out some odd results. I will check again this evening (and perhaps
Mike will check as well). 
It's plausible that it could slow down a little bit on the Mac compared to other 
platforms. I've significantly changed the mix of platform widget/GFX operations 
that we perform. In general we traverse more of the view hierarchy than we did 
before, potentially doing more painting, but subject to a number of 
optimizations that in practice limit the extra work required. Some of those 
optimizations require more intense region computation than we were doing before.  
(But the Mac is reputed to have a good region implementation so I doubt that's a 
problem.)

If the new view manager turns out to be a little bit slower, then I'm not sure 
what to do about it. I suppose I could find some optimizations elsewhere that 
speed the Mac up by 5% and then make a deal :-).
I just ran the tests and the numbers look good on mac. G4 450, 2/19/01 opt
commercial build:

current view manager:
- test 1: 3700 ms
- test 2: 3896 ms

roc's view manager:
- test 3: 3766 ms
- test 4: 3690 ms

So I don't see a 5% speed hit for mac. If anything, it's maybe a smidge better.
I've been testing new "scary" nsViewManager for about 2 months so far and saw no
problems - but saw it fixing my "favourite" bug (4209) :)

(linux)
I am currently reviewing the code in nsViewManager.cpp. I should be done today 
or tommorow.
Robert: Excellent work! 
I don't see any obvious reasons why we shouldn't turn the new viewmanager on. 
When you turn it on could you post a message to relevant news groups. Could you 
also post the pref setting to revert to the old viewmanager so users can isolate 
problems:

I have the following comments which we can address by filing separate bugs:


Documentation
-------------
Since the algorithm used to create the display list has become quite 
complex could you add an overview of the algorithm 
used to support z-index/z-index:auto? The current algorithm looks complex until 
the "reader" realizes what it means we have to support the fact that 
z-index:auto does not create a separate stacking context. This could include   
general description of the the creation of Z-nodes, placeholders, the stable 
sorting of the z-nodes to preserve document order, ReapplyClipInstructions, and 
OptimizeDisplayListClipping. This documentation could go at the top of the 
nsViewManager.cpp file for future reference.


Getting the root widget in AddCoveringWidgetsToOpaqueRegion
------------------------------------------------------------
Should ask the viewmanager for the RootWidget using nsViewManager::GetWidget 
instead of getting it out of the root view. A while back I put in support which 
makes it possible to pass in a widget instead of requiring the root view to have 
a widget. 



Tabs in the file
----------------
We should remove all of the tabs in the nsViewManager.cpp,.h files. Everyone 
should be converting tabs into 2-spaces when they edit. The old viewmanager had 
the tabs messed up, it would nice if the new viewmanager fixed this for good.


Two DisplayZTreeNode per display element
----------------------------------------
Do we really need to create two DisplayZTreeNode for each display element? Maybe 
we could look at consolidating them into a single DisplayZTreeNode per element 
in the future?


Memory Allocation/Deallocation
-------------------------------
The old viewmanager tried to avoid allocating and deallocating memory for the 
display list by re-cycling the memory used by the display elements and 
expanding it as needed.

The new view manager allocates and deallocates both DisplayZTreeNode and display 
list elements for each paint request. It may be advantageous to re-cycle 
the DisplayZTreeNode/Display list elements using an Arena


Backstop rendering using RGB(128, 128, 128)
-------------------------------------------
Line 1231: XXX Which color should we use for these bits.

A better guess for the backstop rendering would be to use the nsLookAndFeel 
Object to get the
system window background color.

Debug code
----------------
It would be nice to have a function which could walk the zTree and dumped its 
contents.



r = kmcclusk@netscape.com


does this get rid of VM1 as well? lets not ship 3 view managers. personally, i 
don't even like the idea of having two in there.

VM1 is gone. There are only two view managers.

We need to keep both of them around for awhile so we can diagnose regressions by
having users switch back to the old viewmanager. Before we ship we need to
remove ViewManager2 from the build.
sr=attinasi - please be sure to address Kevin's comments - Thanks!
Great. Which newsgroups? Porkjockeys, layout, reviewers, anyone else?

Do you want me to resolve those additional issues before we turn it on, or do
you want me to just file bugs against myself and address them afterward? In
either case I will definitely wait until 69346 is fixed before turning it on.
Regarding the individual issues:
> Documentation
Agreed.
> Getting the root widget in AddCoveringWidgetsToOpaqueRegion
OK, sure.
> Tabs in the file
Sure. A separate cleanup-only patch would be good. We can also remove at least
one unused function and some unused variables.
> Two DisplayZTreeNode per display element
I'll have to look at this. They might be combinable but at the cost of confusing
the logic and possibly increasing the size of every DisplayZTreeNode, so it
might not be a win.
> Memory Allocation/Deallocation
We could certainly improve that, but in the spirit of avoiding premature
optimization, I chose not to put in recycling code until we have evidence that
it matters.
> Backstop rendering using RGB(128, 128, 128)
Well, whenever backstop rendering is used, I think we have a view or layout bug
(now that bug 67478 is fixed so that every document has a non-transparent
background).
At some point in the future we may want to have widgets paint their own
backgrounds at the window system level for usability reasons (forget the bug
number but I'm sure you're CCed on it :-) ). This would also eliminate the need
to guess a color for backstop rendering.
> Debug code
I have some debug code to walk the display list already. I find that more useful
than walking the ZTree. It would certainly be useful to have that checked in.

I have some additional view manager cleanup in my trees. Once this bug is closed
I will submit it. I merged the two separate Refresh(rect) and Refresh(region)
functions into one function Refresh(region) and let the view manager use a paint
region (rather than rect) if GFX provides one. I also have other view manager
work addressing various bugs which I'll have to get back to.
Posting your checkin message to porkjockeys, layout, reviewers would be fine. I
would also include layout.checkins.

I don't see any reason to delay turning it on by default. Just file separate
bugs for the open issues.




FYI I've reported one ScaryViewManager2/NewViewManager1
(ViewManager3 for the sake of argument) issue in bug 70446.

Once bug 70446 is FIXED, I will post messages to the newsgroups and turn on the
new view manager by default using the attached patch.
Does that patch include a way to turn on the old view manager?
The existing pref will be honoured if it's present. So just set

 user_pref("nglayout.debug.enable_scary_view_manager", false);
It's not so scary anymore now is it? Maybe we should change the name of the pref
is we plan to keep it there for any length of time...
one question:

do you still need line
http://lxr.mozilla.org/seamonkey/source/view/src/nsViewManager.cpp#1241 for
debugging? or can it be commented/put out somehow?

From a fast lxr search, I think this is the line producing that loads of spam in
the console window...
void nsViewManager::ShowDisplayList(PRInt32 flatlen)
appears to be unused.
Attached patch add ifdef DEBUG — — Splinter Review 

    
    

    
  
Marc: I don't think it's worth changing the pref name.

timeless: There is a lot of cleaning up of unused code that can be done. Much of 
this unused stuff is legacy code from the old view manager. This can be done 
later.

Changing the "final transparent rect" warning to be #ifdef DEBUG is a good idea. 
However it should stay in for debug builds because it reveals the presence of 
real bugs --- and potentially serious ones, at that. No document should ever 
leave any of its area unpainted. If the document is trying to paint its whole 
area, but views are being incorrectly sized or incorrectly marked transparent, 
or region calculations are being done incorrectly, then we need to know about 
that for painting performance reasons if nothing else.

So, sr=roc+moz@cs.cmu.edu for patch 27121. Maybe Kevin can rubber-stamp that 
one with his r=?

All the prerequisite bugs have been fixed. I think we're ready to roll. But it 
might be a good idea to wait until after 0.8.1 has branched before we throw the 
switch.

    
    

    
  
27121 checked in.

    
    

    
  
roc: Throw the switch, man! :-)

Gerv

    
    

    
  
Checked in! Now the fun begins.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
I plan to sit tight for a few days to make sure that everything settles down 
nicely, then file a few bugs against myself to address remaining issues. Then 
someone can go through the bugs that were blocked by this one and resolve them.

    
    

    
  
Filed bugs 73383 and 73382 against myself.

Alright folks, this show's over!

    
    

    
  
This one should have been verified for a long time ;-)

Status: RESOLVED → VERIFIED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: