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)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
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
Blocks: 349181
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?
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).
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?
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?
(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...
> 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.
(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.
> 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.
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: dbaron → bzbarsky
Fixed, by checking in that patch.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
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.

Attachment

General

Created:
Updated:
Size: