264245 - form popup (combobox) appears in wrong place after scroll

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 2004-10-12-07 and later Linux builds

STEPS TO REPRODUCE:
1)  Load https://bugzilla.mozilla.org/query.cgi
2)  Scroll down until the "Sort results by:" combobox first appears in the
    viewport.
3)  Click on this combobox to drop down its dropdown.
4)  Click away from the combobox to close the dropdown
5)  Scroll all the way to the bottom of the page (use the scrollbar, or click
    twice in step 4 to move focus).
6)  Click on the same combobox as in step 3.

EXPECTED RESULTS:  dropdown appears near the new combobox position

ACTUAL RESULTS:  dropdown appears at the same place as the first time, so away
                 from the combobox.

NOTES:  Other comboboxes on that page reproduce the problem too, but this one
does it most reliably.  Based on when it appeared, this is a regression from bug
238493.  It looks very similar to bug 64902, but doesn't need an iframe to
reproduce.  Given the issues I ran into in bug 264231, it almost feels like
ProcessPendingUpdates() is not getting to the popup's view... (which is
floating, but that shouldn't affect things).  Also, I'm not sure about all the
flags being passed to ReflowChild and FinishReflowChild by
nsComboboxControlFrame::ReflowComboChildFrame.

Comment 1

[Bill Mason]

This sounds something like bug 263992, which was known to be caused by bug 238493.

FWIW.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, I'm well aware.  But that bug was Windows-only and caused by a
Windows-specific widget issue, whereas this one is happening on Linux (though it
would be good to test it on Windows if you could).

Comment 3

[Bill Mason]

(In reply to comment #2)
> Yes, I'm well aware.  But that bug was Windows-only and caused by a
> Windows-specific widget issue, whereas this one is happening on Linux (though it
> would be good to test it on Windows if you could).

I was seeing something just like that on a Windows build in the same timeframe
(I don't have it at hand now, because I went back to an older build and I'm not
at that machine right now to try it out again).  I assumed it was bug 263992
though, so I never wrote a separate bug report.

I'll try to retry it on Windows again when I'm back at that machine.

Comment 4

[Bill Mason]

Attachment: Windows screenshot

This is what you get on Windows following the reproduction steps given here.

The popup of options winds up in the wrong place, but not I think the same
wrong place being described on Linux.

This is the 20041013 build, Win2K.

Comment 5

[Boris Zbarsky [:bzbarsky]]

No, that's about what I see on Linux....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimalish testcase

Comment 7

[Bill Mason]

Testcase works for me in the 20041014 build, Win2000.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, I hunted down why this happens in the GTK1 impl.  Comments on the other
impls will follow after the GTK1 problem description.

The basic problem is that calling Move(x, y) on a widget whose bounds already
have (x, y) as the top-left corner of the rect is NOT a no-op.  The code checked
in for bug 238493 assumes it is.

To be precise, there are two issues, and it was pure accident that this stuff
used to work.  Bug 64902 was not at all an accident.   The first issue is that
GTK1 nsWindow impls keep their _screen_ position (Move() uses coordinates
relative to the parent widget) cached.  Calling nsWindow::Move() invalidates
this cache no matter what values are passed in.  This is rather backwards,
because what Move() should really be doing is invalidating the cache of all
_descendants_ for cases when we actually move (since their screen positions have
now changed while their bounds have not).  This is what caused bug 64902,
essentially, as far as I can tell -- the iframe's widget doesn't move relative
to parent, so keeps its cached screen position and the popup positions relative
to that; see next paragraph.

The second issue (and the reason we have this caching) is that popup-type
windows (like combobox popups) actually have to position themselves relative to
the _screen_.  At the moment this is hacked in by not bailing out of
nsWindow::Move() for popups (or rather all toplevel windows) even if the
passed-in parent-relative values are equal to the existing parent-relative
values, "because the parent may have moved".  But we're not even calling into
Move() now, so this hack has no effect.  So really, moving a widget should not
only invalidate the caches of all descendants but also reposition all descendant
popups appropriately....

In any case, simply setting |changedSize = changedPos = PR_TRUE;| in
nsView::ResetWidgetBounds fixes this bug and bug 64902 for GTK1.  I don't think
that's really what we want to do, though.  It'd be better to fix widgets to not
make random assumptions about Move() being called on things that are not
actually moving (which is what would be needed to fix bug 64902 otherwise).  At
the same time that could lead to a lot of traversal of the widget tree...

Anyway, given all of the above, I looked at all our Move() and Resize() methods,
with the following results:

1) BeOS.  nsWindow::Move is not a no-op for popups as above, since they position
   relative to screen.  
