175568 - Shift+F10 popup/contextual menu may appear off-screen (in wrong position - always down of item)

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Vaclav Dvorak]

When navigating with the keyboard, the Shift+F10 context menu of hyperlinks
always appears down of the link; when the link is near the bottom of the screen,
the menu than is partially or even entirely off-screen.

Actually, now that I wanted to make some screenshots, I found that the menu is
sometimes very far down from the link, so this can happen even if the link is
not near the bottom edge of the screen. This is seemingly random, though.

Note that when you activate the popup menu with the mouse, it correctly pops up
near the mouse cursor but always so that it is entirely visible - i.e., it is
shifted up if the mouse is near the bottom of the screen. I would expect the
same for keyboard activation.

I am seeing this on Mozilla 1.2b on Win98SE. I assume this is platform- and
OS-independent, but until confirmed, the platform and OS fields are set. I will
attach screenshots.

Comment 1

[Vaclav Dvorak]

Attachment: screenshot of situation where only the first menu item appears on-screen

Comment 2

[Vaclav Dvorak]

Attachment: screenshot of menu appearing nearly entirely on-screen, but very far from the link

Comment 3

[Vaclav Dvorak]

Confirming - I still see this in 1.3b.

Note that the context menu seems to appear down from the link, with the distance
proportional to the amount of vertical scroll of the window.

Comment 4

[Robert Praetorius]

I think the severity of this should be raised IF there are implications
for users using assistive technology (I don't whether there are or not,
but it seems like a reasonable possibility).

I'm seeing this too, with 

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624
Netscape/7.1 (ax)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20030916
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030902
Firebird/0.6.1+

with both Shift/F10 and the applications key (that mostly neglected
(except perhaps by me) key between the right menu key and the right
control key).

My observations match the (original poster's) hypothesis that the downward
displacement of top of the popup menu is (roughly) proportional to the # of 
screens the page has been scrolled down.

Comment 5

[Vaclav Dvorak]

Indeed, this does seem like an accessibility concern.

Comment 6

[Vaclav Dvorak]

Oh, and I just reproduced it on Linux with Mozilla/5.0 (X11; U; Linux i686;
en-US; rv:1.4) Gecko/20030624, so it's not Windows-specific.

Comment 7

[Oleg Sidletskiy]

*** Bug 192376 has been marked as a duplicate of this bug. ***

Comment 8

[[e-mail]]

*** Bug 243553 has been marked as a duplicate of this bug. ***

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

note that in bug 243553, the menu appeared near the upper left corner of the
screen (i.e. not down of the item, and not offscreen)

Comment 10

[Vaclav Dvorak]

Hmm, I now see the menu popping up in the upper left corner of the content area,
so the behaviour may have changed since the original report. This is with
Mozilla 20040420 on Linux. In fact, this might be sufficient (the menu is
visible), although it would be better if the menu appeared near the selected
object that it pertains to (but making sure it's entirely visible).

Comment 11

[Robert Praetorius]

I'm still seeing the original behavior (pop-up menu starts out just below and
slightly to the right of the link when the link is near the top of the page;
the menu moves farther and farther down the screen as I use links farther and
farther down the page, falling fairly quickly off the bottom of the screen)
using:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040421
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040511
Firefox/0.8.0+

Does anybody who's reading this have any difficulty reproducing this on MS
Windows OSses?  (I ask this because the fonts in my display scheme are not
the default and might influence the result)

Comment 12

[Aponar Kestrel]

Attachment: Test case showing horizontal scroll error

I've attached a test case showing that the observed behavior is not limited to
the vertical.

Steps to reproduce:

1. View the testcase in a window small enough to require horizontal scrolling.
2. Scroll right (not far enough to move the link off the screen, however).
3. Move focus to the link by tabbing.
4. Activate the context menu by Shift-F10 or the Windows-keyboard context-menu
key.

Expected behavior: The context menu appears over (or close to, or adjacent to)
the link.
Observed behavior: The context menu appears where the link would be if the page
were unscrolled (i.e., the context menu is not displaced leftwards to account
for scroll).

This has some relation to the patch for bug 81723. I _think_ there needs to be
a call to vm->GetScrollPosition() in
nsEventListenerManager::GetCoordinatesFor() to adjust this -- but that may not
work. (I can't currently test it, but I'm bringing this box up to spec for
build purposes right now.)

However, if the element which has focus is outside the current scroll-view, the
scroll will currently (and correctly) be adjusted by the minimum necessary to
bring the focused element into the view. (This can be demonstrated by amending
#2 in the above testcase to scroll the link entirely offscreen.) Unfortunately
I don't know where that occurs; I don't see anything that obviously affects the
scroll, and I suspect it may be much, much later -- possibly in the event
handler itself, in XULPopupListenerImpl. This could pose a problem, in that the
result of vm->GetScrollPosition() will be invalid after this adjustment; we
can't call it until after this is done.

(Come to think of it, wouldn't it make more sense to put GetCoordinatesFor() in
the actual event handler anyway? An NS_CONTEXTMENU_KEY event probably needs to
be handled differently from an NS_CONTEXTMENU event.)

Is backing out the 'fix' for bug 81723 a viable temporary solution here? At
least then the context menu would always be visible, if incorrectly placed.

Comment 13

[Felix Miata]

Does anyone still see this in current trunk or FF?

Comment 14

[Robert Praetorius]

> Does anyone still see this in current trunk or FF?

How current?  I'm definitely seeing it on Firefox 0.9.3 (
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
) and Mozilla 1.8a2 (
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040714
) 

I can try the nightlies if you think that would make a difference.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

*** Bug 257119 has been marked as a duplicate of this bug. ***

Comment 16

[Aaron Leventhal]

Why does Dave Hyatt have any Mozilla bugs assigned to him anymore at all? These
bugs need to find their way to someone who might look at thim. Dave's busy with
Safari.

Comment 17

[Aaron Leventhal]

The scroll positions of the current document need to be subtracted to get the
screen coordinate. In fact, it can even be more complicated than that, because
there can be multiply nested scrolled documents.

Comment 18

[Aaron Leventhal]

Attachment: Use known algorithm to get screen coodinates (stolen from nsAccessible::GetScreenOrigin())

I think GetScreenCoordinates should actually be an nsIFrame method, since it
needs to be used in other places in our code (accessibility, Inspector, ... not
sure what else.) What do the reviewers think?

Comment 19

[Aaron Leventhal]

Attachment: Use known algorithm to get screen coodinates (stolen from nsAccessible::GetScreenOrigin())

I think GetScreenCoordinates should actually be an nsIFrame method, since it
needs to be used in other places in our code (accessibility, Inspector, ... not
sure what else.) What do the reviewers think?

Comment 20

[Boris Zbarsky [:bzbarsky]]

From recent email I sent to roc:

  Do we want some function for mapping frames to screen coords too?  I see the
  following functions doing this, in various (correct and not) ways:

  nsMenuPopupFrame::GetScreenPosition
  nsSVGSVGElement::GetScreenPosition
  nsBoxObject::GetScreenRect
  nsAccessibleText::GetCharacterExtents (this has the COORD_TYPE_WINDOW
    weirdness, though).
  Some Inspector code that's unused and removed in my tree... 

So yes, I do think we need such a method.   ;)

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 164186 [details] [diff] [review]
Use known algorithm to get screen coodinates (stolen from nsAccessible::GetScreenOrigin())

