Closed
Bug 349585
Opened 18 years ago
Closed 18 years ago
[reflow branch] Menupopups don't resize correctly when kids are changed
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
1.01 KB,
patch
|
Details | Diff | Splinter Review |
STEPS TO REPRODUCE: 1) Start Firefox. 2) Open the History or Bookmarks menu. 3) Note blank space at the bottom. 1') Start Seamonkey 2') Open the Edit menu 3') Note the scrolling arrows. This only happens the first time the relevant menu is open in a given window. to reproduce again, open a new window. The problem is that menupopups are reflow roots, so incremental reflows use the current height as the computed height, which is wrong if the height has actually changed.
Assignee | ||
Comment 1•18 years ago
|
||
In addition to the mComputedHeight and availableHeight we set, we also don't clear the mPrefSize.width in the nsBoxFrame that's the menupopup. If I do that, I get the right height, but wrong width (because some menus also change width; for example the Seamonkey bookmarks menu). So basically, for a menupopup the assumption that a reflow root's incremental reflow won't change its width and height and that intrinsic sizes need no invalidation is wrong. Do we change the assumption, or do we make menupopups not reflow roots? If we do the latter, we need to refix the issues with submenus of context menus in some other way.... Perhaps we need two different types of reflow roots?
Assignee: nobody → dbaron
Assignee | ||
Comment 2•18 years ago
|
||
It might be simpler (at least for the time being) to just set the parent of popups to the popupset and make them not be reflow roots. That parent would be more correct anyway, in my opinion. Roc, Mats, Neil can you think of any issues with doing that?
Comment 3•18 years ago
|
||
I'm not sure what you mean there; we have three sorts of menus: popup/context menus (which have a popupset), menubars and all other menus and submenus (which menubars are very similar to).
Assignee | ||
Comment 4•18 years ago
|
||
Neil, when a menupopup frame is created, if its parent content's frame is not a menu frame the menupopup is stuck into the root popupset (which comes off the root box). But the parent of the menupopup is not set to the popupset. I'd like to change that. Can you think of issues this would cause?
Assignee | ||
Comment 5•18 years ago
|
||
I'm particularly worried about fuzzer-type issues. Maybe I should just attach a patch and then we can run our XUL fuzzers on it?
Comment 6•18 years ago
|
||
(In reply to comment #4) >Neil, when a menupopup frame is created, if its parent content's frame is not a >menu frame the menupopup is stuck into the root popupset (which comes off the >root box). But the parent of the menupopup is not set to the popupset. I'd >like to change that. Can you think of issues this would cause? So this is a random popup which isn't already a child of a popupset or menu? If you mean changing the DOM parent, this will confuse the hell out of the textbox context menu which is anonymous content bound to anonymous content...
Assignee | ||
Comment 7•18 years ago
|
||
> So this is a random popup which isn't already a child of a popupset or menu? It's a random popup which is not a child of a menu. Being inside a popupset doesn't matter. See the isPopup variable in nsCSSFrameConstructor::ConstructXULFrame. > If you mean changing the DOM parent No, I mean changing the frame parent.
Comment 8•18 years ago
|
||
(In reply to comment #7) >>So this is a random popup which isn't already a child of a popupset or menu? >It's a random popup which is not a child of a menu. OK, it's just that comment #0 confuses me in this case. >See the isPopup variable in nsCSSFrameConstructor::ConstructXULFrame. So you construct the frame, and you add a placeholder where it would have gone, and you add the new frame to the root anonymous popupset, but you don't actually tell the new frame that, which is causing you problems? I don't foresee any issues in changing the parent to a (different) popupset.
Assignee | ||
Comment 9•18 years ago
|
||
> OK, it's just that comment #0 confuses me in this case. To fix issues with menupopups that are not kids of menus, I had to make changes to all menupopups, which caused this bug. > but you don't actually tell the new frame that, which is causing you > problems? Yes. > I don't foresee any issues in changing the parent to a (different) popupset. I don't think the current parent is necessarily a popupset, for what it's worth. ;) I'll post a patch for testing in a bit.
Assignee | ||
Comment 10•18 years ago
|
||
OK, I filed bug 349921 on changing the parent. That code has changed on trunk since the branch was cut, so instead of writing two patches I would like to just fix it on trunk and have branch pick up the changes automagically when dbaron merges it to tip. With those parent changes and the removal of the reflow root flag, both this bug and bug 349181 go away on reflow branch.
Depends on: 349921
Assignee | ||
Comment 11•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Assignee: dbaron → bzbarsky
Assignee | ||
Comment 12•18 years ago
|
||
Fixed, by checking in that patch.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•