Fix GetWidget() in nsMenuPopupFrame
Categories
(Core :: Layout, task)
Tracking
()
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 ?
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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?
Reporter | ||
Comment 2•5 years ago
•
|
||
(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
Assignee | ||
Comment 3•5 years ago
|
||
(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.
Reporter | ||
Comment 4•5 years ago
|
||
(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.
Assignee | ||
Comment 5•5 years ago
|
||
How does one reproduce this to figure out whats happening?
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
It can return the root widget if the menu popup frame doesn't have a widget. None of the callers want this.
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 8•5 years ago
|
||
(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)
Assignee | ||
Comment 9•5 years ago
|
||
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.
Reporter | ||
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c038eb7d1cb1 Fix nsMenuPopupFrame::GetWidget. r=emilio
Comment 11•5 years ago
|
||
bugherder |
Description
•