Closed Bug 835044 Opened 11 years ago Closed 10 years ago
.0b+ on Linux (Windows OK) selects wrong row in XUL tree in level="parent" panel
4.24 KB, application/vnd.mozilla.xul+xml
3.25 KB, patch
|Details | Diff | Splinter Review|
685 bytes, patch
|Details | Diff | Splinter Review|
1.63 KB, patch
|Details | Diff | Splinter Review|
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.
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).
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
In local build: Last Good: 8da9c24417d0 First Bad: a0158d370785 Regressed by: a0158d370785 Karl Tomlinson — b=793501 attach DocumentViewer to top GTK widget r=roc
Summary: Firefox 19.0b+ on Linux (Windows OK) selects wrong row in XUL tree → Firefox 19.0b+ on Linux (Windows OK) selects wrong row in XUL tree in noautohide panel
Summary: Firefox 19.0b+ on Linux (Windows OK) selects wrong row in XUL tree in noautohide panel → Firefox 19.0b+ on Linux (Windows OK) selects wrong row in XUL tree in level="parent" panel
Looks like screen coordinates are being used for widget coordinates somewhere.
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.
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.
(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.
I mean, if we need to pass something different to BaseCreate() when aParent is NULL ...
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.
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.
We can take a low risk fix for uplift, but this wouldn't be a release blocking bug.
(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
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
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.
(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.
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 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.
Attachment #724319 - Flags: review?(matspal) → review-
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.
Attachment #8422052 - Flags: review?(tnikkel)
The test fails on WINNT so I'll file a bug and disable it there. https://tbpl.mozilla.org/?tree=Try&rev=fb720f9a4e70
(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 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.
Attachment #8422052 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e88d5d9505 https://hg.mozilla.org/integration/mozilla-inbound/rev/a35b6341335c Filed bug 1009955 for NT.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.