Closed Bug 39621 Opened 25 years ago Closed 24 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
(deleted), patch
Details | Diff | Splinter Review
Overhaul of nsViewManager2: revision 2
(deleted), patch
Details | Diff | Splinter Review
Infrastructure patch; interface changes only, no behaviour changes expected
(deleted), patch
Details | Diff | Splinter Review
Translucent fixed element is not clipped correctly
(deleted), text/html
Details
Infrastructure patch v2: small changes to nsHTMLContainerFrame.cpp
(deleted), patch
Details | Diff | Splinter Review
Infrastructure patch v3: Additional comment
(deleted), patch
Details | Diff | Splinter Review
Patch to nsViewManager.cpp/nsViewManager.h; spiffy new code for z-ordering/clipping/etc
(deleted), patch
Details | Diff | Splinter Review
Patch to build and use nsViewManager instead of nsViewManager2
(deleted), patch
Details | Diff | Splinter Review
Patch to build nsViewManager and enable it via a pref
(deleted), patch
Details | Diff | Splinter Review
Patch to turn on new view manager by default
(deleted), patch
Details | Diff | Splinter Review
add ifdef DEBUG
(deleted), 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 (deleted) — — 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 → 24 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: