Closed Bug 400976 Opened 17 years ago Closed 17 years ago

XUL menupopups in HTML document disappear in Firefox 2.0.0.8

Categories

(Core :: XUL, defect)

1.8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: marcus, Assigned: smaug)

References

Details

(Keywords: fixed1.8.1.9, regression, verified1.8.1.10)

Attachments

(3 files, 4 obsolete files)

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
Attached file Test case (deleted) —
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.
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.
Blocks: 355789
Keywords: regression
OS: Linux → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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?
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.
Attached image Screenshot (deleted) —
This is what I see on trunk before I give another window focus. Then it disappears entirely.
(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.
Version: unspecified → 1.8 Branch
No longer blocks: 355789
I tried effectively backing out bug 398404, and I still see this problem on branch.
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.
I also backed out bug 393770 and I still see the problem.
At this point I start to wonder about the range... Someone should really try to do a pull by date and see what happens.
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.
Ah, yes. That I would buy.
Backing out bug 384105 fixes this. Are we perhaps ungenerating after the (following?) open has happened because of the event thing?
Blocks: 384105
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.10?
Assignee: nobody → Olli.Pettay
(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.
Attached patch possible patch (obsolete) (deleted) — Splinter Review
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.
Attachment #286135 - Flags: review?(enndeakin)
Attached patch this one (obsolete) (deleted) — Splinter Review
Attachment #286135 - Attachment is obsolete: true
Attachment #286137 - Flags: review?(enndeakin)
Attachment #286135 - Flags: review?(enndeakin)
Comment on attachment 286137 [details] [diff] [review] this one Neils, what do you think about this.
Attachment #286137 - Flags: review?(neil)
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.
Attachment #286137 - Flags: review?(enndeakin) → review+
Attached patch v3 (obsolete) (deleted) — Splinter Review
I'm asking extra review in hope to prevent regressions.
Attachment #286137 - Attachment is obsolete: true
Attachment #286141 - Flags: superreview?(roc)
Attachment #286141 - Flags: review?(neil)
Attachment #286137 - Flags: review?(neil)
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 ;-)
Attachment #286141 - Flags: review?(neil) → review+
Blocks: 400744
Attached patch v4, check also that mPopup hasn't been moved (obsolete) (deleted) — Splinter Review
Attachment #286141 - Attachment is obsolete: true
Attachment #286156 - Flags: superreview?(roc)
Attachment #286141 - Flags: superreview?(roc)
Comment on attachment 286156 [details] [diff] [review] v4, check also that mPopup hasn't been moved >+ new nsASyncUngenerate(menu, child); s/menu/GetContent()
Attachment #286156 - Attachment is obsolete: true
Attachment #286157 - Flags: superreview?(roc)
Attachment #286156 - Flags: superreview?(roc)
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?
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.
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.
The testcase in bug 400744 appears to be a XUL dialog, and popups there were affected by this bug and fixed by this patch.
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 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
Attachment #286157 - Flags: superreview?(roc)
Attachment #286157 - Flags: superreview+
Attachment #286157 - Flags: approval1.8.1.9+
Attachment #286157 - Flags: approval1.8.1.10+
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Added appropriate fixed keywords for branch release tracking.
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.11?
Flags: blocking1.8.1.10+
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
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: