Open Bug 117730 Opened 23 years ago Updated 2 years ago

fix native widget z-ordering

Categories

(Core :: XUL, defect)

defect

Tracking

()

People

(Reporter: jonitis, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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 ().
Dainis, could you possibly attach that code as a |diff -u| using 
http://bugzilla.mozilla.org/attachment.cgi?bugid=117730&action=enter ?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch, review
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
Dainis, could you also attach a |diff -uw| for review purposes?  (tabs suck....)
Attached patch nsBaseWidget.cpp with diff -uw (obsolete) — Splinter Review
BTW not sure if variable 'index' declaration could be between for parenthesis?
Does all compilers on which Mozilla is built can handle that?
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?
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.
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.
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
Attachment #63248 - Attachment is obsolete: true
Attachment #63249 - Attachment is obsolete: true
Comment on attachment 63464 [details] [diff] [review]
nsBaseWidget.cpp  |diff -uw|

That looks better. sr=attinasi
Attachment #63464 - Flags: superreview+
Status: NEW → ASSIGNED
Target Milestone: --- → Future
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 → ---
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 on attachment 63464 [details] [diff] [review]
nsBaseWidget.cpp  |diff -uw|

r=beard
Attachment #63464 - Flags: review+
Okay, this has r= and sr=.  Is it in?
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
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?
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.
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.
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.
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?
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.
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
Blocks: 260690
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: