User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030312 Phoenix/0.5 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Images (in this case, navigation arrows) don't appear in the drop down DHTML menus on this site or others that use this menu system (another example is http://www.washingtonhikes.com). This works in mozilla 1.3b and earlier, doesn't work in any of the 1.4a+ builds. Reproducible: Always Steps to Reproduce: 1. Go to http://www.burmees.nl/menu/menus.htm or http://www.washingtonhikes.com 2. Mouse over menus, watch menu items that expand to show submenus Actual Results: Menu expands correctly and is completely functional, but arrows don't appear in menu items that have submenus. Expected Results: Arrow graphics should appear in menus that expand Menu is also available at http://www.dynamicdrive.com/dynamicindex1/hvmenu/index.htm

Is this a problem with April 17th or newer builds? I checked in a fix for a bunch of problems like this a few days ago....

Still occurring in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030418

OK, this broke sometime between 2003-03-18-05 and 2003-03-19-05, so it's likely my checkin. It's still broken in a current nightly build. I can't tell what could be going wrong, unfortunately. A testcase or at least a pointer to where the JS creates the images would be much appreciated.

mine

Attaching DHTML menu templates

Menu uses two .js files; one just for vars... source for images comes from an array defined in exmplmenu_var.js: var Arrws=['tri1.gif',5,10,'tri2.gif',10,5,'tri3.gif',5,10]; // Arrow source, width and height Images are displayed via: if(eval(WhatMenu+'[3]')&&ShowArrow){ a=RcrsLvl==1&&FirstLineHorizontal?3:RightToLeft?6:0; S=Arrws[a]; W=Arrws[a+1]; H=Arrws[a+2]; T=RcrsLvl==1&&FirstLineHorizontal?Hght-H-2:(Hght-H)/2; L=RightToLeft?2:Wdth-W-2; if(DomYes){ t=Location.document.createElement('img'); this.appendChild(t); t.style.position='absolute'; t.src=S; t.style.width=W; t.style.height=H; t.style.top=T; t.style.left=L} else{ MemVal+="<div style='position:absolute; top:"+T+"; left:"+L+"; width:"+W+"; height:"+H+";visibility:inherit'><img src='"+S+"'></div>"; this.innerHTML=MemVal}} I've attached the menu templates...

CantRenderReplacedElement is slowly heading towards being on my hit list...

Comment on attachment 121102 [details] [diff] [review] Patch. I'm somewhat worried about bailing out if Init() fails... other frame impls could be moronic like nsFramesetFrame and return failure just for the hell of it... So I'd be fine with holding off on that error return in ConstructHTMLFrame till 1.5; the rest of these changes should be just fine for 1.4.

Comment on attachment 121102 [details] [diff] [review] Patch. I'm still sorting out whether CantRenderReplacedElement properly destroys frames in this case.... This patch is not quite right.

OK, this fixes all the issues I had with the first patch... Had to fix a random unrelated crash in RecreateFramesForContent along the way.

Comment on attachment 121172 [details] [diff] [review] Same as diff -w; for review only I'd love to have dbaron look at this, but it's not clear whether he would be able to before freeze... rbs? roc? Could you guys sanity-check this?

Comment on attachment 121172 [details] [diff] [review] Same as diff -w; for review only r/sr=rbs, bug 122748 comment 1, I once wondered what happens to those that can't be rendered. Now I see. Because you are adding the destroy() and there is the risk of "moronic" failures, it might be safe to wait a bit by instead doing: + rv = InitAndRestoreFrame(aPresContext, aState, aContent, + geometricParent, aStyleContext, nsnull, newFrame); + if (NS_FAILED(rv)) { + if (rv == NS_ERROR_FRAME_REPLACED) { add comment: // impls destroy the frame + return NS_OK; + } + #if defined(DEBUG_bzbarsky) || defined(DEBUG_dbaron) // XXXbz - cleanup pending NS_ASSERTION(NS_SUCCEEDED(rv), "InitAndRestoreFrame failed - destroying frame"); + newFrame->Destroy(aPresContext); #endif + return rv; + } There are zillions of code-paths and I doubt all the "moronic" cases will be caught in such a short time frame. Suffer the leaks for a little while, for the frames are arena-allocated and the leaks go away with the arena. So you have a little breadth to catch the fragile cases. Remnant of your other checkin (from bug 202717): - // Set the frame's initial child list + if (childItems.childList) {// Set the frame's initial child list newFrame->SetInitialChildList(aPresContext, nsnull, childItems.childList); } + } + }

+ if (rv == NS_ERROR_FRAME_REPLACED) { add comment: // impls destroy the frame + return NS_OK; + } Hmm.. That's _very_ different from my patch.... To ignore other moronic cases, I could ignore error returns other than NS_ERROR_FRAME_REPLACED (and assert) for now.... Would that make sense? We may want to do that for 1.4b/1.4....

Ops, I read both patches but picked a piece from a wrong patch attachment 121102 [details] [diff] [review] when commenting. >To ignore other moronic cases, I could ignore error returns other than >NS_ERROR_FRAME_REPLACED (and assert) for now.... Would that make sense? That's what I meant, but you said it more clearly. That is, retain the old behavior , but with some asserts to catch the moronic cases so that the right thing (tm) can be done gradually.

Ultimately, don't we want replacement failure to be detected and handled in content anyway?

Ultimately, yes. At least partially... But not for 1.4. ;)

Comment on attachment 121172 [details] [diff] [review] Same as diff -w; for review only I'd prefer the name aContentParent to aViewParent. The view hierarchy mirrors the frame hierarchy so the actual parent view is actually obtained by following the frame hierachy up aGeometricParent :-).

Comment on attachment 121228 [details] [diff] [review] Patch updated to review comments carrying the r= yep, this patch is conservative and much safer/robust for transition.

Checked in.