349921 - [FIX]XUL popups in root popupset have wrong parent

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion in bug 349585.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w (review only)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks nice, just one question. Why aren't the popups available from the popupSetFrame via GetFirstChild/GetAdditionalChildListName? Wouldn't that make this special casing unnecessary?

+      // Return here, because the postcondition for this function actually
+      // fails for this case, since the popups are not in a "real" frame list
+      // in the popup set.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I could try to change that.  But nsPopupSetFrame doesn't store the popups in a frame list; it stores a list of nsPopupFrameList structs.  I could chain the nsIFrames in those together, but I'd need to figure out what to do in the "entry already exists" case in nsPopupSetFrame::AddPopupFrame, would need to link aPopup to the previous entry there (so would need to modify GetEntry()), and so forth.  since it was code I'm not that familiar with, I didn't really want to try mucking with it right now.  If you'd like a followup bug on doing that, I would be happy to file one.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 235128 [details] [diff] [review]
Proposed fix

fair enough

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Bustage fix for NO XUL configuration

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236018 [details] [diff] [review]
Bustage fix for NO XUL configuration

>-    mCreatorIsBlock(PR_FALSE),
>-    mRootBox(nsIRootBox::GetRootBox(aPresShell))
>+#ifdef MOZ_XUL    
>+    mRootBox(nsIRootBox::GetRootBox(aPresShell)),
>+#endif
>+    mCreatorIsBlock(PR_FALSE)

This is bad -- changing the order here triggers warnings.  Please put the ',' inside the #ifdef instead.

>+#ifdef MOZ_XUL
>     if (isPopup) {
>       NS_ASSERTION(aState.mRootBox && aState.mRootBox->GetPopupSetFrame(),
>                    "How did we get here?");
>       geometricParent = aState.mRootBox->GetPopupSetFrame();
>     } else {
>       geometricParent = aParentFrame;
>     }
>+#else
>+      geometricParent = aParentFrame;
>+#endif

I think I'd prefer to have ifdefs around the "if() {} else" part and have the "{ ... }" part outside the ifdefs.  That should give the right behavior without code duplication.

Sorry I wasn't more careful about this stuff.  :(

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Bustage rev 2

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Forgot remove comma :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk earlier today.  I'll file a followup tomorrow morning.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236032 [details] [diff] [review]
Forgot remove comma :(

>+    } else
>+#else
>       geometricParent = aParentFrame;
>+#endif

No, this is fragile.  Please look again at what I suggested you do here -- I want those curly braces.

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: It is better? ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236066 [details] [diff] [review]
It is better? ;)

I want something like:

#ifdef MOZ_XUL
  if (isPopup) {
...
  }
  else
#else 
  {
    geometricParent = aParentFrame;
  }

Comment 14

[Boris Zbarsky [:bzbarsky]]

Except with an #endif.  I'd do it myself, but I don't have a tree with --disable-xul to test in and can't quickly have one.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Build bustage fix I checked in

On second thought, it's not like it can get worse than it is now... I checked this in; let me know if there are remaining issues.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Filed bug 350740 as a followup per comment 3 and comment 4.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Minor correction to the above

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since you removed the isReplaced = PR_TRUE, as part of merging the reflow branch to a new branch, I'm adding an IsFrameOfType override in nsMenuPopupFrame.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Er, the isReplaced part shouldn't have gone away.  That was a merging mistake (the patch was initially developed against reflow branch, where that line is gone).  I'll reinstate it on trunk.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Restore the isReplaced line

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I undid my merging changes.)