Last Comment Bug 400976 - XUL menupopups in HTML document disappear in Firefox 2.0.0.8
: XUL menupopups in HTML document disappear in Firefox 2.0.0.8
Status: RESOLVED FIXED
: fixed1.8.1.9, regression, verified1.8.1.10
Product: Core
Classification: Components
Component: XUL (show other bugs)
: 1.8 Branch
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 384105 400744
  Show dependency treegraph
 
Reported: 2007-10-24 09:25 PDT by Marcus Better
Modified: 2008-07-31 03:25 PDT (History)
19 users (show)
dveditz: blocking1.8.1.9+
dveditz: blocking1.8.1.10+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (961 bytes, text/html)
2007-10-24 09:28 PDT, Marcus Better
no flags Details
Screenshot (12.59 KB, image/png)
2007-10-24 16:09 PDT, Ria Klaassen (not reading all bugmail)
no flags Details
possible patch (2.69 KB, patch)
2007-10-25 03:39 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
this one (2.74 KB, patch)
2007-10-25 03:45 PDT, Olli Pettay [:smaug]
enndeakin: review+
Details | Diff | Review
v3 (2.89 KB, patch)
2007-10-25 04:10 PDT, Olli Pettay [:smaug]
neil: review+
Details | Diff | Review
v4, check also that mPopup hasn't been moved (2.80 KB, patch)
2007-10-25 06:32 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
v5, fix the "typo" in previous patch (2.81 KB, patch)
2007-10-25 07:01 PDT, Olli Pettay [:smaug]
dveditz: superreview+
dveditz: approval1.8.1.9+
dveditz: approval1.8.1.10+
Details | Diff | Review

Description Marcus Better 2007-10-24 09:25:01 PDT
User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.5; Linux) KHTML/3.5.7 (like Gecko) (Debian)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071004 Iceweasel/2.0.0.8 (Debian-2.0.0.8-1)

I have an HTML web app that uses some XUL elements to provide pop-up menus. This worked fine in Firefox before 2.0.0.8. In version 2.0.0.8 the pop-ups  disappear very quickly after being shown. A workaround is to apply a small delay with setTimeout before showPopup.


Reproducible: Always
Comment 1 Marcus Better 2007-10-24 09:28:54 PDT
Created attachment 286021 [details]
Test case

This shows a popup menu with the text "POPUP!". When run in Firefox 2.0.0.8 you can see that the popup only flashes very quickly and then disappears. If you comment out the call to elt.showPopup() and uncomment the setTimeout line instead, the popup stays on screen.

The test case is not perfect since it doesn't work in earlier versions. On 2.0.0.7 or 2.0.0.6 it gives an error: "elt.showPopup is not a function". However my full webapp does work on earlier versions and creates popups in much the same way.
Comment 2 Ria Klaassen (not reading all bugmail) 2007-10-24 14:56:16 PDT
This was repaired on trunk but it regressed again. Regression range for this is  http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1186421100&maxdate=1186423079 so it is probably caused by Bug 355789.
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-24 15:40:32 PDT
Marcus, thanks for the testcase. Is there any chance we can test the webapp in question too?

This seems to have regressed on branch between 2007-10-03 and 2007-10-04:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-03+03&maxdate=2007-10-04+05&cvsroot=%2Fcvsroot
Regression from bug 398404?

I suspect a relation with bug 400744 too.
Comment 4 Rob Arnold [:robarnold] 2007-10-24 15:47:16 PDT
The patch for bug 355789 wasn't checked in to branch (which 2.0.0.8 was built
from). Does this problem occur on trunk?
Comment 5 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-24 15:57:07 PDT
It is worksforme, using current trunk build, but I'm using the win2000 theme here.
However, I see a scrollbar appearing in the document, for a brief while, on trunk.
Comment 6 Ria Klaassen (not reading all bugmail) 2007-10-24 16:09:11 PDT
Created attachment 286077 [details]
Screenshot

