Context menus on view elements should be positioned above the finger when

Context menus on view elements should be positioned above the finger when
invoked using long press.

BUG=142015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157005

[sky, 2012-09-13 16:15:58]

Is there a reason we can't do this automatically? RunMenuAt takes the parent
widget.

[varunjain, 2012-09-13 16:25:42]

On 2012/09/13 16:15:58, sky wrote:
> Is there a reason we can't do this automatically? RunMenuAt takes the parent
> widget.

That was indeed my first solution, but is one instance in which the caller has
explicitly set context menu position to TOPRIGHT. I am not sure if RunMenuAt
should override the position sent to it by the caller. IMO, we should unify the
positioning of context menu, so that no matter what position the caller
supplies, in RunMenuAt, we will have something like:

if (types & MenuRunner::CONTEXT_MENU) {
  anchor = parent->GetCurrentEvent()->IsGestureEvent() ? BOTTOMCENTER : TOPLEFT;
}

The instance where this would change current behavior is:
bookmark_editor_view.cc
Thats the only one I could find and I am not sure why is the context menu
position set to TOPRIGHT there.

[sky, 2012-09-13 17:24:11]

On Thu, Sep 13, 2012 at 9:25 AM,  <varunjain@chromium.org> wrote:
> On 2012/09/13 16:15:58, sky wrote:
>>
>> Is there a reason we can't do this automatically? RunMenuAt takes the
>> parent
>> widget.
>
>
> That was indeed my first solution, but is one instance in which the caller
> has
> explicitly set context menu position to TOPRIGHT. I am not sure if RunMenuAt
> should override the position sent to it by the caller. IMO, we should unify
> the
> positioning of context menu, so that no matter what position the caller
> supplies, in RunMenuAt, we will have something like:
>
> if (types & MenuRunner::CONTEXT_MENU) {
>   anchor = parent->GetCurrentEvent()->IsGestureEvent() ? BOTTOMCENTER :
> TOPLEFT;
> }

I like this, SGTM
>
> The instance where this would change current behavior is:
> bookmark_editor_view.cc
> Thats the only one I could find and I am not sure why is the context menu
> position set to TOPRIGHT there.

I think that is wrong and should be TOPLEFT.

  -Scott

> https://chromiumcodereview.appspot.com/10916180/

[varunjain, 2012-09-14 16:41:44]

Done.. PTAL.


On 2012/09/13 17:24:11, sky wrote:
> On Thu, Sep 13, 2012 at 9:25 AM,  <mailto:varunjain@chromium.org> wrote:
> > On 2012/09/13 16:15:58, sky wrote:
> >>
> >> Is there a reason we can't do this automatically? RunMenuAt takes the
> >> parent
> >> widget.
> >
> >
> > That was indeed my first solution, but is one instance in which the caller
> > has
> > explicitly set context menu position to TOPRIGHT. I am not sure if RunMenuAt
> > should override the position sent to it by the caller. IMO, we should unify
> > the
> > positioning of context menu, so that no matter what position the caller
> > supplies, in RunMenuAt, we will have something like:
> >
> > if (types & MenuRunner::CONTEXT_MENU) {
> >   anchor = parent->GetCurrentEvent()->IsGestureEvent() ? BOTTOMCENTER :
> > TOPLEFT;
> > }
> 
> I like this, SGTM
> >
> > The instance where this would change current behavior is:
> > bookmark_editor_view.cc
> > Thats the only one I could find and I am not sure why is the context menu
> > position set to TOPRIGHT there.
> 
> I think that is wrong and should be TOPLEFT.
> 
>   -Scott
> 
> > https://chromiumcodereview.appspot.com/10916180/

[sky, 2012-09-14 17:19:57]

https://chromiumcodereview.appspot.com/10916180/diff/9001/ui/views/controls/m...
File ui/views/controls/menu/menu_runner.cc (right):

https://chromiumcodereview.appspot.com/10916180/diff/9001/ui/views/controls/m...
ui/views/controls/menu/menu_runner.cc:298: if (anchor ==
MenuItemView::BOTTOMCENTER)
Can this be moved to UpdateInitialLocation?

[varunjain, 2012-09-14 18:46:06]

https://codereview.chromium.org/10916180/diff/9001/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_runner.cc (right):

https://codereview.chromium.org/10916180/diff/9001/ui/views/controls/menu/men...
ui/views/controls/menu/menu_runner.cc:298: if (anchor ==
MenuItemView::BOTTOMCENTER)
On 2012/09/14 17:19:57, sky wrote:
> Can this be moved to UpdateInitialLocation?

Done.

[sky, 2012-09-14 19:45:43]

LGTM

[commit-bot: I haz the power, 2012-09-14 19:49:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10916180/2004

[commit-bot: I haz the power, 2012-09-15 00:52:59]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-09-15 01:01:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10916180/2004

[commit-bot: I haz the power, 2012-09-15 01:52:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 02:26:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10916180/2004

[commit-bot: I haz the power, 2012-09-15 03:22:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 03:55:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10916180/2004

[commit-bot: I haz the power, 2012-09-15 04:53:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.