835044 - Firefox 19.0b+ on Linux (Windows OK) selects wrong row in XUL tree in level="parent" panel

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Laszlo Rozson]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130116073211

Steps to reproduce:

The extension 'MyContents' works fine since Firefox 3.6 -18. From 19.0b+ versions on Linux the XUL tree works wrong (on Windows all 19.0b-21.0a+ versions are good).


Actual results:

The XUL tree selects the wrong item when you click on it. It selects the item with two or more items below the one you clicked (depending on the location of the main window).
getCellAt, getRowAt functions also return with the wrong cell/row.


Expected results:

It should have selected the row on which you have clicked, as it used to happen in the case of versions prior to 19.0b1 and the Windows versions.

Comment 1

[Loic]

Could you use the devtool mozregression on Linux to find a possible regression range please: http://harthur.github.com/mozregression/

Just create a new testing profile (see https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles), install the add-on 'MyContents' and use this profile with mozregression (don't use your current profile) with the command --profile=/path
First FF19 nightlies started in October (--good=2012-10-01).

Comment 2

[Alice0775 White]

Attachment: reduced xul

Comment 3

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/48502b61a63e
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19 Firefox/19.0a1 ID:20121023030553
Bad:
http://hg.mozilla.org/mozilla-central/rev/93cc1ee94291
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19 Firefox/19.0a1 ID:20121024030643
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48502b61a63e&tochange=93cc1ee94291

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/35ab233bf290
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19 Firefox/19.0a1 ID:20121022020823
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a30e45e50e2
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19 Firefox/19.0a1 ID:20121023021208
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=35ab233bf290&tochange=3a30e45e50e2

Comment 4

[Alice0775 White]

In local build:
Last Good: 8da9c24417d0
First Bad: a0158d370785	
Regressed by:
a0158d370785	Karl Tomlinson — b=793501 attach DocumentViewer to top GTK widget r=roc

Comment 5

[Karl Tomlinson (:karlt)]

Looks like screen coordinates are being used for widget coordinates somewhere.

Comment 6

[Chris Coulson]

The issue is that nsWindow::GetParent() for the popup top-level window returns the main top-level window, and GetWidgetOffset() in nsLayoutUtils assumes that the bounds for the popup window are relative to this top-level window (which is not the case, as they are relative to the screen instead).

This worked before because the viewWidget in nsLayoutUtils::TranslateWidgetToView() used to be a child window, which had no reference to it's parent (nsWindow::mParent was null - another bug?), so fromRoot != toRoot, and then it did the translation using nsWindow::WidgetToScreenOffset() instead.

Should nsWindow::GetParent() return anything for a top-level window, even if it's a popup? It looks like it doesn't on Windows.

Comment 7

[Karl Tomlinson (:karlt)]

GetParent docs say it returns "nullptr if this is a top level window".
http://hg.mozilla.org/mozilla-central/annotate/0c45e6378f1f/widget/nsIWidget.h#l568

Sometimes I wonder whether nsIWidget thinks popups are not top level (layout considers popups in main document coords), so I checked what is currently happening.

nsBaseWidget::GetParent() always returns NULL.

On Cocoa:

  nsCocoaWindow, for top level windows I assume, uses the nsBaseWidget method.

  mParentWidget in nsChildView is always set to aParent in Create().
  nsChildView::GetParent() returns mParentWidget.

  BaseCreate() is always called with aParent.

On Android:

  mParent is always set to either aParent or aNativeParent in Create().
  GetParent() returns mParent.

  BaseCreate() is always called with a NULL parent widget.

On WINNT:

  mParent is always set to aParent or derived from aNativeParent (if possible)
  in Create().  mParent is not used, except to hold a reference.

  GetParent() uses GetParentWindow(aIncludeOwner = false), which uses native
  API and locates an nsWindow from a native window.

  GetBounds returns screen coords for popups on WINNT since 
  http://hg.mozilla.org/mozilla-central/rev/2a8d41b42688

  BaseCreate() is called with a non-NULL parent widget only for child windows
  with aParent non-null.

There seems to be an expectation that if GetParent() returns non-null, it will
return the same as what is passed to BaseCreate():

  nsBaseWidget::Destroy() uses GetParent() and RemoveChild() if that is
  non-null.

  nsBaseWidget::RemoveChild() asserts aChild->GetParent() == this.

  nsBaseWidget::SetZIndex() uses GetParent()

nsBaseWidget::GetTopLevelWidget() (for all platforms) also uses GetParent():

  nsDocumentViewer::CreateDeviceContext uses nsIWidget::GetTopLevelWidget()
  and I assume this doesn't want owner windows.

  Use of GetTopLevelWidget in nsEventStateManager::GenerateMouseEnterExit()
  looks like it doesn't want owner windows.

  I wondered whether nsGtkIMModule::SetCursorPosition might have wanted
  GetTopLevelWidget() to return owner windows.  It should be consistent with
  NS_QUERY_TEXT_RECT.  But nsGtkIMModules are not created for popups.

I'm thinking I'll try setting GTK's nsWindow mParent to the parameter passed to
BaseCreate, which is consistent with what is passed to BaseCreate in the WINNT
implementation.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Chris Coulson from comment #6)
> This worked before because the viewWidget in
> nsLayoutUtils::TranslateWidgetToView() used to be a child window, which had
> no reference to it's parent.

Thanks for working this out.

> (nsWindow::mParent was null - another bug?)

Possibly.  I don't know, but this may not be important now.  If we need to pass something different to BaseCreate() when aParent is now, I'd like to deal with that separately.

Comment 9

[Karl Tomlinson (:karlt)]

I mean, if we need to pass something different to BaseCreate() when aParent is NULL ...

Comment 10

[Timothy Nikkel (:tnikkel)]

Yeah, I remember there being inconsistencies and things that needed to be fixed in this area but I didn't touch it because I couldn't point to anything that was specifically broken at the time and didn't want to break something that was working.

Comment 11

[Timothy Nikkel (:tnikkel)]

So we could fix the code in nsLayoutUtils to stop when it hits a popup widget, or we could make the parent of the popup be null. Either (or both) change is good I think.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

We can take a low risk fix for uplift, but this wouldn't be a release blocking bug.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> On WINNT:

>   BaseCreate() is called with a non-NULL parent widget only for child windows
>   with aParent non-null.

I got that wrong.
BaseCreate() is called with a non-NULL parent on WINNT for popup windows. 

>   nsBaseWidget::Destroy() uses GetParent() and RemoveChild() if that is
>   non-null.
> 
>   nsBaseWidget::RemoveChild() asserts aChild->GetParent() == this.

So I wonder what removes references to popup windows from their parent before
the parent is destroyed.

Anyway, making the parent of the popup null, for GetParent() and BaseCreate() passes GTK mochitests;
https://tbpl.mozilla.org/?tree=Try&rev=2a7652b5982e

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: make only child windows have parents

Comment 15

[Karl Tomlinson (:karlt)]

Test passes for Cocoa and GTK.
Haven't looked much into what's going wrong on WINNT, yet.
https://tbpl.mozilla.org/?tree=Try&rev=c8aea27ac693

Comment 16

[Mats Palmgren (inactive)]

Attachment: GetParent() hack

The gtk2 nsWindow::Create seems consistent with Windows currently:
http://hg.mozilla.org/mozilla-central/annotate/69aaf27c6841/widget/windows/nsWindow.cpp#l442

That is, the same condition is used to null out the widget passed to
BaseCreate and mParent is unconditionally set to aParent when it's non-null.
What differs is the GetParent() method, so why not fix that instead?

This patch seems to fix the bug too, but what I don't understand is why
Windows GetParent() returns null for popups since 'mIsTopWidgetWindow'
would be false in that case (and you said aParent is non-null for popups).
http://hg.mozilla.org/mozilla-central/annotate/69aaf27c6841/widget/windows/nsWindow.cpp#l972
Do we get to "::GetAncestor(mWnd, GA_PARENT)" and it returns NULL?

I think I'd prefer to hack gtk2 GetParent() to return null in the same cases
as on Windows rather than change mParent (ownership), which is currently
mostly compatible across platforms.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Mats Palmgren [:mats] from comment #16)
> The gtk2 nsWindow::Create seems consistent with Windows currently:
> http://hg.mozilla.org/mozilla-central/annotate/69aaf27c6841/widget/windows/
> nsWindow.cpp#l442
> 
> That is, the same condition is used to null out the widget passed to
> BaseCreate

Yes, the BaseCreate() aParent parameter is consistent between GTK and NT.  I
suspect that is very old code that was copied, and from the comment I'd guess
the copier didn't understand it.

Cocoa is different again (comment 7).

> and mParent is unconditionally set to aParent when it's non-null.

That is similar though the behavior when aParent is null is different.

mParent is a toolkit-specific implementation variable.  It is not in
nsBaseWidget.  Beyond GetParent(), it is used only in SetParent() as a
shortcut for GetParent(), and in an NS_ASSERTION in SetFocus() that I may need
to fix.

I gather windows/nsWindow.h's mParent is set more often to avoid some crashes.
I don't expect that to be necessary in the GTK port. 

> What differs is the GetParent() method, so why not fix that instead?

The main reason is that having an nsBaseWidget child whose GetParent() returns
null seems confusing and inconsistent with what nsBaseWidget expects.  Do you
know a reason why the windows/nsWindow.cpp would not be leaking child popup
windows (comment 13)?

> Do we get to "::GetAncestor(mWnd, GA_PARENT)" and it returns NULL?

We get there and it is returning 0x10014, which I assume is a root window or
similar.  That is consistent with the documentation for GA_PARENT at
http://msdn.microsoft.com/en-nz/library/windows/desktop/ms633502%28v=vs.85%29.aspx
GetNSWindowPtr does not know about 0x10014, returning NULL.

> I think I'd prefer to hack gtk2 GetParent() to return null in the same cases
> as on Windows rather than change mParent (ownership), which is currently
> mostly compatible across platforms.

My main concern is with the nsBaseWidget concepts of child and parent.  We can
sneak in extra references to owner windows if necessary, but that adds a
different concept of a parent, adding more code and making things more
complex, so I'd like to avoid that if possible, and I don't have a good reason
for a popup to keep a reference to its owner.

Comment 18

[Karl Tomlinson (:karlt)]

Still investigating why my test is getting incorrect coordinates on WINNT, differing by about the size of the main window decorations (despite this bug not existing on that platform).
https://tbpl.mozilla.org/?tree=Try&rev=88d6470c3961

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 724319 [details] [diff] [review]
make only child windows have parents

> mParent is a toolkit-specific implementation variable.  It is not in
> nsBaseWidget.  

I disagree, mParent is effectively cross-platform.  I think GetParent/
SetParent should have the same semantics across platforms, because it's
used from view/frame code that is cross-platform.  Widget parent/child
ownership must also be the same on all platforms.

Therefore I think we should move mParent to nsBaseWidget and make
Get/SetParent behave the same on all platforms.

I'd prefer GetParent() to simply return mParent, as on gtk2, cocoa,
android etc.  If a separate concept of "IsTopLevelWidget" is needed
for some consumers, then let's add that.

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: handle popup windows with parents in GetWidgetOffset

I'm undecided on what nsIWidget::GetParent() should return when there is no parent.  I don't consider owner windows to be parents, but the Create() methods do, so it would be nice to be consistent with Create().  However, Create() has its own peculiarities with different behavior for native and XP parents.
So handling nsIWidget's quirks in layout seems the easiest solution here.

There are probably multiple permutations of WindowType and GetParent that would produce the same results here, but I've picked one to try to make it clear what this is doing: finding the offset to the non-child ancestor window.

Comment 22

[Karl Tomlinson (:karlt)]

The test fails on WINNT so I'll file a bug and disable it there.
https://tbpl.mozilla.org/?tree=Try&rev=fb720f9a4e70

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #18)
> Still investigating why my test is getting incorrect coordinates on WINNT,
> differing by about the size of the main window decorations (despite this bug
> not existing on that platform).
> https://tbpl.mozilla.org/?tree=Try&rev=88d6470c3961

Presumably a mixup between inner and outer bounds of the widget somewhere in the Windows code?

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8422052 [details] [diff] [review]
handle popup windows with parents in GetWidgetOffset

It'd be great if we had a consistent story for parent widgets, but I don't think we need to block this on fixing that.

Comment 25

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e88d5d9505
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35b6341335c

Filed bug 1009955 for NT.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7e88d5d9505
https://hg.mozilla.org/mozilla-central/rev/a35b6341335c