393791 - While hovering over menu, popup shows briefly as 2px*2px on first load

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

To reproduce:
- Close Firefox and reopen
- Open the File menu by clicking on it.
- Now hover over the Edit, View, History, Bookmarks, Tools and Help menu

Result:
Before the normal popup is display, I see very briefly a 2px by 2px popup appear.
I see this only happening when you hover over one of these menus the first time. After that, it doesn't happen anymore.

I especially noticed this in Thunderbird trunk. I suspect it might be easier noticeable with slower computers.

I think this is a regression from bug 279703 (although I haven't looked for a regression range).

Comment 1

[Neil Deakin]

Martijn, you mentioned this before, but I couldn't reproduce it. I still can't. Does this tiny popup look like a normal one or just an empty tooltip?

Comment 2

[Martijn Wargers (dead)]

It looks like a normal tiny popup, not like an empty tooltip.
That being said, in today's trunk build it seems much more difficult to see (but I didn't have my morning coffee yet).

Comment 3

[Martijn Wargers (dead)]

Fwiw, I can see this more easily, when doing something cpu intensive on the side (like compiling stuff).

Comment 4

[Martijn Wargers (dead)]

This seems to have regressed recently, in the sense that I can see this more easily in the latest nightly builds than before, I think (or I am more easitly annoyed for the past few days).

Comment 5

[Jordan]

I reported this on the MozillaZine forums, and there are a few others(including myself) who have noticed it.

I can tell you that it does NOT happen after the first opening of each menu item.
If you open Firefox, and click on a menu item, you will see it. Clicking again without restarting Firefox will not produce the same bug.

From that, I draw the conclusion that it is a bug may be related to the initial populating of the menus.

I can only guess, however, since I do not have access to the source code.

Comment 6

[Andrés Delfino]

Marking as blocking as this makes Fx seem pretty unfinished.

Comment 7

[Martijn Wargers (dead)]

Yeah, I agree with the blocking flag, this has become much worse lately. It might be useful to find out what check-in caused it to make it worse.

Comment 8

[Steve England [:stevee]]

This problem was brought up in http://forums.mozillazine.org/viewtopic.php?t=633425 and if http://forums.mozillazine.org/viewtopic.php?p=3277949#3277949 is anything to go by, it may have got worse between 2008-02-28 21:10 and 2008-02-28 22:53 which gives a range of:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-28+21%3A10&maxdate=2008-02-28+22%3A53&cvsroot=%2Fcvsroot

So maybe bug 418552. Unfortunately I have a real hard time seeing this bug so I can't check if it did actually get worse between these two builds. But someone else who does see this might benefit from the range.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, I don't think this is 418552, at least it shouldn't be related especially on non-vista.  The changes there had no impact on widget sizing/showing/etc, just purely visual changes.

Comment 10

[Vlado Valastiak [:wladow]]

Attachment: screenshot showing the square

This deserves a screenshot. As it's hard to capture it i'm adding one taken from mozillazine (by fittysix).

Comment 12

[:glob ✱]

from bug 425402:

This regressed between 2008-02-22 05:00 and 2008-02-23 06:00, so the regression
window is
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-22+05%3A00%3A00&maxdate=2008-02-23+06%3A00%3A00&cvsroot=%2Fcvsroot

see attachment 312165 [details] for an animated gif of the issue.

Comment 13

[Martijn Wargers (dead)]

Byron, thanks! So with that regression range, it seems like the patch for bug 405719 made it worse.

Comment 14

[Thomas Bertels]

It's now sure, since I've been able to get this regression window down to http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-22+11%3A00%3A00+PDT&maxdate=2008-02-22+12%3A20%3A00+PDT&cvsroot=%2Fcvsroot with custom builds.

Comment 15

[Kai Liu]

If the problem is that this behavior has been around since before 2008-02-22 and that the changes between 2008-02-22 and 2008-02-23 made it worse (slowing the process down so that it is more noticeable), then perhaps the solution is not necessarily to speed it up (though that would be nice), but to hide the menu until it has finished populating?

Comment 16

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