This is what I see on trunk before I give another window focus. Then it disappears entirely.
Comment 7 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-24 16:17:37 PDT
(In reply to comment #6)
> This is what I see on trunk before I give another window focus. Then it
> disappears entirely.

Ok, that's what I can see too. I filed bug 401027 as a trunk bug.

This bug is now meant as the regression that can be seen on branch.
Comment 8 Boris Zbarsky [:bz] 2007-10-24 21:20:32 PDT
I tried effectively backing out bug 398404, and I still see this problem on branch.
Comment 9 Boris Zbarsky [:bz] 2007-10-24 21:35:45 PDT
So the menu comes down with the following stack:

#0  nsMenuPopupFrame::HideChain (this=0x8dceaa4)
    at ../../../../../mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:1855
#1  0xb55b0229 in nsMenuDismissalListener::Rollup (this=0x8dd8528)
    at ../../../../../mozilla/layout/xul/base/src/nsMenuDismissalListener.cpp:110
#2  0xb67ab9ca in nsWindow::Destroy (this=0x8b14d70)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:391
#3  0xb57d6c3b in ~nsView (this=0x8b14d18) at ../../../mozilla/view/src/nsView.cpp:266
#4  0xb57d6e08 in nsIView::Destroy (this=0x8b14d18)
    at ../../../mozilla/view/src/nsView.cpp:304
...
#12 0xb53747c2 in nsFrameManager::RemoveFrame (this=0x8d75dcc, aParentFrame=0x8dca4f8, 
    aListName=0x0, aOldFrame=0x8dceaa4)
    at ../../../mozilla/layout/base/nsFrameManager.cpp:717
#13 0xb5332597 in nsCSSFrameConstructor::ContentRemoved (this=0x8da49e8, 
    aContainer=0x8c07e90, aChild=0x8b90c28, aIndexInContainer=0, aInReinsertContent=0)
...
#18 0xb533aa7f in nsCSSFrameConstructor::RestyleEvent::HandleEvent (this=0x8b13748)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:14286

I have no idea what this restyle is and why it's apparently happening later than it used to on branch.
Comment 10 Boris Zbarsky [:bz] 2007-10-24 21:39:12 PDT
I also backed out bug 393770 and I still see the problem.
Comment 11 Boris Zbarsky [:bz] 2007-10-24 21:40:07 PDT
At this point I start to wonder about the range...  Someone should really try to do a pull by date and see what happens.
Comment 12 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-24 21:46:06 PDT
I'm terribly sorry!
I had my Firefox2 build described as 2007-10-04, but it turns out it is from 2007-10-06.
I'll narrow the regression range again from there.
Again, sorry for this.
Comment 13 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-24 21:58:16 PDT
Ok, the regression on branch is between 2007-10-04 and 2007-10-05:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-04+03&maxdate=2007-10-05+05&cvsroot=%2Fcvsroot
which makes bug 384105 to be a likely candidate for the regression.
Comment 14 Boris Zbarsky [:bz] 2007-10-24 22:04:18 PDT
Ah, yes.  That I would buy.
Comment 15 Boris Zbarsky [:bz] 2007-10-24 22:09:59 PDT
Backing out bug 384105 fixes this.