2) Cocoa.  nsCocoaWindow::Move has the same issues for popups.  It may have
   other issues too; I can't tell for sure.
3) GTK1.  As described above, and nsWidget::Resize and nsWindow::Resize do some
   work no matter what (but this may not be necessary; in fact doesn't look like
   it is).
4) GTK2.  nsWindow::Move has the usual popup issues.  There is no screen
   position cache, unlike GTK1, but this code is still assuming that Move() will
   actually be called on the popup's widget any time an ancestor widget moves.
5) Mac.  nsWindow::Resize always recalculates window regions, even if the size
   didn't change, because "our parents may have changed" (nice comment by
   pinkerton to that effect).
6) OS/2.  Looks like it may be ok, but I don't really grok that code since I
   don't know the system APIs there.
7) Photon.  nsWindow::Move has the usual popup issues.
8) QT.  Looks ok to me (assuming that the nsIWidget impl is nsCommonWidget here)
9) Windows.  nsWindow::Move has the usual popup issues (needs to be called if
   parent has moved).  nsWindow::Resize (the version that takes x/y values) has
   the same issues.
10) Xlib.  This is basically a copy/paste of GTK1 and has basically the same
    issues.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> |changedSize = changedPos = PR_TRUE;|

How about we just have nsView::ResetWidgetBounds always call
nsIWidget::Resize(x,y,w,h) and leave various optimizations to the widget layer?
That will fix the bug, isolate the brokeness to individual widget
implementations, and allow any performance issues to be fixed there piecemeal.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That will fix the bug

In general, yes, since we're widget caching around reflow and at the end of an
update we'd walk the entire widget tree and call Resize() on all of them.

But if some code doesn't batch view updates and does something that repositions
widgets it could still run afoul of this issue.

Now I'm all for not supporting any sort of widget geometry manipulations outside
of view update batches (though combobox may be doing that now when it drops
down... I can't recall).  But if so, we should clearly document that in the
relevant apis (nsIWidget, nsIView, nsIViewManager) and remove the code in views
that repositions/resizes widgets when the views are repositioned/resized.

If we do plan to support both modes of operation, we need to fix the widget
impls after all...

In any case, no matter what we decide, we need to clearly document it all in
nsIWidget.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, let's see what's entailed in fixing widgets. It sounds like the following:
1) When we move a widget, traverse the widgets under it and move any popups we find.
2) When we move a widget, traverse the widgets under it and invalidate any
cached screen coordinates they may have. (GTK1 only?)

But I think it would be OK for popups to not move while they're visible. If so,
then we can replace 1) with
1b) When a popup becomes visible (via Show() or size change away from (0,0)),
reposition it at the correct screen offset.

1b) and 2) seem reasonable to implement, IMHO.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 1b) and 2) seem reasonable to implement, IMHO.

Agreed.  Again, we should document all this in the nsIWidget apis as needed. 
Help out authors of new widget impls if nothing else...

I'll defer to blizzard and bryner on whether it's faster to walk the kids every
time clearing the cache or just not cache at all (the cache is used by
WidgetToScreen(), and I don't really know what the performance expectations for
that code are).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that this still leaves the mac Resize() code, though... I can't tell what
that code is trying to do, unfortunately .

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure we want to say that Move() *never* moves popups, because maybe we
will find a need to move popups while they're visible. Probably we should just
say that changes in the geometry of a popup's parent widget chain will not take
effect while the popup is visible.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But that means that Move(x,y) ... Move(x,y) might not be idempotent if there is
a parent geometry change in the middle ... argh.

Comment 16

[Boris Zbarsky [:bzbarsky]]

argh indeed....

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then what I suggest is the following policy:
Move() and Resize(x,y,w,h) do not move popups immediately. Popups only change
position to the desired position when they go from hidden to shown.

If later we discover a need to move visible popups, we can add a RepositionPopup
method to nsIWidget which will move the popup to the desired position.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK.  So from the widget impl point of view to get correct functioning:

1)  Move/Resize will be called on things in some order not to be depended on.
2)  Popups should be properly positioned in the Show() call.
3)  Move/Resize on the popup itself have no effect while it's shown.