We're doing nsWindow::Show on the popup when it's 6x6. Here's the stack:

	gkwidget.dll!nsWindow::Show(int bState=1)  Line 1677	C++
 	gklayout.dll!nsView::SetVisibility(nsViewVisibility aVisibility=nsViewVisibility_kShow)  Line 475	C++
 	gklayout.dll!nsViewManager::SetViewVisibility(nsIView * aView=0x048d3f20, nsViewVisibility aVisible=nsViewVisibility_kShow)  Line 1738	C++
 	gklayout.dll!nsMenuPopupFrame::AdjustView()  Line 330	C++
 	gklayout.dll!nsMenuFrame::DoLayout(nsBoxLayoutState & aState={...})  Line 790	C++
 	gklayout.dll!nsIFrame::Layout(nsBoxLayoutState & aState={...})  Line 563	C++

The interesting thing is, this is happening due to a layout flush caused by a paint event delivered to the main window. So we probably shouldn't be showing the menu just because of that.

Comment 17

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

The nsMenuPopupFrame has no child frames, so I guess we didn't get around to creating its children yet, and that's why it's vestigial.

Comment 18

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

Attachment: fix

This seems to fix it.

Neil, does this make sense? Any way to test this?

Comment 19

[Vlado Valastiak [:wladow]]

FYI square seems to be gone with this patch applied. Tested with WinVista.

Comment 20

[Neil Deakin]

Comment on attachment 316358 [details] [diff] [review]
fix

Looks OK. I can't reproduce the bug, but I can't think of a way to test it. You would need to be able to have a script run between the popupshowing and popupshown events, which I'm not sure is possible.

Comment 21

[[e-mail]]

Comment on attachment 316358 [details] [diff] [review]
fix

I'm sure I once used to see this too but now I can't reproduce it either... it looks reasonable though.

Comment 22

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

Comment on attachment 316358 [details] [diff] [review]
fix

cosmetic issue, but a very small fix.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316358 [details] [diff] [review]
fix

a1.9=beltzner

Comment 24

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

checked in

Comment 25

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

Backed out due to test failures

Comment 26

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

This caused a whole lot of mochitest failures, but interestingly only on Windows.

Comment 27

[Martijn Wargers (dead)]

These are the failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1208822429.1208826607.18572.gz

*** 51239 ERROR FAIL | cursor right skip disabled DOMMenuItemActive fired |  | /tests/toolkit/content/tests/widgets/test_menubar.xul
*** 51242 ERROR FAIL | Test timed out. |  | /tests/toolkit/content/tests/widgets/test_menubar.xul
*** 51245 ERROR FAIL | cursor right skip disabled popuphiding fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51247 ERROR FAIL | cursor right skip disabled popuphidden fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51249 ERROR FAIL | cursor right skip disabled DOMMenuItemInactive fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51250 ERROR FAIL | cursor right skip disabled DOMMenuInactive fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51251 ERROR FAIL | cursor right skip disabled DOMMenuBarInactive fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51252 ERROR FAIL | cursor right skip disabled DOMMenuItemInactive fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51253 ERROR FAIL | cursor right skip disabled DOMMenuItemInactive fired |  | /tests/toolkit/content/tests/widgets/test_menuchecks.xul
*** 51415 ERROR FAIL | Test timed out. |  | /tests/toolkit/content/tests/widgets/test_panelfrommenu.xul
*** 52069 ERROR FAIL | Test timed out. |  | /tests/toolkit/content/tests/widgets/test_popup_coords.xul
*** 52074 ERROR FAIL | Test timed out. |  | /tests/toolkit/content/tests/widgets/test_popup_keys.xul
*** 52075 ERROR FAIL | Too many test timeouts, giving up. |  | /tests/toolkit/content/tests/widgets/test_popup_keys.xul

window_menubar.xul is failing and timing out with the "cursor right skip disabled" test:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/tests/widgets/window_menubar.xul

I did some digging, and the order of DOMMenuItemActive and popupshown events are switched. There are comments in the window_menubar.xul file that indicates that windows is doing something different than other platforms currently. Maybe the patch in this bug is making windows do the same as other platforms.
Unfortunately, I couldn't really make a complete sense of what the test is doing, because of the complicated nature of it. But it seems to me that the test shouldn't have timed out at all. I think that's a bug in the test.

Comment 28

[Martijn Wargers (dead)]

Attachment: zipped up testcase that shows difference with patch

This zipped up testcase shows the difference with and without the patch (sorry, the testcase is rather clumsy):
- Open file named "editor_error.htm", click on the button
- Press Alt-F
- Press the right arrow key
- Click with the mouse in the document to dismiss the popup

