Remove unnecessary menutobedisplayed

RESOLVED WONTFIX

Status

()

Core
XUL
RESOLVED WONTFIX
14 years ago
10 years ago

People

(Reporter: Aaron Leventhal, Assigned: Joshua Welderson)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
Follow up from bug 282438:

In nsPopupSetFame.cpp, pass mIsOpen to SyncViewWithFrame to avoid having touse
the menutobedisplayed attribute.

While we're in there it might be a good idea look at the possibility of removing
mShouldRollup and EnableRollup where they aren't used or necessary.
(Assignee)

Comment 1

13 years ago
Created attachment 200307 [details] [diff] [review]
Fix

Taking and requesting review.
Assignee: neil.parkwaycc.co.uk → pythonesque+bugzilla
Status: NEW → ASSIGNED
Attachment #200307 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #200307 - Flags: review?(bryner)

Comment 2

13 years ago
Comment on attachment 200307 [details] [diff] [review]
Fix

>   nsresult SyncViewWithFrame(nsPresContext* aPresContext, const nsString& aPopupAnchor,
>                              const nsString& aPopupAlign,
>-                             nsIFrame* aFrame, PRInt32 aXPos, PRInt32 aYPos);
>+                             nsIFrame* aFrame, PRInt32 aXPos, PRInt32 aYPos,
>+                             PRBool shouldDisplay);
Might want to make shouldDisplay default to PR_FALSE (see below).

>-    // When we sync the popup view with the frame, we'll show the popup if |menutobedisplayed|
>+    // When we sync the popup view with the frame, we'll show the popup if mIsOpen
>     // is set by setting the |menuactive| attribute. This used to trip css into showing the menu
>     // but now we do it ourselves. 
>-    if (aActivateFlag)
>-      // XXXben hook in |width| and |height| usage here? 
>-      aEntry->mPopupContent->SetAttr(kNameSpaceID_None, nsXULAtoms::menutobedisplayed, NS_LITERAL_STRING("true"), PR_TRUE);
>-    else {
>+    if (!aActivateFlag) {
>       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None, nsXULAtoms::menuactive, PR_TRUE);
>-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None, nsXULAtoms::menutobedisplayed, PR_TRUE);
What I don't understand is why we don't just set/unset menuactive here
directly, which is why I'm chickening out of the review.

>-      menuPopup->SyncViewWithFrame(mPresContext, popupAnchor, popupAlign, this, -1, -1);
>+      menuPopup->SyncViewWithFrame(mPresContext, popupAnchor, popupAlign, this, -1, -1, IsOpen());
>-    menuPopup->SyncViewWithFrame(presContext, popupAnchor, popupAlign, this, -1, -1);
>+    menuPopup->SyncViewWithFrame(presContext, popupAnchor, popupAlign, this, -1, -1, IsOpen());
As far as I can tell, the menu will already be active at this point, so it
makes no difference what you pass to the shouldDisplay argument.
Attachment #200307 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 3

13 years ago
I have a new patch with some other stuff fixed (mShouldRollup has been removed,
although EnableRollUp doesn't seem to be unnecessary), as well as some of the
things you said.  But I'd like clarification on this point:

(In reply to comment #2)
> >+    if (!aActivateFlag) {
> >       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menuactive, PR_TRUE);
> >-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menutobedisplayed, PR_TRUE);
> What I don't understand is why we don't just set/unset menuactive here
> directly, which is why I'm chickening out of the review.
Do you mean you don't understand why menuactive isn't being set on the aEntry,
or why it's not being set regardless of aActivateFlag, or something else?  I'm
not terribly familiar this part of the codebase, but I think all the present
code expects menuactive to be set on the menupopup, so I presume that's not what
you meant...

Comment 4

13 years ago
(In reply to comment #3)
>>>+    if (!aActivateFlag) {
>>>       aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menuactive, PR_TRUE);
>>>-      aEntry->mPopupContent->UnsetAttr(kNameSpaceID_None,
nsXULAtoms::menutobedisplayed, PR_TRUE);
>>What I don't understand is why we don't just set/unset menuactive here
>>directly, which is why I'm chickening out of the review.
>Do you mean you don't understand why menuactive isn't being set on the aEntry,
>or why it's not being set regardless of aActivateFlag, or something else?
I mean why does the code wait until the sync before setting the menuactive.
(Assignee)

Comment 5

13 years ago
Created attachment 200381 [details] [diff] [review]
Fixes + mShouldRollUp

Fix addressing the most of Roc's comments and removing mShouldRollup. 
Hopefully Brian can clear up Neil's point.  I couldn't find any unnecessary
instances of EnableRollup, though, so that's not included in the patch.
Attachment #200307 - Attachment is obsolete: true
Attachment #200381 - Flags: review?(bryner)
(Assignee)

Updated

13 years ago
Attachment #200307 - Flags: review?(bryner)
(Assignee)

Comment 6

13 years ago
Er, Neil.  I'm bad with names.
(Assignee)

Comment 7

13 years ago
Brian, would you please take a look or so at this?
Comment on attachment 200381 [details] [diff] [review]
Fixes + mShouldRollUp

I don't know this code well enough to review, and I don't think I'll have time to come up to speed on it.
Attachment #200381 - Flags: review?(bryner)

Updated

10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.widgets

Comment 9

10 years ago
Popup changed make this bug obsolete
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.