Bug 72104 - [GTK]Context menu doesn't display for selected node when menu is pressed.
Summary: [GTK]Context menu doesn't display for selected node when menu is pressed.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-11 02:01 PST by chandra shekar vallala
Modified: 2015-05-07 18:19 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.19 KB, patch)
2011-11-11 02:17 PST, chandra shekar vallala
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chandra shekar vallala 2011-11-11 02:01:45 PST
Open www.webkit.org with 'GtkLauncher'. Select some text press menu key. Context menu should open with correct options.
Comment 1 chandra shekar vallala 2011-11-11 02:17:29 PST
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.
Comment 2 Martin Robinson 2011-11-11 08:42:07 PST
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.
Comment 3 chandra shekar vallala 2011-11-11 17:31:19 PST
(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.
Comment 4 Martin Robinson 2011-11-12 08:39:28 PST
(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?
Comment 5 chandra shekar vallala 2011-11-15 06:02:41 PST
(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.
Comment 6 Martin Robinson 2011-11-15 08:46:30 PST
(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?
Comment 7 chandra shekar vallala 2011-11-18 05:03:56 PST
(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.
Comment 8 Martin Robinson 2011-11-18 06:23:25 PST
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?
Comment 9 chandra shekar vallala 2011-12-05 00:18:00 PST
(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?
Comment 10 Martin Robinson 2011-12-07 07:00:06 PST
(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?
Comment 11 chandra shekar vallala 2011-12-09 04:42:39 PST
(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 ?
Comment 12 Martin Robinson 2011-12-09 06:06:59 PST
(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.
Comment 13 Anders Carlsson 2014-02-05 10:51:19 PST
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.
Comment 14 Martin Robinson 2015-05-07 18:19:31 PDT
Going to close this since it addresses WebKit1.