Result without the patch on the left, result with the patch on the right:
DOMMenuBarActive    DOMMenuBarActive
popupshowing        popupshowing
DOMMenuItemActive   DOMMenuItemActive
popupshown          DOMMenuItemActive
DOMMenuItemActive   popupshown
DOMMenuItemInactive DOMMenuItemInactive
DOMMenuItemActive   DOMMenuItemActive
popuphiding         popuphiding
popuphidden         popuphidden
popupshowing        popupshowing
DOMMenuItemInactive DOMMenuItemInactive
DOMMenuInactive     DOMMenuInactive
popupshown          DOMMenuItemActive
DOMMenuItemActive   popupshown
popuphiding         popuphiding
popuphidden         popuphidden
DOMMenuItemInactive DOMMenuItemInactive
DOMMenuInactive     DOMMenuInactive
DOMMenuBarInactive  DOMMenuBarInactive
DOMMenuItemInactive DOMMenuItemInactive
DOMMenuItemInactive DOMMenuItemInactive

Neil, could you perhaps comment/explain about what's going on?

Comment 29

[Neil Deakin]

The DOMMenuXXX events fire while the popup is opening when asynchonous events fire. The popupshown event is fired asyncronously after a layout. They may be fired in a different order if some style rule or a flush occurs in-between.

It doesn't matter that the two events (DOMMenuItemActive and popupshown) fire in a different order as long as they both fire. It actually makes sense that this patch  fixes this so that it is consistent on all platforms, as this patch ensures that the layout doesn't occur until the child frames are created, and thus the popupshown event doesn't fire until later.

Comment 30

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

I got about this far in my own debugging.

It's unfortunate that the tests just hang when events fire in the wrong order.

Comment 31

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

Attachment: fix

Original patch plus fix for tests.

With this patch, all the toolkit/content/widget tests pass on Windows for me. I'm not sure why they were all fixed by this patch, since the only substantive changes are in window_menubar.xul, but perhaps the other failures were cascading from the first failure in menubar.

Comment 32

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

I'm going to reland this with the test fixes.

Comment 33

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

Relanded. Turns out this is tested, really, since the tests check the order of events and the order of events without this fix is different, which is why we had to change the tests.

Comment 34

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre

Thanks for fixing this, Robert!

Comment 35

[Thomas Goldstein]

However, it seems like menu dropdowns now flicker very briefly as you switch between them. Someone at Mozillazine also reported menu lags since this fix got checked in.

I actually managed to take a screen of the slight flickering I witness: http://stifu.free.fr/pics/temp/fx-menu-shadow.png

Basically, the shadow moves too slowly.

Comment 36

[Thomas Goldstein]

Correction: the menu dropdown shadow actually moves *too fast*...

Comment 37

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

Hmm. Is that worse than the 2x2 problem? if so, we should back this out.

Comment 38

[Vlado Valastiak [:wladow]]

(In reply to comment #35)

I don't see flickering or lags with Mozilla/5.0 (Windows; U; Windows NT 6.0; sk; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre

How to reproduce this?

Comment 39

[Jim Jeffery not reading bug-mail 1/2/11]

WFM, is this being seen by users that use 'classic windows' ?

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre Firefox/3.0 ID:2008042407

Comment 40

[Kai Liu]

WFM in Classic...

...but if I move my mouse around very quickly in the menu bar, opening and closing menus as I swipe the mouse across, there is what appears to be a minor lag in the painting, but it's not very noticeable, nor do I think it's even specific to Fx3 (though it does feel a bit more pronounced in 3 than it was in 2).  I'm not sure if that's what comment 35 was referring to, but if it is, then the 2x2 thing is worse than this.

Comment 41

[Thomas Goldstein]

I managed to reproduce the bug with a new profile: http://stifu.free.fr/pics/temp/fx-menu-shadow2.png

However, that screen was a bit harder to catch, as the flickering was much faster than with my usual profile, most likely due to the fact it was all fresh and clean, with no bookmark or extra data to load.

I'm on Windows XP SP2, and I'm not in "classic windows" mode (although I set the Windows bars to grey rather than blue, but "XP style" grey... but I don't think that makes a difference).

I'm not *100% sure* this problem was introduced by this bug fix, but I think it was (as I never noticed it before). I'll have to do more testing later by going back to an older build.

Comment 42

[Thomas Goldstein]

I downgraded to Gecko/2008042207 Minefield/3.0pre, and I can confirm the shadow desynch was already in it... So never mind my comments, that was all unrelated. Sorry about that.