We leave the viewmanager code as-is and file followup bugs on widget impls that
try to compensate for changes to parents when Move/Resize is called on a kid.

Widget owners, does that seem reasonable?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually Show(PR_TRUE) can force the popup to be repositioned even if it's
already visible, so no need for a new RepositionPopup API.

> 3)  Move/Resize on the popup itself have no effect while it's shown.

Actually it's simpler to say "Move/Resize on popups does not take effect until
Show(PR_TRUE) is called". If the popup is not currently shown then there is no
visible effect anyway until it becomes shown.

Comment 20

[Christopher Blizzard (:blizzard)]

> 4) GTK2.  nsWindow::Move has the usual popup issues.  There is no screen
>    position cache, unlike GTK1, but this code is still assuming that Move() will
>    actually be called on the popup's widget any time an ancestor widget moves.

Careful.  Gtk2 actually caches this internally.  That's why it's not implemented
in our widget code.

Comment 21

[Christopher Blizzard (:blizzard)]

> 
> We leave the viewmanager code as-is and file followup bugs on widget impls that
> try to compensate for changes to parents when Move/Resize is called on a kid.

So you're assuming a very strong connection here between popups and their
parents?  I don't think that's ever existed before and it makes my spider sense
tingle.

We want to avoid doing any _actual_ movement of widgets until they are actually
shown.  This is a huge cost in X, because of all of the server round trips that
are required when you position relative to a parent or screen.

Also, why is it painful to do this at a higher level in one place instead of
doing it in several different widget implemenations?  That's not clear to me.

Comment 22

[Christopher Blizzard (:blizzard)]

> Actually it's simpler to say "Move/Resize on popups does not take effect until
> Show(PR_TRUE) is called". If the popup is not currently shown then there is no
> visible effect anyway until it becomes shown.

Sorry, I didn't read this before replying.  I'm pretty sure that the gtk2 code
does this.  It took a long time to get right.

Comment 23

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #20)
> Careful.  Gtk2 actually caches this internally.

So if I have two widgets, widget1 child of widget2.  And I call Move() on
widget2 (which changes the screen positions of both widgets, but not the
nsIWidget position of widget1).  Then the cache of widget1 will _not_ be
updated?  If it's properly updated then we don't care  that there is a cache. 
The problem is that GTK1 doesn't update the cache, not that it has one.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Also, why is it painful to do this at a higher level in one place instead of
> doing it in several different widget implemenations?  That's not clear to me.

If you can come up with a simple specification for how popup positioning works,
which explains in detail when callers can and must call Move(), Resize(x,y,w,h),
etc, that is true for all of our widget implementations, and isn't going to cost
us performance now or looking forward, then we could do it at the higher level.
Boris and I don't see it.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A problem with the approach in comment #17 is that it raises the question "what
happens if you do GetBounds or WidgetToScreen on a popup after calling Move()
but before calling Show(PR_TRUE)"? It also may complicate things in general to
allow an inconsistency between the "specified" position and the "actual" position.

Right now I'm leaning towards specifying simply that Move() or Resize(x,y,w,h)
on a widget *must* put all descendants in the right place, popups or not, as
seen on screen and as observed by GetBounds, WidgetToScreen, etc. That's the
simplest policy. Widget implementations can optimize the movement of hidden
popups subject as long as they produce the correct observable behaviour.

Comment 26

[Boris Zbarsky [:bzbarsky]]

If we do that, is there any way we can prevent having to do extra work on the
widget tree?   Possibly walk the view tree twice, first calling WillMove() (or
something) on all views we plan to move, then calling Move().  This way widgets
can mark subtrees as "will be moved, don't have to deal with it now".

Or is that case (of having to call Move() on several widgets that are kids of
each other) rare enough that optimizing it is not realy worth it?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In what situations will we have a lot of widgets? I think only when you have a
page with a lot of scrolling elements (overflow:auto, overflow:scroll,
listboxes, comboboxes, edit boxes). My guess is that even in these cases the
widget tree will be fairly shallow. And if you ensure that you only scan the
widget subtree when the root of that subtree is actually moving, then I think we
will find in practice that non-leaf widgets don't move very often, so we don't
scan a big widget subtree very often. And scanning itself should be quite cheap,
it's only when we have lots of popups that must be moved that we have a problem.
On GTK1/2 we're going to only have to actually move visible popups, which won't
be many, and on Windows we can do that too, or we can just move all of them if
that's cheap. Dunno about Mac.