This works fine except when the element is scrolled completely outside
the viewport (the context menu can popup outside the window).
Maybe we should scroll the element into view for this case?

Comment 22

[Aaron Leventhal]

(In reply to comment #21)
> (From update of attachment 164186 [details] [diff] [review])
> This works fine except when the element is scrolled completely outside
> the viewport (the context menu can popup outside the window).

As far as I know the element should always be in the viewport, because we always
scroll to the focus if it isn't visible.

If we want to change the context menu so that it always displays completely
within the top level window, we should do that in another bug. That affects
context menus opened from the mouse as well. We do keep it on screen, at least.

Other than that, I'm not sure what you're suggesting we scroll to that we don't
already scroll to.

Comment 23

[Mats Palmgren (inactive)]

What I did was:
1. Put focus on a link
2. Grab the page scrollbar thumb and scroll the link outside the viewport
3. Shift+F10

Comment 24

[Aaron Leventhal]

(In reply to comment #21)
> Maybe we should scroll the element into view for this case?
Okay, I see it now that I scroll out of the way first. The element does get
scrolled, but the menu appears relative to the old position.

Comment 25

[Aaron Leventhal]

Attachment: Ensure the element is onscreen before getting screen coordinates

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 165213 [details] [diff] [review]
Ensure the element is onscreen before getting screen coordinates

This works nicely.
Please indent one more space on these comments before checkin:

>+   // Get the view's origin within its widget
>+    nsIWidget *widget = view->GetNearestWidget(&viewOrigin);
>+
>+   // Add the widget's screen origin to the frame and view offsets
>+    nsRect newRect;

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165213 [details] [diff] [review]
Ensure the element is onscreen before getting screen coordinates

First, this is in layout code.	So no need for GetViewExternal().

Second, the GetViewExternal/GetOffsetFromView code can be replaced by a single
call to nsIFrame::GetClosestView with a non-null aPoint passed in.

Make those changes please?  And file a bug on me to factor out the "get the
screen position of this frame" code into a function on nsIFrame?

Comment 28

[Aaron Leventhal]

Filed bug 268576 on consolodating screen coordinate getters onto a single
nsIFrame method.

Comment 29

[Aaron Leventhal]

Attachment: Use GetClosestView() instead of GetViewExternal/GetOffsetFromView

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165275 [details] [diff] [review]
Use GetClosestView() instead of GetViewExternal/GetOffsetFromView

Hmm.. I just realized I have some more questions...  First of all, what
happened to the line-height thing commented on?  Don't we need to keep that? 
Otherwise the context menu for a link will cover up the link, no?

Second, why is this being done in screen coordinates?  Is it because the
existing aEvent->refPoint is in screen coordinates?  What coordinate systems
should targetPoint and refPoint end up in, anyway?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165275 [details] [diff] [review]
Use GetClosestView() instead of GetViewExternal/GetOffsetFromView

OK, I tried the patch.	If I move my window around, the context menu will come
up in the wrong place (off by as much as 200px vertically).  So we're doing
something pretty wrong with the coordinates...

Comment 32

[Aaron Leventhal]

Attachment: Fix coord system, improve lineheight stuff and put back in

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165316 [details] [diff] [review]
Fix coord system, improve lineheight stuff and put back in

So why are coordinates to nearest parent widget the relevant ones here?  Is
that what the event coordinates are in?  

In particular, was this tested with fixed-position elements (which have their
own widgets)?  What about subframes?  Nested subframes?  Fixed-positioned
widgets inside subframes?

Comment 34

[Aaron Leventhal]

Attachment: Uses root view coordinate system. Also takes care of tree special case where the focus is drawn but not associated with a specific tree item node/frame

Comment 35

[Aaron Leventhal]

Comment on attachment 165420 [details] [diff] [review]
Uses root view coordinate system. Also takes care of tree special case where the focus is drawn but not associated with a specific tree item node/frame

This fixes the www.w3.org/style case.
Also, while I tested I noticed a problem with tree views that this patch fixes,
although perhaps the code should be factored out. Or maybe that just makes
things even more complex. Your call.

Comment 36

[Aaron Leventhal]

Boris, I tried to track down why this coordinate system is working so well. I
believe that the popup frame is created it uses these coordinates relative to
the presshell, and there is one presshell per viewmanager (we're relative to the
root from from that). I'm having a hard time tracking down the exact place it
happens. Popup creation and display is a lot more complex than I would have thought.

Comment 37

[Aaron Leventhal]

*** Bug 271531 has been marked as a duplicate of this bug. ***

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165420 [details] [diff] [review]
Uses root view coordinate system. Also takes care of tree special case where the focus is drawn but not associated with a specific tree item node/frame

>Index: content/events/src/nsEventListenerManager.cpp
>+// Get screen coordinates for element, first ensuring the element is onscreen

Please fix this comment to reflect what the function is actually doing?

>+    nsPoint frameOrigin(0, 0), viewOrigin(0, 0);

Why do you need both?  Can't you just do:

  frameOrigin += view->GetOffsetTo(rootView);

in the place where you currently set viewOrigin?

>+              aPresShell->GetPrimaryFrameFor(colContent, &frame);
>+              if (frame) {
>+                frameOrigin.y += frame->GetSize().height;

Why modify frameOrigin.y here instead of setting extra as elsewhere, for
consistency?

>+    aTargetPt.x = NSTwipsToIntPixels(frameOrigin.x + viewOrigin.x + extra, t2p) + extraPixelsX;

Adding extra to the x coord is wrong.  It's purely a y-offset measure (derived
from frame heights, line heights, etc). I realize the existing code was doing
it, but I don't think we should continue that...

>Index: layout/xul/base/src/tree/public/nsITreeBoxObject.idl

Could you run this api change by jan or someone else who's familiar with trees
please?  Setting r= request accordingly...

sr=bzbarsky with those issues fixed.

Comment 39

[[e-mail]]

Comment on attachment 165420 [details] [diff] [review]
Uses root view coordinate system. Also takes care of tree special case where the focus is drawn but not associated with a specific tree item node/frame

>+        multiSelect->GetCurrentIndex(&currentIndex);
>+        treeBox->GetCoordsForCellItem(currentIndex, nsnull, NS_LITERAL_CSTRING(""),
>+                                      &extraPixelsX, &extraPixelsY, &width, &height);
GetCurrentIndex may return -1 or (even occasionally fail) when the tree is
empty but GetCoordsForCellItem should never be passed -1. Also I believe that
if you use that call you should be using an element of
NS_LITERAL_CSTRING("cell"). But I think that is overkill when you could use
GetRowHeight() and GetFirstVisibleRow(). Also worth considering is
EnsureRowIsVisible() - Windows 98 Explorer will happily pop the context menu up
off screen if you scroll the selected item out of view, we wouldn't want to
copy Microsoft's mistake now, would we? To be fair, Windows 2000 at least clips
the context menu to the screen. Finally, is GetCurrentIndex really what you
want? Other possibilities include:
* The current index, if it is selected and visible
* Any visible selected index
* Scroll the current index into view, if it is selected
* The current index, if it is visible
* Scroll the current index into view, if there is no selection
I'm not sure which if any of these behaviours would be useful.

Comment 40

[Aaron Leventhal]

> >+              aPresShell->GetPrimaryFrameFor(colContent, &frame);
> >+              if (frame) {
> >+                frameOrigin.y += frame->GetSize().height;
> 
> Why modify frameOrigin.y here instead of setting extra as elsewhere, for
> consistency?
Because extra is added to both x and y. I just want to alter the y coord.

> >+    aTargetPt.x = NSTwipsToIntPixels(frameOrigin.x + viewOrigin.x + extra,
t2p) + extraPixelsX;
> 
> Adding extra to the x coord is wrong.  It's purely a y-offset measure (derived
> from frame heights, line heights, etc). I realize the existing code was doing
> it, but I don't think we should continue that...
It looks better when the context menu is offset to the right. Offsetting it
horizontally by the same amount that it is offset vertically just looks good. I
think we should keep it that way.

Comment 41

[Aaron Leventhal]

Checking in content/events/src/nsEventListenerManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventListenerManager.cpp,v  <-- 
nsEventListenerManager.cpp
new revision: 1.187; previous revision: 1.186
done

Comment 42

[Boris Zbarsky [:bzbarsky]]

> It looks better when the context menu is offset to the right. Offsetting it
> horizontally by the same amount that it is offset vertically just looks good.

I disagree.  If I use a 30-px font size, would you actually want to offset 30px
to the right?  If the frame is 2px tall, would you really want to offset 2px
down and only 2px to the right?  I'd say offset to the right should be some
fixed number if we want to do it...

Comment 43

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

It looks like this broke --disable-xul and/or --disable-accessibility.  See
myotonic tinderbox on SeaMonkey-Ports.

Comment 44

[Boris Zbarsky [:bzbarsky]]

There's a good chance the tree code in this patch needs to be ifdefed to only
run when XUL is enabled....

Comment 45

[Darin Fisher]

Looks like this also broke the Minimo build (see neptune on Seamonkey-Ports),
presumably due to --disable-xul being busted.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Yep, trees are simply not built when --disable-xul is used.  I checked in
MOZ_XUL ifdefs around the relevant includes and code.

Comment 47

[[e-mail]]

Thanks for the tree logic fixes, but there's one final nit; on an empty tree the
context menu now appears to try to pop up just below the entire tree :-/

Comment 48

[Aaron Leventhal]

*** Bug 267411 has been marked as a duplicate of this bug. ***

Comment 49

[[e-mail]]

I'm seeing an intermittent problem in the sidebar - sometimes the context menus
are offset; I'm wondering whether this patch is the culprit.

Comment 50

[Mats Palmgren (inactive)]

*** Bug 271498 has been marked as a duplicate of this bug. ***

Comment 51

[Anatolij Kupriyanov]

Attachment: Screen-shot with wrong positioned menu

The problem is still here. It happens when context menu is opening by keyboard
(context menu button or Shift-F10)

Comment 52

[Anatolij Kupriyanov]

The previous screen-shot are made for Firefox v1.0.1

Comment 53

[Aaron Leventhal]

Don't bother doing any testing work with 1.0.x. That's based off a branch
created one and a half years ago.

Testers need to use nightly builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

You can install it in a separate Firefox-test directory and keep your 1.0.1 for
real use if you want.

Comment 54

[:Gavin Sharp [email: [e-mail]]]

*** Bug 287657 has been marked as a duplicate of this bug. ***

Comment 55

[:Gavin Sharp [email: [e-mail]]]

*** Bug 302817 has been marked as a duplicate of this bug. ***

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Still happen with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008030404 Minefield/3.0b5pre ID:2008030404.

Now that the context menu shortcut is working under OS X this will be more visible in the future.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please forget my comment 56. It's not true. It works like a charm.

Comment 59

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381