1598488 - Fix GetWidget() in nsMenuPopupFrame

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Tim Nguyen :ntim]

GetWidget() seems to return the root window widget for <menupopup> and the popup window widget itself for <panel>.

GetView()->GetWidget() seems to return the popup window widget itself for both the <panel> and the <menupopup>.

I was caught by this in bug 1597120 and ended up using GetView()->GetWidget().

I don't know if that's the intended behavior, but if it isn't, this is probably related to GetRootViewForPopup returning the wrong view.

:tnikkel, do you know if this behavior is intended ?

Comment 1

[Timothy Nikkel (:tnikkel)]

Thanks for reporting this!

That seems just wrong. Looking at the callers of GetWidget they are seem to want to widget of the popup and not the root widget.

Do you happen to know why GetRootViewForPopup is returning the wrong view? Is it because we skip over the widget for the popup because the window type is not eWindowType_popup? Or because the view we start with from |nsView* view = aStartFrame->GetClosestView()| is a view outside of the popup because the nsMenuPopupFrame doesn't have a view yet?

Comment 2

[Tim Nguyen :ntim]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

Thanks for reporting this!

That seems just wrong. Looking at the callers of GetWidget they are seem to want to widget of the popup and not the root widget.

Do you happen to know why GetRootViewForPopup is returning the wrong view? Is it because we skip over the widget for the popup because the window type is not eWindowType_popup? Or because the view we start with from |nsView* view = aStartFrame->GetClosestView()| is a view outside of the popup because the nsMenuPopupFrame doesn't have a view yet?

Tbh, I haven't really looked into it deeper than this (I don't have a full build locally, all of my debugging involved sending my commits to try, downloading the artifacts to test them, and reading the code).

But from reading the code, I don't think it can be the former, since we init the widget with eWindowType_popup: https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/layout/xul/nsMenuPopupFrame.cpp#270

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Tim Nguyen :ntim from comment #2)

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

Thanks for reporting this!

That seems just wrong. Looking at the callers of GetWidget they are seem to want to widget of the popup and not the root widget.

Do you happen to know why GetRootViewForPopup is returning the wrong view? Is it because we skip over the widget for the popup because the window type is not eWindowType_popup? Or because the view we start with from |nsView* view = aStartFrame->GetClosestView()| is a view outside of the popup because the nsMenuPopupFrame doesn't have a view yet?

Tbh, I haven't really looked into it deeper than this (I don't have a full build locally, all of my debugging involved sending my commits to try, downloading the artifacts to test them, and reading the code).

But from reading the code, I don't think it can be the former, since we init the widget with eWindowType_popup: https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/layout/xul/nsMenuPopupFrame.cpp#270

Hmm, well I don't think it can be the other one actually, because you switched to GetView()->GetWidget(), so we must have a view on the menu popup frame in this case. So we don't understand what's going on here? It would be good to know what's going on.

Comment 4

[Tim Nguyen :ntim]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

Hmm, well I don't think it can be the other one actually, because you switched to GetView()->GetWidget(), so we must have a view on the menu popup frame in this case. So we don't understand what's going on here? It would be good to know what's going on.

Yeah, I switched to GetView()->GetWidget() because GetWidget() wasn't correct, GetView()->GetWidget() returns the right thing though.

Comment 5

[Timothy Nikkel (:tnikkel)]

How does one reproduce this to figure out whats happening?

Comment 6

[Timothy Nikkel (:tnikkel)]

Ah, nevermind, I figured it out. I was missing the obvious. If our view doesn't have a widget we proceed to the parent view, it'll have a widget of the wrong type, but it won't have a parent view, and so we return that view. I'll attach a patch to get rid of this complication, it's not useful.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1598488. Fix nsMenuPopupFrame::GetWidget. r=emilio

It can return the root widget if the menu popup frame doesn't have a widget. None of the callers want this.

Comment 8

[Tim Nguyen :ntim]

(Note that I triggered landing for bug 1597120 a few hours ago, and I see you just triggered landing here as well so I'll make sure to land a follow up here to remove my workaround)

Comment 9

[Timothy Nikkel (:tnikkel)]

The new code is equivalent to your work around and I think the same pattern is used elsewhere in the file so it's okay to leave it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c038eb7d1cb1
Fix nsMenuPopupFrame::GetWidget. r=emilio

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c038eb7d1cb1