Closed Bug 1598488 Opened 10 months ago Closed 10 months ago

Fix GetWidget() in nsMenuPopupFrame

Categories

(Core :: Layout, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: ntim, Assigned: tnikkel)

References

Details

Attachments

(1 file)

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 ?

Flags: needinfo?(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?

Flags: needinfo?(tnikkel)

(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

Flags: needinfo?(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.

Flags: needinfo?(tnikkel)

(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.

Flags: needinfo?(tnikkel)

How does one reproduce this to figure out whats happening?

Flags: needinfo?(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.

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

Assignee: nobody → tnikkel
Attachment #9110767 - Attachment description: Bug 1598488. Fix nsMenoPopupFrame::GetWidget. r?emilio → Bug 1598488. Fix nsMenuPopupFrame::GetWidget. r=emilio

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

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.

Summary: Investigate GetWidget() value in nsMenuPopupFrame → Fix GetWidget() in nsMenuPopupFrame
Blocks: 1597120
See Also: 1597120
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c038eb7d1cb1
Fix nsMenuPopupFrame::GetWidget. r=emilio
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.