Bug 72104 – [GTK]Context menu doesn't display for selected node when menu is pressed.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKitGTK

[chandra shekar vallala]

Open www.webkit.org with 'GtkLauncher'. Select some text press menu key. Context menu should open with correct options.

[chandra shekar vallala]

Created attachment 114654 [details]
patch

When menu key is pressed, it would be better if context menu displayed below the focused node. subtracted margin from maxY to display the contextmenu with correct options.

[Martin Robinson]

Is the issue that the context menu has the wrong options, is displayed in the wrong place, or some combination of these? It's unclear from your patch.

[chandra shekar vallala]

(In reply to comment #2)
> Is the issue that the context menu has the wrong options, is displayed in the wrong place, or some combination of these? It's unclear from your patch.

Its about displaying for wrong node.

[Martin Robinson]

(In reply to comment #3)

> Its about displaying for wrong node.

I don't really get why you are using the context menu margin in the place that determines the location of the mouse click. Ideally figuring out where the mouse event is and positioning the popup menu are two different logical actions.  Are you sure the issue isn't that because we are using y() now the click is off the left edge of the node?  In that case, wouldn't it be better to put the context menu in the middle of the node?

[chandra shekar vallala]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > Its about displaying for wrong node.
> 
> I don't really get why you are using the context menu margin in the place that determines the location of the mouse click. Ideally figuring out where the mouse event is and positioning the popup menu are two different logical actions.  Are you sure the issue isn't that because we are using y() now the click is off the left edge of the node?  In that case, wouldn't it be better to put the context menu in the middle of the node?

Actually this patch has two issues.
1. Placing the context menu below the focused node.
Since we are using the y(), mouse event is created with top-left corner position. We take the event GlobalX,Y values to position the menu which is causing the menu to open at top-left corner. I used maxY() to display menu at bottom of node since firefox and chrome does same on menu key event.

2. The location that we are calculating to create mouse event[x, maxY] is not giving the same selected/focused node in hitTestResult at given location point. This is causing the menu to create with different options. Hence I used margin to get the correct node(same node) in hitTest.

[Martin Robinson]

(In reply to comment #5)

> Actually this patch has two issues.
> 1. Placing the context menu below the focused node.
> Since we are using the y(), mouse event is created with top-left corner position. We take the event GlobalX,Y values to position the menu which is causing the menu to open at top-left corner. I used maxY() to display menu at bottom of node since firefox and chrome does same on menu key event.

It's probably best to move this particular change to another patch/bug, since it seems unrelated to the bug summary.

> 2. The location that we are calculating to create mouse event[x, maxY] is not giving the same selected/focused node in hitTestResult at given location point. This is causing the menu to create with different options. Hence I used margin to get the correct node(same node) in hitTest.

I think the use of gContextMenuMargin is inapparopriate here.  gContextMenuMargin is used to position the menu, not to find the element. Why not just find the centerpoint of the element and make the event there?

[chandra shekar vallala]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > Actually this patch has two issues.
> > 1. Placing the context menu below the focused node.
> > Since we are using the y(), mouse event is created with top-left corner position. We take the event GlobalX,Y values to position the menu which is causing the menu to open at top-left corner. I used maxY() to display menu at bottom of node since firefox and chrome does same on menu key event.
> 
> It's probably best to move this particular change to another patch/bug, since it seems unrelated to the bug summary.
> 
> > 2. The location that we are calculating to create mouse event[x, maxY] is not giving the same selected/focused node in hitTestResult at given location point. This is causing the menu to create with different options. Hence I used margin to get the correct node(same node) in hitTest.
> 
> I think the use of gContextMenuMargin is inapparopriate here.  gContextMenuMargin is used to position the menu, not to find the element. Why not just find the centerpoint of the element and make the event there?

Generating an event at center of selected content/focused node may place the menu at out of node region. Ex. A link rendered in across two lines.

[Martin Robinson]

Good point, but it seems that choosing maxY - 1 would  still have this issue? Why not just put the point at x +1, y + 1?

[chandra shekar vallala]

(In reply to comment #8)
> Good point, but it seems that choosing maxY - 1 would  still have this issue? Why not just put the point at x +1, y + 1?

I think I was not clear in my previous comment. If a link located in two lines i.e.. link located at upperRight and lowerLeft of the lines, getRect() will return  union of upper + lower bounds. Taking point(x+1, y+1) or center point may not hit the focus node.

If you don't mind, can you clarify the issue if we use maxY-1?

[Martin Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > Good point, but it seems that choosing maxY - 1 would  still have this issue? Why not just put the point at x +1, y + 1?
> 
> I think I was not clear in my previous comment. If a link located in two lines i.e.. link located at upperRight and lowerLeft of the lines, getRect() will return  union of upper + lower bounds. Taking point(x+1, y+1) or center point may not hit the focus node.
> 
> If you don't mind, can you clarify the issue if we use maxY-1?

If what you say is true, then wouldn't a node shaped like:

__________________________________________________
|                                                 |
-------------------            -------------------
                   |-----------|

Perhaps it's possible to get a list of rects. I think there's still an issue though, because of z-order issues. The more I look at this, the more I wonder if the right thing to do here is to override the node of the ContextMenuController manually. What do you think?

[chandra shekar vallala]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Good point, but it seems that choosing maxY - 1 would  still have this issue? Why not just put the point at x +1, y + 1?
> > 
> > I think I was not clear in my previous comment. If a link located in two lines i.e.. link located at upperRight and lowerLeft of the lines, getRect() will return  union of upper + lower bounds. Taking point(x+1, y+1) or center point may not hit the focus node.
> > 
> > If you don't mind, can you clarify the issue if we use maxY-1?
> 
> If what you say is true, then wouldn't a node shaped like:
> 
> __________________________________________________
> |                                                 |
> -------------------            -------------------
>                    |-----------|
> 
> Perhaps it's possible to get a list of rects. I think there's still an issue though, because of z-order issues. The more I look at this, the more I wonder if the right thing to do here is to override the node of the ContextMenuController manually. What do you think?

It would be better to take a best point from list of rects rather than overriding the node of ContextmenuController. I think it not possible to display menu for a node which is below a posZorderLayer with right-click. If it is not done by the mouse then why should with menu key ?

[Martin Robinson]

(In reply to comment #11)

> It would be better to take a best point from list of rects rather than overriding the node of ContextmenuController. I think it not possible to display menu for a node which is below a posZorderLayer with right-click. If it is not done by the mouse then why should with menu key ?

The way the menu key works, it should display a menu for whatever node has the selection, regardless of where it is in relation to other page content. This is important for accessibility.

[Anders Carlsson]

Comment on attachment 114654 [details]
patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Martin Robinson]

Going to close this since it addresses WebKit1.