146108 - popup menu position wrong after opening tabs.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Thomas Schoepf]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0rc2) Gecko/20020520
Debian/1.0rc2-3
BuildID:    20020520 Debian/1.0rc2-3

Popup menus in the content area have an offset in y direction if the page was
loaded without visible tab bar and after loading a new tab was opened.

When the page on the old tab is reloaded (or a new url is opened) the menu is
back at the correct position. This bug also does not happen, when the menu has
not been opened before the new tab was added.

This also applies to menus for <input> elements and can cause accidental selections.

Reproducible: Always
Steps to Reproduce:
1.Open a new navigator window
2.Right-click into the content area
3.Open a new tab (so that the tab bar shows up)
4.Go back to the old tab
5.Right-click again.

Actual Results:  Menu pops up at a wrong position, offset by about the height of
the tab bar.

Comment 1

[John Levon]

This happens with combo boxes too.

Comment 2

[Thomas Schoepf]

same with the sidebar (open menu, toggle sidebar, open menu again)

Comment 3

[Thomas Schoepf]

Attachment: Screenshot, illustrating the problem

Comment 4

[Thomas Schoepf]

Reason is that (widget/src/gtk/)nsWindow::GetWindowPos() uses cached x/y coords
and obviously these cached values are not invalidated when tabs or sidebars show
up/vanish.

Comment 5

[Thomas Schoepf]

Attachment: patch, getting rid of the position cache

This patch removes the position cache (that seems to be implemented only with
gtk, e.g. gtk2 doesn't cache) and therefore fixes the problem.

The reason why page reloading cured the symptom was not that the cache was
invalidated but rather a completely new nsWindow object was created.

Comment 6

[Brendan Eich [:brendan]]

=> blizzard, fast blast for 1.0 or 1.0.1.

/be

Comment 7

[Thomas Schoepf]

I'd like to ask for a review of attachment 85173 [details] [diff] [review]
Thanks

Comment 8

[Christopher Blizzard (:blizzard)]

Wait, the position cache is there for a reason.  Why not find the problem in the
cache instead of removing it wholesale?

Comment 9

[Thomas Schoepf]

As far as I can see, the cache invalidation is not working for the following
reasons:

- the gtk event handler that invokes InvalidateWindowPos() is only triggered,
when the whole mozilla window is moved or when it is resized (even without
changing the (x,y) position).

- nsWindow::Move() invalidates the cache only when the position changed but
viewing/hiding the sidebar/tabbar/main menus triggers a call to Move() with
unchanged coordinates (always 0,0). Obviously that document window is contained
in another window.

This containing window is indeed moved to the correct coordinates and calls
gdk_window_move(mSuperWin->shell_window, aX, aY) in its nsWindow::Move() method.

This gdk_window_move() does not trigger the InvalidateWindowPos event. So I
think the reason for this bug is that gdk_window_move() must invalidate all
position caches of all contained windows.

As I am quite unfamiliar with gdk/gtk I have no idea about what needs to be done
to have this event triggered.

Comment 10

[Robin Lu]

Attachment: patch

Because signal toplevel_configure is only attached to toplevel widget, position
or size change of non-toplevel widget won't trigger the change of cached
position coordination, e.g, sidebar open/close. We can't attach every widget to
the configurenotify of their every ancestor because it is too inefficent. To
traverse all the child while get ConfigNotify to invalidate the cache is also
inefficent. So I choose to invalidate the cache in Move method. Althoug some
Move methed does not actually cause a move, this way at least saves the cache
and avoid a castcade invalidating. Fortunately, layout calls the Move of the
child widget when it's ancestor does a move.
Seeking review=?

Comment 11

[Thomas Schoepf]

Comment on attachment 85173 [details] [diff] [review]
patch, getting rid of the position cache

Would be ok with me, much better than having no cache at all.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

*** Bug 94296 has been marked as a duplicate of this bug. ***

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

I duped bug 94296 (with a bunch of dups) to this one.

You don't have to open the menu/widget first before causing the toolbar/etc to
collapse/appear to see the problem.  I.e.

With sidebar on
Go to page with dropdown select widget
Turn off sidebar (F9)
Open dropdown

Let's get reviews on this and determine if it's the right solution

Comment 14

[Robin Lu]

I'd like to give a FAQ of this bug to push the review:

1. When does widget update its cached location coordinate?
Only when the widget revieves the toplevel_configure signal or someone calls
InvalidateWindowPos explicitly. ( See handle_invalidate_pos in
widget/src/gtk/nsWindow.cpp)

2. Who will send the toplevel_configure signal?
When the toplevel widget revieces the ConfigureNotify, it will send out the
signal. ( See attach_toplevel_listener and toplevel_window_filter in
widget/src/gtksuperwin/gtkmozarea.c)

3. Will the cached position be invalidated when a non-toplevel parent widget get
the position change? (e.g. sidebar show/hide, toolbar show/hide will cause the
position of the other widgets changed)
No.

4. Why mozilla for windows don't have such problem?
It does not use such cache. ( See nsWindow::Move in widget/src/window/nsWindow.cpp )

Comment 15

[Christopher Blizzard (:blizzard)]

Comment on attachment 98852 [details] [diff] [review]
patch

r=blizzard

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 98852 [details] [diff] [review]
patch

sr=bryner

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 98852 [details] [diff] [review]
patch

a=asa for checkin to 1.2 (on behalf of drivers).

Comment 18

[Jay Yan]

checked in.

Comment 19

[Pratik]

*** Bug 53318 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 112853 has been marked as a duplicate of this bug. ***

Comment 21

[Boris Zbarsky [:bzbarsky]]

*** Bug 118424 has been marked as a duplicate of this bug. ***

Comment 22

[Boris Zbarsky [:bzbarsky]]

Note that this did not fix <select>s inside <iframe>s...