Are we perhaps ungenerating after the (following?) open has happened because of the event thing?
Comment 16 Marcus Better 2007-10-25 01:12:37 PDT
(In reply to comment #3)
> Marcus, thanks for the testcase. Is there any chance we can test the webapp
> in question too?

Sure, send me a private mail if you still need it.
Comment 17 Olli Pettay [:smaug] 2007-10-25 03:39:04 PDT
Created attachment 286135 [details] [diff] [review]
possible patch

This is not perfect, but I don't know what else could be done.
If we recreate the frame for menu, don't unset menugenerated.
Comment 18 Olli Pettay [:smaug] 2007-10-25 03:45:51 PDT
Created attachment 286137 [details] [diff] [review]
this one
Comment 19 Olli Pettay [:smaug] 2007-10-25 03:54:15 PDT
Comment on attachment 286137 [details] [diff] [review]
this one

Neils, what do you think about this.
Comment 20 Neil Deakin 2007-10-25 03:56:23 PDT
Comment on attachment 286137 [details] [diff] [review]
this one


>+    : mMenu(aMenu), mContent(aContent)

Maybe mContent(aContent) should be mPopup(aPopup) so it is clearer what node it refers to.

>+    if (mMenu) {

Could just null check this before creating the event, or just assert since this shouldn't be null.

Otherwise, looks OK.
Comment 21 Olli Pettay [:smaug] 2007-10-25 04:10:59 PDT
Created attachment 286141 [details] [diff] [review]
v3

I'm asking extra review in hope to prevent regressions.
Comment 22 neil@parkwaycc.co.uk 2007-10-25 05:55:50 PDT
Comment on attachment 286141 [details] [diff] [review]
v3

I was trying to work out which frame it was better to check for but I think checking for the menu frame is OK.

As for the null-check, child is (surprise) a child of mContent so it's going to be null when mContent is ;-)
Comment 23 Olli Pettay [:smaug] 2007-10-25 06:32:28 PDT
Created attachment 286156 [details] [diff] [review]
v4, check also that mPopup hasn't been moved
Comment 24 Olli Pettay [:smaug] 2007-10-25 06:57:23 PDT
Comment on attachment 286156 [details] [diff] [review]
v4, check also that mPopup hasn't been moved

>+          new nsASyncUngenerate(menu, child);

s/menu/GetContent()
Comment 25 Olli Pettay [:smaug] 2007-10-25 07:01:31 PDT
Created attachment 286157 [details] [diff] [review]
v5, fix the "typo" in previous patch
Comment 26 Mike Schroepfer 2007-10-25 09:30:15 PDT
Smaug/Bz any comments on risk level of this patch - we are about ready to start a 2009 and are not sure if we should take this.  Also - does this only affect XUL elements?
Comment 27 Olli Pettay [:smaug] 2007-10-25 10:24:20 PDT
This is "only" about xul popups/menus behaving badly.
Not sure about bug 400744, though not sure if the patch fixes that either.
Risk should be low, but not minimal. The risk is there in cases where
xul popups/menus are used in some not-so-usual way.
Comment 28 Boris Zbarsky [:bz] 2007-10-25 10:59:05 PDT
If this patch fixes bug 400744 I think we should take it for 2.0.0.9.

If it doesn't, then I'm a little torn.  If popups in XUL documents are not affected (something that needs to be checked), then I'm not sure XUL is used enough in HTML (or supported enough in that configuration) to put this in the firedrill release.
Comment 29 Daniel Veditz [:dveditz] 2007-10-25 11:23:04 PDT
The testcase in bug 400744 appears to be a XUL dialog, and popups there were affected by this bug and fixed by this patch.
Comment 30 Mike Schroepfer 2007-10-25 11:34:48 PDT
So my $.02 is since there is a workaround, this deals with XUL popups only we should put this in 2.0.0.10 when we have more time to test it...

Comment 31 Daniel Veditz [:dveditz] 2007-10-25 12:25:32 PDT
Comment on attachment 286157 [details] [diff] [review]
v5, fix the "typo" in previous patch

sr=dveditz

approved for 1.8.1.9/1.8.1.10, a=dveditz for release-drivers
Comment 32 Olli Pettay [:smaug] 2007-10-25 12:35:37 PDT
Checked in.
Comment 33 Daniel Veditz [:dveditz] 2007-10-29 17:09:41 PDT
Added appropriate fixed keywords for branch release tracking.
Comment 34 Carsten Book [:Tomcat] 2007-11-15 08:07:52 PST
verified fixed 1.8.1.10 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.10pre) Gecko/2007111503 BonEcho/2.0.0.10pre and the testcase from this bug.

Testcase is working fine now - adding verified keyword

Note You need to log in before you can comment on or make changes to this bug.