[reflow branch] Menupopups don't resize correctly when kids are changed

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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

12 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)

Updated

12 years ago
Blocks: 349181
(Assignee)

Comment 2

12 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

12 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

12 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

12 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

12 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

12 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

12 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

12 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

12 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

12 years ago
Created attachment 235122 [details] [diff] [review]
Branch patch that we'll need
(Assignee)

Updated

12 years ago
Assignee: dbaron → bzbarsky
(Assignee)

Comment 12

12 years ago
Fixed, by checking in that patch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

2 months ago
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.