Closed Bug 572296 Opened 10 years ago Closed 10 years ago

a couple of small context menu position fixes

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tnikkel, Assigned: tnikkel)

Details

Attachments

(2 files, 1 obsolete file)

I noticed a few things with the positioning of the context menu.
Positioning the context menu at the top left of the nearest widget looks especially weird with bug 130078 applied, where it opens above the chrome when the content has focus.
Attachment #451500 - Flags: review?(enndeakin)
The way the code is currently the only way checkLineHeight would be true is when the current focus is not a nsIDOMXULMultiSelectControlElement and is a nsIDOMXULMenuListElement. The else branch was added to the if in bug 387109. I think this is what the logic should be.
Attachment #451504 - Flags: review?(enndeakin)
Comment on attachment 451504 [details] [diff] [review]
Fix logic error in GetCurrentItemAndPositionForElement.

Actually, nsIDOMXULMenuListElement inherits from nsIDOMXULSelectControlElement so the QI isn't even needed, so let's just remove it instead.
Comment on attachment 451500 [details] [diff] [review]
Make the default position to open the context menu the top left of the document, not the top left of the nearest widget.

I don't understand the root view, root widget and offsets model enough to be able to review this.
Attachment #451500 - Flags: review?(enndeakin)
Comment on attachment 451500 [details] [diff] [review]
Make the default position to open the context menu the top left of the document, not the top left of the nearest widget.

This actually depends on the GetOffsetToWidget function I added in patches in bug 563878, which I didn't realize when I posted it. But that should be okay.
Attachment #451500 - Flags: review?(matspal)
(In reply to comment #3)
> Actually, nsIDOMXULMenuListElement inherits from nsIDOMXULSelectControlElement
> so the QI isn't even needed, so let's just remove it instead.

I'm sorry, I don't follow which QI isn't needed. Note that the code currently checks that the element does _not_ QI to nsIDOMXULMenuListElement.
Comment on attachment 451500 [details] [diff] [review]
Make the default position to open the context menu the top left of the document, not the top left of the nearest widget.

The comment suggests that the result of GetRootWidget can be null.  I think we need some null checks here.
(In reply to comment #7)
> The comment suggests that the result of GetRootWidget can be null.  I think we
> need some null checks here.

GetOffsetToWidget is actually null-safe. It is the only place in these context menu functions (AdjustContextMenuKeyEvent, PrepareToUseCaretPosition, and GetCurrentItemAndPositionForElement) where we use the widget. If you'd still prefer null checks I can do that.
> GetOffsetToWidget is actually null-safe.

Yes, but it calls GetViewFor(aWidget) that has a precondition that
aWidget is non-null, so I think we should definitely null-check the
widget.  I'd like a null-check on 'rootFrame' too.
Attachment #451504 - Flags: review?(enndeakin) → review+
Ok, null checked those things. I put the null checks for the widget in the functions PrepareToUseCaretPosition and GetCurrentItemAndPositionForElement into "Part 15. Fix GettOffsetTo call sites" in bug 563878 since that rewrites the stuff dealing with the widget anyways. (I didn't update the patches in the bug, but its available in my public patch queue for that bug.)
Attachment #451500 - Attachment is obsolete: true
Attachment #453243 - Flags: review?(matspal)
Attachment #451500 - Flags: review?(matspal)
Comment on attachment 453243 [details] [diff] [review]
Make the default position to open the context menu the top left of the document, not the top left of the nearest widget. v2

r=mats
Attachment #453243 - Flags: review?(matspal) → review+
Landed the default position patch
http://hg.mozilla.org/mozilla-central/rev/9ff9e0dc7644
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.