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)
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)
(deleted),
text/html
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
dveditz
:
superreview+
dveditz
:
approval1.8.1.9+
dveditz
:
approval1.8.1.10+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•17 years ago
|
||
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•17 years ago
|
||
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.
Updated•17 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
This is what I see on trunk before I give another window focus. Then it disappears entirely.
Comment 7•17 years ago
|
||
(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
Comment 8•17 years ago
|
||
I tried effectively backing out bug 398404, and I still see this problem on branch.
Comment 9•17 years ago
|
||
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•17 years ago
|
||
I also backed out bug 393770 and I still see the problem.
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Ah, yes. That I would buy.
Comment 15•17 years ago
|
||
Backing out bug 384105 fixes this.
Are we perhaps ungenerating after the (following?) open has happened because of the event thing?
Updated•17 years ago
|
Assignee: nobody → Olli.Pettay
Reporter | ||
Comment 16•17 years ago
|
||
(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.
Assignee | ||
Comment 17•17 years ago
|
||
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)
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #286135 -
Attachment is obsolete: true
Attachment #286137 -
Flags: review?(enndeakin)
Attachment #286135 -
Flags: review?(enndeakin)
Assignee | ||
Comment 19•17 years ago
|
||
Comment on attachment 286137 [details] [diff] [review]
this one
Neils, what do you think about this.
Attachment #286137 -
Flags: review?(neil)
Comment 20•17 years ago
|
||
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+
Assignee | ||
Comment 21•17 years ago
|
||
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 22•17 years ago
|
||
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+
Assignee | ||
Comment 23•17 years ago
|
||
Attachment #286141 -
Attachment is obsolete: true
Attachment #286156 -
Flags: superreview?(roc)
Attachment #286141 -
Flags: superreview?(roc)
Assignee | ||
Comment 24•17 years ago
|
||
Comment on attachment 286156 [details] [diff] [review]
v4, check also that mPopup hasn't been moved
>+ new nsASyncUngenerate(menu, child);
s/menu/GetContent()
Assignee | ||
Comment 25•17 years ago
|
||
Attachment #286156 -
Attachment is obsolete: true
Attachment #286157 -
Flags: superreview?(roc)
Attachment #286156 -
Flags: superreview?(roc)
Comment 26•17 years ago
|
||
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?
Assignee | ||
Comment 27•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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+
Assignee | ||
Comment 32•17 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
Comment 33•17 years ago
|
||
Added appropriate fixed keywords for branch release tracking.
Keywords: fixed1.8.1.10,
fixed1.8.1.9
Updated•17 years ago
|
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.11?
Flags: blocking1.8.1.10+
Comment 34•17 years ago
|
||
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
Keywords: fixed1.8.1.10 → verified1.8.1.10
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.
Description
•