Open
Bug 117730
Opened 23 years ago
Updated 2 years ago
fix native widget z-ordering
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
NEW
People
(Reporter: jonitis, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
9.62 KB,
patch
|
Details | Diff | Splinter Review | |
1.27 KB,
patch
|
beard
:
review+
attinasi
:
superreview+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7) Gecko/20011221 BuildID: This function modifies the window z-index relative to the parent window and modifies the the current widget position in the parent's sorted nsISupportsArray. 1. The bug The problem is that currently the code loops through all child widgets of the parent and if the z-index is smaller (behind) the i-th element then the call to PlaceBehind () is made which positions the current widget behind the i-th widget. But the code does not handle the case when the z-index of current node is the largest (on top of all other child windows) of all child widgets. In that case the PlaceBehind () is not called at all. The code just loops through all array elements but IF in the loop is never true. The IF right after the FOR loop is true, but it does not make call to the PlaceBehind (). Looks that this problem was introduced in 18-jul-2001 by pollmann@netscape.com when he fixed the defect #43410. http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/widget/src/xpwidgets&command=DIFF_FRAMESET&file=nsBaseWidget.cpp&rev1=1.92&rev2=1.93&root=/cvsroot To fix the problem when the widget is the topmost in the list of parent widget child widgets the call to PlaceBehind (nsnull, PR_FALSE) should be made which places the window behind the desktop window (topmost window). 2. Optimization. Current implementation always (even if there is only one element) deletes the current element from the nsISupportsArray with call to RemoveElement () and then reinserts it back into empty array with call to AppendElement (). This is inefficient waste of time. Especially because most of the time there is only one child widget in the array. I propose to remove and after insert the element back in right position only in case when number of element in array is LARGER than 1. The old code: NS_IMETHODIMP nsBaseWidget::SetZIndex(PRInt32 aZIndex) { mZIndex = aZIndex; // reorder this child in its parent's list. nsBaseWidget* parent = NS_STATIC_CAST(nsBaseWidget*, GetParent()); if (nsnull != parent) { parent->mChildren->RemoveElement(this); PRUint32 childCount, index; if (NS_SUCCEEDED(parent->mChildren->Count(&childCount))) { for (index = 0; index < childCount; index++) { nsCOMPtr<nsIWidget> childWidget; if (NS_SUCCEEDED(parent->mChildren->QueryElementAt(index, NS_GET_IID(nsIWidget), (void**)getter_AddRefs(childWidget)))) { PRInt32 childZIndex; if (NS_SUCCEEDED(childWidget->GetZIndex(&childZIndex))) { if (aZIndex < childZIndex) { parent->mChildren->InsertElementAt(this, index); PlaceBehind(childWidget, PR_FALSE); break; } } } } // were we added to the list? if (index == childCount) { parent->mChildren->AppendElement(this); } } NS_RELEASE(parent); } return NS_OK; } The new code: NS_IMETHODIMP nsBaseWidget::SetZIndex(PRInt32 aZIndex) { mZIndex = aZIndex; // reorder this child in its parent's list. nsBaseWidget* parent = NS_STATIC_CAST(nsBaseWidget*, GetParent()); if (nsnull != parent) { PRUint32 childCount; if (NS_SUCCEEDED(parent->mChildren->Count(&childCount))) { if (childCount > 1) { parent->mChildren->RemoveElement(this); for (PRUint32 index = 0; index < childCount; index++) { nsCOMPtr<nsIWidget> childWidget; if (NS_SUCCEEDED(parent->mChildren->QueryElementAt(index, NS_GET_IID(nsIWidget), (void**)getter_AddRefs(childWidget)))) { PRInt32 childZIndex; if (NS_SUCCEEDED(childWidget->GetZIndex(&childZIndex))) { if (aZIndex < childZIndex) { parent->mChildren->InsertElementAt(this, index); PlaceBehind(childWidget, PR_FALSE); break; } } } } // were we added to the list? if (index == childCount) { parent->mChildren->AppendElement(this); PlaceBehind(nsnull, PR_FALSE); } } } NS_RELEASE(parent); } return NS_OK; } Reproducible: Always Steps to Reproduce: This was found on OS/2 where popup menus were displayed behind the parent menus. Even if these changes fixed the situation this bug is not a real cause for menu problem in OS/2. The real problem is #101771 - menus need to be clipsibling: http://bugzilla.mozilla.org/show_bug.cgi?id=101771 I guess you can't reproduce the problem (at least on Windows) and won't see any difference after applying a fix. Just look at the code and see that now it is wrong :) Actually this does not affect all platforms - only those which implement nsWidget::PlaceBehind ().
Comment 1•23 years ago
|
||
Dainis, could you possibly attach that code as a |diff -u| using http://bugzilla.mozilla.org/attachment.cgi?bugid=117730&action=enter ?
Reporter | ||
Comment 2•23 years ago
|
||
Modify nsBaseWidget::SetZIndex () 1. To remove element from array and insert in correct position only if there is more than one child widget in the array. 2. Call PlaceBehind () with nsnull first parameter if this widget is the topmost to place it right under the desktop 3. Replaces the tabs with spaces
Comment 3•23 years ago
|
||
Dainis, could you also attach a |diff -uw| for review purposes? (tabs suck....)
Reporter | ||
Comment 4•23 years ago
|
||
BTW not sure if variable 'index' declaration could be between for parenthesis? Does all compilers on which Mozilla is built can handle that?
Comment 5•23 years ago
|
||
Comment on attachment 63249 [details] [diff] [review] nsBaseWidget.cpp with diff -uw Why is the childCount not decremented after removing 'this' and before the for loop that goes until index == childCount? It seems that it should be, or the for loop should stop when index == (childCount-1) What am I missing?
Comment 6•23 years ago
|
||
BTW: declaration of 'index' in the for loop parens is OK, it is done everywhere. Personally I don't like it, but it is ubiquitous in the Mozilla code base.
Reporter | ||
Comment 7•23 years ago
|
||
Reporter | ||
Comment 8•23 years ago
|
||
Reporter | ||
Comment 9•23 years ago
|
||
Marc, of course you are not missing anything - I just somehow did not thought about it :( The childCount should be decremented. Changed the if to this: if (childCount-- > 1) These two new attachments make obsolete the previous ones, but I am not authorized to mark them as such.
Comment 10•23 years ago
|
||
The portability hazard to-do with for (int i = 0; i < N; i++) {...} is that the C++ standard says the scope of i is the loop control and the ... block, but at least MSVC messes up, and lets you use i after the loop-closing brace. People who test only on Windows tend to break other platforms by using i after the loop. It's hard to see where the scope should end, so some people play it safe by never declaring i in the initialization part of the loop control. /be
Updated•23 years ago
|
Attachment #63248 -
Attachment is obsolete: true
Updated•23 years ago
|
Attachment #63249 -
Attachment is obsolete: true
Comment 11•23 years ago
|
||
Comment on attachment 63464 [details] [diff] [review] nsBaseWidget.cpp |diff -uw| That looks better. sr=attinasi
Attachment #63464 -
Flags: superreview+
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Comment 12•23 years ago
|
||
Unfuturing and reassigning to attinasi (I don't know why hyatt had this bug in the first place). This bug has sr=attinasi. cc: beard for a review and blessing, since cvsblame sez he wrote this originally.
Assignee: hyatt → attinasi
Status: ASSIGNED → NEW
Target Milestone: Future → ---
Comment 13•23 years ago
|
||
assigning this to neard to get on his radar -- this needs a review from you Patrick. Once it gets your blessing, it can get checked in.
Assignee: attinasi → beard
Comment 14•23 years ago
|
||
Comment on attachment 63464 [details] [diff] [review] nsBaseWidget.cpp |diff -uw| r=beard
Attachment #63464 -
Flags: review+
Comment 15•22 years ago
|
||
Okay, this has r= and sr=. Is it in?
Comment 16•22 years ago
|
||
Oh, boy do we suck... Patch checked in to the trunk; Dainis, thanks a ton for doing this! I swear, usually patches don't lie around for 6 months after getting reviews....
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 17•22 years ago
|
||
Interesting. Checking this in brought back good old: http://bugzilla.mozilla.org/show_bug.cgi?id=101771 It would appear that now that this is in, the clipSiblings flag is bad. I backed it out and it seemed to fix things... Any opinion Dainis?
Comment 18•22 years ago
|
||
Michael, could you check which of the two parts of this patch causes the problem? There's the reorganization of the top of the loop and there's the call to |PlaceBehind(nsnull, PR_FALSE);| at the bottom.... The are independent of each other, and it would be good to know which is the problem here.
Comment 19•22 years ago
|
||
The new PlaceBehind is definitely the problem. Note I removed the clipSiblings flag from nsMenuPopupFrame, and that looked like it fixed the problem, but it did not. So the PlaceBehind is not working for OS/2.
Updated•22 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I am reopening this bug since it is the cause of the bad regression in bug 162501. Bug 162501 makes all absolute positionning look terrible in Mozilla. I have to add that this bug shows exactly what we should never do : the patch attached to this bug having r= and sr= is ***NOT*** the one that was checking in by bz the 9th of august at 19:07. Because of that, I could not determine that thix fix was the cause of the regression just by reverse-applying the patch attached here. I had to look at the diff from Bonsai, discover the fixes are not the same, and revert to version 1.103 and 1.104 to track the bug. The for this bug was my candidate number 1 for the regression and I tested it immediately. Because of the difference between the patch attached and the patch really applied, I spent 8 hours on this regression instead of 10 minutes. Thanks a lot. Please, _do not do that again_.
The fix for this bug causing two regressions, Boris who checked it in for the author being on vacation, I am backing out the change from the trunk.
I don't think there's anything wrong with what Boris did -- he fixed the horrible whitespace in the file, and he did note that in the checkin comment.
Comment 23•22 years ago
|
||
Um.. The patch that has reviews is a -uw diff. I checked in the -u version of the same diff, which is also attached to this bug. Had you reversed the -u diff instead of the -uw diff, you would have been (almost) fine (almost, because I had to change the scoping of one of the loop variables to be correct). Daniel, the same question applies as in comment 18. Which of the two substantive parts of the patch caused the regression in bug 162501?
Comment 24•22 years ago
|
||
OK. This is all nice, but: 1) Dainis has a point. The PlaceBehind() call seems to be something that should happen. Why is OS/2 failing to deal with it? 2) What was the real cause for the perf problem? It does not seem to be a problem on Linux, so I'm somewhat limited in my ability to test it... roc? any ideas?
Currently only Windows and OS/2 implement PlaceBehind(nsnull). The others basically ignore it. That's why you see XP differences. What seems to be happening with the regression is that whenever a view is re-synced with its frame, the view manager calls SetZIndex on the view's widget. In the common case, z-index values are all the same (zero) and so the view's widget is moved to the end of the list and PlaceBehind(nsnull) is called, which puts it on top of the stack. Problem is, widgets for position:fixed elements are assumed to be on top of the widget that scrolls the HTML body, and this gets broken. So this is arguably exposing a view manager bug being exposed, but I argue instead that using CSS z-index values to set the z-index of widgets is pretty bogus since the calculations that determine the true z-order of elements use a lot more than just the raw CSS z-index value. In fact the nsIWidget::Get/SetZIndex API is bogus because ordering the widgets by an integer index is just inappropriate. Therefore I think we should remove nsIWidget::Get/SetZIndex completely; it's a useless interface. For now we can just delete the calls to Get/SetZIndex from the view system; they're not doing anything useful currently, anyway. We should modify PlaceBehind so that if called on a child widget, it reorders the parent's child list like SetZIndex used to --- *BUT* only if the widget implementation actually successfully reorders the native widgets!!! This would be great because currently our internal child list orders can get out of sync with the actual native widget ordering, and there are some significant painting performance gains to be had if I can trust the lists to be in sync. To avoid regressing performance (for scrolling) and correctness (for plugins), this would have to be done in conjunction with a view manager patch: after computing a display list for painting, scan it to see if the final view z-order is consistent with the z-order for the widgets, and if it isn't, issue PlaceBehind calls to make it consistent. If widgets are fixed like I suggested in the previous paragraph, then I can do another patch to the view manager: fix UpdateWidgetArea to believe the widget z-ordering information and only update the visible area of each widget.
Comment 26•21 years ago
|
||
roc, would you like this bug per comment 25? Should I file a separate bug on that?
taking as per comment 25
Assignee: beard → roc+moz
Status: REOPENED → NEW
Summary: nsBaseWidget::SetZIndex has a bug and could be optimized → fix native widget z-ordering
Assignee: roc → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•