An alternative to doing all this might be to change our model for popups so that
the popup widget simply has no parent, which is much more in line with what the
underlying window systems do. I don't know what the implications of that might be...

Comment 28

[Boris Zbarsky [:bzbarsky]]

Agreed about the fact that non-leaf widgets don't move much.  Sounds like we
have a plan, then.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually at the moment I sorta favour my last idea.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I'm not really wise enough in the ways of widgetry to evaluate that one... 
blizzard?  Others?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix?

This patch seems to work and fix things on GTK1. It doesn't seem to need any
widget implementation changes at all. (But once we verify that all popup
widgets have no parent, we can probably simplify some crufty code in a lot of
places.) It *may* cause more widget motion than we currently do, but I doubt it
matters. Anyway it would be relatively easy to fix GTK1/2 to delay moving
hidden popups.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix for other GTK1 problems

This fixes the other problem bz noted in GTK1: invalidate the position cache on
the entire widget subtree when the root of the subtree moves. Also, I noticed
that GetWindowPos only needs to go to the X server for root widgets and can use
the cached window pos on the root widget otherwise.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

I encountered some problems with GTK2. To fix them, I made popups be roots in
the nsIWidget tree but still be parented in the native widget tree.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

fix v2 stilll works great in GTK1. Do I have any volunteers to test it in
Windows and Mac?

Comment 35

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #27)
> In what situations will we have a lot of widgets? 

Remember that we also have very complicated xul dialogs, like ff's prefs dialog.
that have lots of scrolly bits and complex UI. We're not solely talking web
pages here.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the sort of page which has given us trouble in the past is the Slashdot
moderation page, where you have a complicated layout containing several hundred
comboboxes. That should be alright with these changes, I hope.

Comment 37

[Martijn Wargers (dead)]

(In reply to comment #34)
> fix v2 stilll works great in GTK1. Do I have any volunteers to test it in
> Windows and Mac?
I've tested this fix v2 on Windows with my mingw debug build of Mozilla.
The bug mentioned here is not visible with or without the patch in Windows.
But I think I see some regressions with this fix:
http://home.hccnet.nl/m.wargers/test/mozilla/menu.png
The menu popup gets too low.
http://home.hccnet.nl/m.wargers/test/mozilla/menu_offsetx.png
http://home.hccnet.nl/m.wargers/test/mozilla/menu_offsety.png
It seems as if the amount of distance it is too left or too right is the same
amount as the x,y position of the Mozilla window on the screen.

Also in the preference menu the drop down menus get too low:
http://home.hccnet.nl/m.wargers/test/mozilla/preferences.png

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Windows fix?

Martjin, if you apply this patch, does that make your Windows problems go away?

Comment 39

[Martijn Wargers (dead)]

Yep, that seems to fix it all :)

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: combined fix

This is all the fixes rolled together, plus some assertions to catch anyone
passing in a popup with an nsIWidget parent (which is forbidden now).

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops. I still need to get this working on Mac, apparently. I've attached a
proposed extra fix for the Mac in bug 264235, attachment 162697 [details] [diff] [review]

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: change to comments

I guess we should add a comment in nsIWidget.h, like this.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 162696 [details] [diff] [review]
combined fix

I'd like to check this in ASAP. I know it will unbust GTK/GTK2, and should at
least get Windows through smoketests although there could still be regressions
left over as described in bug 263992 comments. It doesn't fix Mac yet,
apparently, but it doesn't bust Mac any more than it's already busted.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162696 [details] [diff] [review]
combined fix

r=bzbarsky

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162702 [details] [diff] [review]
change to comments

r+sr=bzbarsky

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked in these patches. As far as I know, only Mac still has problems. See
bug 264235.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Robert, those fix bug 64902 as well, right?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I hope so, but I haven't checked.

Comment 49

[[e-mail]]

I don't see bug 64902 fixed in my GTK1 that I built 12 hours ago.

Also, whenever a popup opens its "parent" deactivates. Related?

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Also, whenever a popup opens its "parent" deactivates. Related?

Probably. GTk1 probably needs to relate the popup to its native widget parent in
a way similar to what GTK2 does. Care to fix it? :-)

Comment 51

[[e-mail]]

Another bug I see in GTK1 but not on Windows - the open location dialog opens at
a different location (!) on the screen every time...