Issue 2876203003: Make shelf item can be dragged when context menu is opened.

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-12 19:12:22 UTC) #1

minch1

minch@chromium.org changed reviewers: + xiyuan@chromium.org

3 years, 7 months ago (2017-05-12 19:12:22 UTC) #2

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-12 19:13:53 UTC) #3

xiyuan

https://codereview.chromium.org/2876203003/diff/1/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2876203003/diff/1/ui/views/controls/menu/menu_controller.cc#newcode828 ui/views/controls/menu/menu_controller.cc:828: } nit: revist to something like if (part.submenu) { ...

3 years, 7 months ago (2017-05-12 20:24:57 UTC) #4

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-14 00:55:51 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/40001

3 years, 7 months ago (2017-05-14 00:56:01 UTC) #6

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-14 00:56:29 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-14 01:09:22 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/452987)

3 years, 7 months ago (2017-05-14 01:09:22 UTC) #9

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-15 17:03:43 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/40001

3 years, 7 months ago (2017-05-15 17:04:56 UTC) #11

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-15 17:25:29 UTC) #12

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/418916) mac_chromium_rel_ng on ...

3 years, 7 months ago (2017-05-15 17:25:30 UTC) #13

xiyuan

We probably can put added code in #if defined(OS_CHROMEOS) since they are chromeos specific and ...

3 years, 7 months ago (2017-05-15 19:20:53 UTC) #15

minch1

xiyuan@, can you help check the comments? Thanks. https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc#newcode1663 ash/shelf/shelf_view.cc:1663: if ...

3 years, 7 months ago (2017-05-15 22:16:35 UTC) #16

xiyuan@, can you help check the comments? Thanks.

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1663: if (index == 0 || !item) {
On 2017/05/15 19:20:52, xiyuan wrote:
> Why we need to include |index| == 0? Can you document it?

This is for the launcher icon (the first item in the shelf). The gesture events
of this item will not go through ShelfButton's OnGestureEvent. This will produce
a flash behavior if long press of the item.
I think it is fine to make the context menu of this item the same as if touching
empty space on the shelf. Since the contents of the context menu in these two
situations are the same.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:676: static bool do_cancel_;
On 2017/05/15 19:20:52, xiyuan wrote:
> Why this needs to be static? We already have a static getter for
MenuController.
> Can we make it a regular member and use the getter to access it?

But in shelf_view.cc where where we want to set this variable. We have no
MenuController instance yet. |active_instance_| returned by GetActiveInstance()
in MenuController is initialized in the Constructor.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:166:
views::MenuController::SetDoCancel(true);
On 2017/05/15 19:20:52, xiyuan wrote:
> Is this call necessary?

Yes. I think this is necessary. 
1. Press the shelf items. This will set do_calcel_ to false.
2. Touch anywhere else except the shelf.
If we don't set do_cancel_ back to true here. Then, for the LONG_PRESS events
(open context menu) on the screen will do TransferEvents which is not what we
want.

xiyuan

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc#newcode1663 ash/shelf/shelf_view.cc:1663: if (index == 0 || !item) { On 2017/05/15 ...

3 years, 7 months ago (2017-05-15 22:41:11 UTC) #17

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1663: if (index == 0 || !item) {
On 2017/05/15 22:16:34, minch1 wrote:
> On 2017/05/15 19:20:52, xiyuan wrote:
> > Why we need to include |index| == 0? Can you document it?
> 
> This is for the launcher icon (the first item in the shelf). The gesture
events
> of this item will not go through ShelfButton's OnGestureEvent. This will
produce
> a flash behavior if long press of the item.
> I think it is fine to make the context menu of this item the same as if
touching
> empty space on the shelf. Since the contents of the context menu in these two
> situations are the same.

It is more accurate to check |item| type than the model index since model index
might change in the future e.g. when UX allows launcher button to float.
i.e. item.type == TYPE_APP_LIST

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:676: static bool do_cancel_;
On 2017/05/15 22:16:34, minch1 wrote:
> On 2017/05/15 19:20:52, xiyuan wrote:
> > Why this needs to be static? We already have a static getter for
> MenuController.
> > Can we make it a regular member and use the getter to access it?
> 
> But in shelf_view.cc where where we want to set this variable. We have no
> MenuController instance yet. |active_instance_| returned by
GetActiveInstance()
> in MenuController is initialized in the Constructor. 

How about adding a new MenuRunner::RunTypes mask and MenuRunnerImpl sets this
flag when it sees the new RunTypes bit is set ?

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:166:
views::MenuController::SetDoCancel(true);
On 2017/05/15 22:16:34, minch1 wrote:
> On 2017/05/15 19:20:52, xiyuan wrote:
> > Is this call necessary?
> 
> Yes. I think this is necessary. 
> 1. Press the shelf items. This will set do_calcel_ to false.
> 2. Touch anywhere else except the shelf.
> If we don't set do_cancel_ back to true here. Then, for the LONG_PRESS events
> (open context menu) on the screen will do TransferEvents which is not what we
> want.

That would be another reason to make |do_cancel_| non-static.

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-16 18:34:37 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/60001

3 years, 7 months ago (2017-05-16 18:36:49 UTC) #19

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-16 18:37:38 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/60001

3 years, 7 months ago (2017-05-16 18:39:03 UTC) #21

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-16 22:06:25 UTC) #22

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/446142) win_chromium_x64_rel_ng on ...

3 years, 7 months ago (2017-05-16 22:06:26 UTC) #23

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-16 22:12:44 UTC) #24

minch1

xiyuan@, could you help review? Thanks. https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc#newcode1663 ash/shelf/shelf_view.cc:1663: if (index == ...

3 years, 7 months ago (2017-05-16 22:13:39 UTC) #25

xiyuan@, could you help review? Thanks.

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1663: if (index == 0 || !item) {
On 2017/05/15 22:41:10, xiyuan wrote:
> On 2017/05/15 22:16:34, minch1 wrote:
> > On 2017/05/15 19:20:52, xiyuan wrote:
> > > Why we need to include |index| == 0? Can you document it?
> > 
> > This is for the launcher icon (the first item in the shelf). The gesture
> events
> > of this item will not go through ShelfButton's OnGestureEvent. This will
> produce
> > a flash behavior if long press of the item.
> > I think it is fine to make the context menu of this item the same as if
> touching
> > empty space on the shelf. Since the contents of the context menu in these
two
> > situations are the same.
> 
> It is more accurate to check |item| type than the model index since model
index
> might change in the future e.g. when UX allows launcher button to float.
> i.e. item.type == TYPE_APP_LIST

Done.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:675: // cancel existing touch events.
On 2017/05/15 19:20:52, xiyuan wrote:
> Can we make the document from the perspective of menu? Mentioning dragging
shelf
> item etc does not make much sense in the context of menu. I would prefer
> something like
> 
>   // Whether the menu |owner_| needs gesture events. When set to true
>   // MenuController forwards unhandled gesture events to |owner_|.
>   bool owner_needs_gesture_events_ = false;
> 
> Note the proposed |owner_needs_gesture_events_| is opposite of |do_cancel_|.

Done.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.h:676: static bool do_cancel_;
On 2017/05/15 22:41:10, xiyuan wrote:
> On 2017/05/15 22:16:34, minch1 wrote:
> > On 2017/05/15 19:20:52, xiyuan wrote:
> > > Why this needs to be static? We already have a static getter for
> > MenuController.
> > > Can we make it a regular member and use the getter to access it?
> > 
> > But in shelf_view.cc where where we want to set this variable. We have no
> > MenuController instance yet. |active_instance_| returned by
> GetActiveInstance()
> > in MenuController is initialized in the Constructor. 
> 
> How about adding a new MenuRunner::RunTypes mask and MenuRunnerImpl sets this
> flag when it sees the new RunTypes bit is set ?

Done.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.cc:130: if (parent &&
!views::MenuController::DoCancel()) {
On 2017/05/15 19:20:52, xiyuan wrote:
> nit: Use |menu_controller| instance after making DoCancel non-static.

Done.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_host.h (right):

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.h:70: void TransferEvents(Widget* source,
Widget* target);
On 2017/05/15 19:20:52, xiyuan wrote:
> This does not depend on any MenuHost state and not called externally. So let's
> put it in an anonymous namespace in cc file. Also maybe call it
TransferGesture
> to be more accurate.

Done.

https://codereview.chromium.org/2876203003/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_host.h:86: Widget* owner_ = NULL;
On 2017/05/15 19:20:52, xiyuan wrote:
> NULL -> nullptr

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/60001

3 years, 7 months ago (2017-05-16 22:13:53 UTC) #26

xiyuan

https://codereview.chromium.org/2876203003/diff/60001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/60001/ash/shelf/shelf_button.cc#newcode29 ash/shelf/shelf_button.cc:29: #if defined(OS_CHROMEOS) Ash code is chromeos only now. So ...

3 years, 7 months ago (2017-05-16 22:46:57 UTC) #27

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 03:25:10 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/80001

3 years, 7 months ago (2017-05-17 03:25:49 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 07:52:31 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/446820)

3 years, 7 months ago (2017-05-17 07:52:32 UTC) #31

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 16:50:23 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/80001

3 years, 7 months ago (2017-05-17 16:51:08 UTC) #33

minch1

https://codereview.chromium.org/2876203003/diff/60001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/60001/ash/shelf/shelf_button.cc#newcode29 ash/shelf/shelf_button.cc:29: #if defined(OS_CHROMEOS) On 2017/05/16 22:46:56, xiyuan wrote: > Ash ...

3 years, 7 months ago (2017-05-17 16:55:38 UTC) #34

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 18:04:28 UTC) #35

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/100001

3 years, 7 months ago (2017-05-17 18:05:03 UTC) #36

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 18:08:13 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/120001

3 years, 7 months ago (2017-05-17 18:09:43 UTC) #38

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 18:20:43 UTC) #39

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/140001

3 years, 7 months ago (2017-05-17 18:22:20 UTC) #40

xiyuan

lgtm Cool. Let's get an owner to take a look. How feasible to write a ...

3 years, 7 months ago (2017-05-17 18:26:55 UTC) #41

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 19:49:07 UTC) #42

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-17 19:49:09 UTC) #43

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-17 20:59:27 UTC) #44

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-17 20:59:40 UTC) #45

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-17 21:01:36 UTC) #48

msw

+CC sky for menu code, would you like to review? https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode466 ...

3 years, 7 months ago (2017-05-17 23:03:30 UTC) #49

sky

Please add tests to cover what you are trying to accomplish. https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): ...

3 years, 7 months ago (2017-05-18 16:33:30 UTC) #51

minch1

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode466 ash/shelf/shelf_button.cc:466: views::MenuController* controller = On 2017/05/17 23:03:30, msw wrote: > ...

3 years, 7 months ago (2017-05-18 17:42:57 UTC) #52

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:466: views::MenuController* controller =
On 2017/05/17 23:03:30, msw wrote:
> Can you comment on what this is doing and why?

1.LONG_PRESS the shelf items until the context menu is opened. This will  
transfer events from owner(shelf_widget) to MenuHost. 
2.Release LONG PRESS directly without dragging. The context menu is still
opened. If we touch anywhere else at this time. The context menu will close. And
MenuHost will transfer events back to the owner at this time. But Actually, we
don't want events to be transferred back in this situation.
Since there is a LONG_TAP events followed when release the press. We tried to
|set_owner_needs_gesture_events| to false here. To make sure the events will not
be transferred back in this situation.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:469:
controller->set_owner_needs_gesture_events(false);
On 2017/05/18 16:33:30, sky wrote:
> Instead of changing the menu code, could you accomplish what you need with a
> post target event handler?
> 
> That said, I'm a bit hazy on why you need this. Why do you need to transfer
> events?

I think the reason we need to transfer events here is because the window of the
menu capture all the events when the menu is created? But it did nothing for the
gesture events of the shelf items. So we cannot dragged the shelf items when
context menu is created. 

Transfer events from the owner to the menu. Then we can handle the events that
not happened on the menu. Make sure it will not be dropped. Then, after menu is
closed, if needed transfer the events back to make sure the owner can continue
the events.
xiyuan@, please help explain a little more if I made something wrong? Thanks.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1694: if (wm_shelf()->auto_hide_behavior() ==
SHELF_AUTO_HIDE_BEHAVIOR_NEVER)
On 2017/05/17 23:03:30, msw wrote:
> It's not clear to me how the comment above relates to this code... can you
> explain this a bit more?

We cannot drag the shelf items when the shelf is auto-hidden (for the SCROLL
events, before open the context menu.) I want to keep the same behavior as
SCROLL events. So, if shelf is auto-hidden, we will not transfer events from
owner to the menu host. Then, shelf items can not be dragged after the context
menu is created.
Set |run_types| here, then, in menu_runner_impl.cc will
|set_owner_needs_gesture_events| according to the |run_types|. Then, in MenuHost
we can know whether we need to transfer events or not.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.h:680: #if !defined(OS_MACOSX)
On 2017/05/17 23:03:30, msw wrote:
> Why is this non-mac?

Actually, I am not sure about this. xiyuan@, can you help explain a little bit?
Thanks.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:94: // Owner of the menu can be dragged
after menu is created. For example the
On 2017/05/17 23:03:30, msw wrote:
> This is weird to me, I'd like to see it in action. Does dragging close the
menu?

Yes, dragging will close the menu.

xiyuan

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode469 ash/shelf/shelf_button.cc:469: controller->set_owner_needs_gesture_events(false); On 2017/05/18 17:42:57, minch1 wrote: > On 2017/05/18 ...

3 years, 7 months ago (2017-05-18 18:00:47 UTC) #53

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:469:
controller->set_owner_needs_gesture_events(false);
On 2017/05/18 17:42:57, minch1 wrote:
> On 2017/05/18 16:33:30, sky wrote:
> > Instead of changing the menu code, could you accomplish what you need with a
> > post target event handler?
> > 
> > That said, I'm a bit hazy on why you need this. Why do you need to transfer
> > events?
> 
> I think the reason we need to transfer events here is because the window of
the
> menu capture all the events when the menu is created? But it did nothing for
the
> gesture events of the shelf items. So we cannot dragged the shelf items when
> context menu is created. 
> 
> Transfer events from the owner to the menu. Then we can handle the events that
> not happened on the menu. Make sure it will not be dropped. Then, after menu
is
> closed, if needed transfer the events back to make sure the owner can continue
> the events.
> xiyuan@, please help explain a little more if I made something wrong? Thanks.

Each widget has a GestureProvider that translates touch events to gesture
events. The GestureProvider starts the work when it sees a TAP_DOWN. When the
context menu is created, it captures all touch events. But since we don't yet
lift the fingure, it will not generate gestures and silently drop all touch
events. By TransferGesture, we make MenuHost continue to translate the touch
events to gesture events. We forward the unhandled gestures to Shelf widget so
that the dragging can be continued after the menu is open.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.h:680: #if !defined(OS_MACOSX)
On 2017/05/18 17:42:57, minch1 wrote:
> On 2017/05/17 23:03:30, msw wrote:
> > Why is this non-mac?
> 
> Actually, I am not sure about this. xiyuan@, can you help explain a little
bit?
> Thanks.

GestureRecognizer does not work on Mac. GestureRecognizerImplMac is currently a
dummy impl to make compiler happy. Also Mac does not use Views code fully.
Widget::GetNativeView gives NSView* and it would not compile with
TransferEventsTo since NSView* is not a GestureConsumer*.

sky

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode469 ash/shelf/shelf_button.cc:469: controller->set_owner_needs_gesture_events(false); On 2017/05/18 17:42:57, minch1 wrote: > On 2017/05/18 ...

3 years, 7 months ago (2017-05-18 19:43:12 UTC) #54

msw

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode466 ash/shelf/shelf_button.cc:466: views::MenuController* controller = On 2017/05/18 17:42:57, minch1 wrote: > ...

3 years, 7 months ago (2017-05-18 20:42:20 UTC) #55

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:466: views::MenuController* controller =
On 2017/05/18 17:42:57, minch1 wrote:
> On 2017/05/17 23:03:30, msw wrote:
> > Can you comment on what this is doing and why?
> 
> 1.LONG_PRESS the shelf items until the context menu is opened. This will  
> transfer events from owner(shelf_widget) to MenuHost. 
> 2.Release LONG PRESS directly without dragging. The context menu is still
> opened. If we touch anywhere else at this time. The context menu will close.
And
> MenuHost will transfer events back to the owner at this time. But Actually, we
> don't want events to be transferred back in this situation.
> Since there is a LONG_TAP events followed when release the press. We tried to
> |set_owner_needs_gesture_events| to false here. To make sure the events will
not
> be transferred back in this situation.

It's not clear to me (at least from that explanation) why we need to
|set_owner_needs_gesture_events(false)| instead of just handling/ignoring this
one LONG_TAP event. Also, I'm hoping that you'll add a comment in code, not just
on this CL review, readers of this code probably won't understand what's
happening or why here.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1694: if (wm_shelf()->auto_hide_behavior() ==
SHELF_AUTO_HIDE_BEHAVIOR_NEVER)
On 2017/05/18 17:42:57, minch1 wrote:
> On 2017/05/17 23:03:30, msw wrote:
> > It's not clear to me how the comment above relates to this code... can you
> > explain this a bit more?
> 
> We cannot drag the shelf items when the shelf is auto-hidden (for the SCROLL
> events, before open the context menu.) I want to keep the same behavior as
> SCROLL events. So, if shelf is auto-hidden, we will not transfer events from
> owner to the menu host. Then, shelf items can not be dragged after the context
> menu is created.
> Set |run_types| here, then, in menu_runner_impl.cc will
> |set_owner_needs_gesture_events| according to the |run_types|. Then, in
MenuHost
> we can know whether we need to transfer events or not.

Why won't the shelf itself correctly ignore drag touch events when it's in an
auto-hide mode (ie. we shouldn't change the menu's run type based on shelf
state, the shelf itself should handle/ignore gesture events correctly depending
on its internal state). If you need to keep this, add an explanatory comment in
code, like "Shelf items can not be dragged if the shelf is not autohide."

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.h:680: #if !defined(OS_MACOSX)
On 2017/05/18 18:00:47, xiyuan wrote:
> On 2017/05/18 17:42:57, minch1 wrote:
> > On 2017/05/17 23:03:30, msw wrote:
> > > Why is this non-mac?
> > 
> > Actually, I am not sure about this. xiyuan@, can you help explain a little
> bit?
> > Thanks.
> 
> GestureRecognizer does not work on Mac. GestureRecognizerImplMac is currently
a
> dummy impl to make compiler happy. Also Mac does not use Views code fully.
> Widget::GetNativeView gives NSView* and it would not compile with
> TransferEventsTo since NSView* is not a GestureConsumer*.

Can we try to reduce the preprocessor blocks a bit? (ie. just prevent the
inapplicable code from compiling, and compile everything possible, like this
flag, regardless).

minch1

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode466 ash/shelf/shelf_button.cc:466: views::MenuController* controller = On 2017/05/18 20:42:20, msw wrote: > ...

3 years, 7 months ago (2017-05-18 22:02:54 UTC) #56

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:466: views::MenuController* controller =
On 2017/05/18 20:42:20, msw wrote:
> On 2017/05/18 17:42:57, minch1 wrote:
> > On 2017/05/17 23:03:30, msw wrote:
> > > Can you comment on what this is doing and why?
> > 
> > 1.LONG_PRESS the shelf items until the context menu is opened. This will  
> > transfer events from owner(shelf_widget) to MenuHost. 
> > 2.Release LONG PRESS directly without dragging. The context menu is still
> > opened. If we touch anywhere else at this time. The context menu will close.
> And
> > MenuHost will transfer events back to the owner at this time. But Actually,
we
> > don't want events to be transferred back in this situation.
> > Since there is a LONG_TAP events followed when release the press. We tried
to
> > |set_owner_needs_gesture_events| to false here. To make sure the events will
> not
> > be transferred back in this situation.
> 
> It's not clear to me (at least from that explanation) why we need to
> |set_owner_needs_gesture_events(false)| instead of just handling/ignoring this
> one LONG_TAP event. Also, I'm hoping that you'll add a comment in code, not
just
> on this CL review, readers of this code probably won't understand what's
> happening or why here.

We handled the LONG_TAP here to make it will not open a context menu again.
Besides that we also want to make sure the MenuHost will not transfer events
back in this situation. That is why |set_owner_needs_gesture_events(false)|
here.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:469:
controller->set_owner_needs_gesture_events(false);
On 2017/05/18 19:43:12, sky wrote:
> On 2017/05/18 17:42:57, minch1 wrote:
> > On 2017/05/18 16:33:30, sky wrote:
> > > Instead of changing the menu code, could you accomplish what you need with
a
> > > post target event handler?
> > > 
> > > That said, I'm a bit hazy on why you need this. Why do you need to
transfer
> > > events?
> > 
> > I think the reason we need to transfer events here is because the window of
> the
> > menu capture all the events when the menu is created? But it did nothing for
> the
> > gesture events of the shelf items. So we cannot dragged the shelf items when
> > context menu is created. 
> 
> Why is that a problem though? Shouldn't the user close the menu, and then
> attempt the gesture to do the drag?
> 
> > 
> > Transfer events from the owner to the menu. Then we can handle the events
that
> > not happened on the menu. Make sure it will not be dropped. Then, after menu
> is
> > closed, if needed transfer the events back to make sure the owner can
continue
> > the events.
> > xiyuan@, please help explain a little more if I made something wrong?
Thanks.
> 

I guess this is a requirement from UX?

msw

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_button.cc#newcode466 ash/shelf/shelf_button.cc:466: views::MenuController* controller = On 2017/05/18 22:02:53, minch1 wrote: > ...

3 years, 7 months ago (2017-05-18 22:14:47 UTC) #57

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-23 22:06:07 UTC) #58

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/160001

3 years, 7 months ago (2017-05-23 22:07:01 UTC) #59

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-23 22:30:59 UTC) #60

Description was changed from

==========
Make shelf item can be dragged when context menu is opened.

Changes in this cl,
1. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press.
2. Handle the LONG_TAP events on shelf items. Make sure the events will
not be transferred back in this situation which is not needed.
3. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.
4. UT maybe later.

BUG=710601
==========

to

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events are
sent back from menu. LONG_TAP should be handled by the shelf to avoid 
to show context menu again. 
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.
5. UT maybe later.

BUG=710601
==========

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-23 23:13:15 UTC) #61

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-23 23:13:16 UTC) #62

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 00:30:48 UTC) #63

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/180001

3 years, 7 months ago (2017-05-26 00:32:04 UTC) #64

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 00:48:15 UTC) #65

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/347929)

3 years, 7 months ago (2017-05-26 00:48:16 UTC) #66

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 02:59:11 UTC) #67

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/180001

3 years, 7 months ago (2017-05-26 03:00:31 UTC) #68

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 03:16:58 UTC) #69

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/393749)

3 years, 7 months ago (2017-05-26 03:16:59 UTC) #70

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 15:37:17 UTC) #71

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/180001

3 years, 7 months ago (2017-05-26 15:37:40 UTC) #72

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 15:52:10 UTC) #73

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/348313)

3 years, 7 months ago (2017-05-26 15:52:12 UTC) #74

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 7 months ago (2017-05-26 17:43:33 UTC) #75

Description was changed from

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events are
sent back from menu. LONG_TAP should be handled by the shelf to avoid 
to show context menu again. 
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.
5. UT maybe later.

BUG=710601
==========

to

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.
5. UT maybe later.

BUG=710601
==========

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 18:23:54 UTC) #76

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/200001

3 years, 7 months ago (2017-05-26 18:24:28 UTC) #77

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-26 19:33:08 UTC) #78

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-26 19:33:09 UTC) #79

minch1

minch@chromium.org changed reviewers: + sadrul@chromium.org

3 years, 7 months ago (2017-05-26 20:50:02 UTC) #80

minch1

Could you please help review? Thanks. https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.cc#newcode1693 ash/shelf/shelf_view.cc:1693: // Shelf items ...

3 years, 7 months ago (2017-05-26 23:01:17 UTC) #81

Could you please help review? Thanks.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1693: // Shelf items can only be dragged if the shelf is
not autohide.
On 2017/05/17 23:03:30, msw wrote:
> nit: s/autohide/in auto-hide/ (or 'is not auto-hidden')

update the comment according to next comment.

https://codereview.chromium.org/2876203003/diff/140001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1694: if (wm_shelf()->auto_hide_behavior() ==
SHELF_AUTO_HIDE_BEHAVIOR_NEVER)
On 2017/05/18 20:42:20, msw wrote:
> On 2017/05/18 17:42:57, minch1 wrote:
> > On 2017/05/17 23:03:30, msw wrote:
> > > It's not clear to me how the comment above relates to this code... can you
> > > explain this a bit more?
> > 
> > We cannot drag the shelf items when the shelf is auto-hidden (for the SCROLL
> > events, before open the context menu.) I want to keep the same behavior as
> > SCROLL events. So, if shelf is auto-hidden, we will not transfer events from
> > owner to the menu host. Then, shelf items can not be dragged after the
context
> > menu is created.
> > Set |run_types| here, then, in menu_runner_impl.cc will
> > |set_owner_needs_gesture_events| according to the |run_types|. Then, in
> MenuHost
> > we can know whether we need to transfer events or not.
> 
> Why won't the shelf itself correctly ignore drag touch events when it's in an
> auto-hide mode (ie. we shouldn't change the menu's run type based on shelf
> state, the shelf itself should handle/ignore gesture events correctly
depending
> on its internal state). If you need to keep this, add an explanatory comment
in
> code, like "Shelf items can not be dragged if the shelf is not autohide."

Done.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.h:680: #if !defined(OS_MACOSX)
On 2017/05/18 20:42:20, msw wrote:
> On 2017/05/18 18:00:47, xiyuan wrote:
> > On 2017/05/18 17:42:57, minch1 wrote:
> > > On 2017/05/17 23:03:30, msw wrote:
> > > > Why is this non-mac?
> > > 
> > > Actually, I am not sure about this. xiyuan@, can you help explain a little
> > bit?
> > > Thanks.
> > 
> > GestureRecognizer does not work on Mac. GestureRecognizerImplMac is
currently
> a
> > dummy impl to make compiler happy. Also Mac does not use Views code fully.
> > Widget::GetNativeView gives NSView* and it would not compile with
> > TransferEventsTo since NSView* is not a GestureConsumer*.
> 
> Can we try to reduce the preprocessor blocks a bit? (ie. just prevent the
> inapplicable code from compiling, and compile everything possible, like this
> flag, regardless).

Done.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.cc:153: // Cancel existing touches, so we don't
miss some touch release/cancel
On 2017/05/17 23:03:30, msw wrote:
> nit: move comment into else?

Done.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_host.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.h:81: #if !defined(OS_MACOSX)
On 2017/05/17 23:03:30, msw wrote:
> ditto question. If you keep all these blocks, commend on the #else and #endif
to
> point out what block it's part of (ie. #endif  // !defined(OS_MACOSX))

Done.

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2876203003/diff/140001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:96: OWNER_NEEDS_DRAG = 1 << 7,
On 2017/05/17 23:03:30, msw wrote:
> nit: SEND_UNHANDLED_GESTURES_TO_OWNER or similar?

Done.

xiyuan

https://codereview.chromium.org/2876203003/diff/200001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2876203003/diff/200001/ui/views/controls/menu/menu_controller.cc#newcode834 ui/views/controls/menu/menu_controller.cc:834: // gesture events on the shelf can be created ...

3 years, 7 months ago (2017-05-26 23:13:38 UTC) #82

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-26 23:52:49 UTC) #83

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/220001

3 years, 7 months ago (2017-05-26 23:53:10 UTC) #84

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-27 00:02:19 UTC) #85

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/348877) chromeos_daisy_chromium_compile_only_ng on ...

3 years, 7 months ago (2017-05-27 00:02:20 UTC) #86

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-05-29 20:56:51 UTC) #87

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/240001

3 years, 6 months ago (2017-05-29 20:57:04 UTC) #88

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-05-29 22:20:44 UTC) #89

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-05-29 22:20:46 UTC) #90

minch1

https://codereview.chromium.org/2876203003/diff/200001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2876203003/diff/200001/ui/views/controls/menu/menu_controller.cc#newcode834 ui/views/controls/menu/menu_controller.cc:834: // gesture events on the shelf can be created ...

3 years, 6 months ago (2017-06-01 16:42:28 UTC) #91

xiyuan

The approach looks fine to me. sky@, sadural@ what do you think? minch@, could you ...

3 years, 6 months ago (2017-06-01 16:55:13 UTC) #92

msw

Nice, some comment nits. Tests seem like a very good idea. https://codereview.chromium.org/2876203003/diff/240001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): ...

3 years, 6 months ago (2017-06-01 20:24:28 UTC) #93

sky

On 2017/06/01 20:24:28, msw wrote: > Nice, some comment nits. Tests seem like a very ...

3 years, 6 months ago (2017-06-01 22:01:16 UTC) #94

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-02 18:26:08 UTC) #95

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/260001

3 years, 6 months ago (2017-06-02 18:26:56 UTC) #96

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-02 19:12:27 UTC) #97

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/469076)

3 years, 6 months ago (2017-06-02 19:12:29 UTC) #98

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-20 17:58:39 UTC) #99

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/280001

3 years, 6 months ago (2017-06-20 17:59:04 UTC) #100

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-20 18:15:24 UTC) #101

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/410886)

3 years, 6 months ago (2017-06-20 18:15:25 UTC) #102

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-20 18:27:05 UTC) #103

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/300001

3 years, 6 months ago (2017-06-20 18:27:25 UTC) #104

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-20 19:46:06 UTC) #105

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-20 19:46:08 UTC) #106

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 6 months ago (2017-06-23 01:08:27 UTC) #107

Description was changed from

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.
5. UT maybe later.

BUG=710601
==========

to

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.

BUG=710601
==========

minch1

Really sorry for the delay. For "The shelf can then install a pre-target handler on ...

3 years, 6 months ago (2017-06-23 23:13:03 UTC) #108

sadrul

On 2017/06/23 23:13:03, minch1 wrote: > Really sorry for the delay. For "The shelf can ...

3 years, 5 months ago (2017-06-27 03:05:13 UTC) #109

minch1

On 2017/06/27 03:05:13, sadrul wrote: > On 2017/06/23 23:13:03, minch1 wrote: > > Really sorry ...

3 years, 5 months ago (2017-06-27 18:39:12 UTC) #110

sadrul

On 2017/06/27 18:39:12, minch1 wrote: > On 2017/06/27 03:05:13, sadrul wrote: > > On 2017/06/23 ...

3 years, 5 months ago (2017-06-28 04:57:48 UTC) #111

xiyuan

On 2017/06/28 04:57:48, sadrul wrote: > On 2017/06/27 18:39:12, minch1 wrote: > > On 2017/06/27 ...

3 years, 5 months ago (2017-06-28 14:53:54 UTC) #112

On 2017/06/28 04:57:48, sadrul wrote:
> On 2017/06/27 18:39:12, minch1 wrote:
> > On 2017/06/27 03:05:13, sadrul wrote:
> > > On 2017/06/23 23:13:03, minch1 wrote:
> > > > Really sorry for the delay. For "The shelf can then install a
> > > > pre-target handler on the menu, so that it can see when
> > > > there are gesture-scroll-update events with sufficient delta
> > > > from the originating location." 
> > > > Shelf should get the target before install the pre-target handler. But
> looks
> > > > like we don't have the api to get the target MenuHost. And it is buried
> > deeply
> > > > in SubmenuView.
> > > > One more concern it that is that good to expose MenuHost
> > > > outside the menu system (eg, shelf)?
> > > 
> > > I don't think you need to get access to the MenuHost. You should be able
to
> > > install a pre-target handler (or views::PointerWatcher) on ash::Shell, and
> > that
> > > should be sufficient for you to decide when to close the menu and start
the
> > > drag.
> > > 
> > > > One more thing is that since the MenuHost captures all the event. In
order
> > to
> > > > make sure that gesture sequence remains alive. I think transfer events
in
> > > > menuhost is necessary?
> > > 
> > > I think so. The alternative would be to introduce some amount of
complexity
> in
> > > menu so that it is possible to have some menus without having a capture,
> which
> > I
> > > think would be more complex than we want it to be.
> > 
> > Thanks. Let me try it.
> 
> Just to clarify: I assume you mean you will try the pre-target
> handler/PointerWatcher on ash::Shell approach (as opposed to not transfering
> gesture sequence to menu)

I don't understand the benefits of using pre-target approach. To keep the
gesture alive, we would need the MenuRunner flag so that MenuHost knows to
TransferGesture during construction. So most of the menu code change would stay.
Pre-target handler only saves the code to forward the gesture events out to the
host/owner of the menu. What did I miss?

msw

lgtm with test nits; follow up with xiyuan and sadrul, but fwiw, I think the ...

3 years, 5 months ago (2017-06-29 19:02:10 UTC) #113

lgtm with test nits; follow up with xiyuan and sadrul, but fwiw, I think the
current approach (*without* a pre-target handler) is okay.

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:156: class TestWidget :
public Widget {
nit: |GestureTestWidget| and comment "A test widget that counts gesture events."

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:535: owner_.reset(new
TestWidget);
nit: owner = base::MakeUnique<TestWidget>(); (optionally ditto elsewhere)

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1133: // Gesture triggered
outside the menu.
nit: "The menu's owner should receive gestures triggered outside the menu."
(move this below the new section testing behavior when the flag is false)

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1138:
controller->set_send_gesture_events_to_owner(true);
optional nit: test that gesture events are not sent to the owner when the flag
is not set, ie. before setting the flag, do something like this:
  // Gesture events should not be forwarded if the flag is not set.
  EXPECT_EQ(CountOwnerOnGestureEvent(), 0);
  EXPECT_FALSE(controller->send_gesture_events_to_owner());
  controller->OnGestureEvent(sub_menu, &event);
  EXPECT_EQ(CountOwnerOnGestureEvent(), 0);

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1148: // ET_GESTURE_END
disabled preserve_current_gesture_for_owner. Then
nit: ET_GESTURE_END resets the |send_gesture_events_to_owner| flag, so further
gesture events should not be sent to the owner.

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 5 months ago (2017-06-29 21:06:43 UTC) #114

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/320001

3 years, 5 months ago (2017-06-29 21:07:11 UTC) #115

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 5 months ago (2017-06-29 22:16:22 UTC) #116

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 5 months ago (2017-06-29 22:16:24 UTC) #117

minch1

Thanks for your review. Fixed comments. And can I land it? https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu/menu_controller_unittest.cc File ui/views/controls/menu/menu_controller_unittest.cc (right): ...

3 years, 5 months ago (2017-06-30 19:53:08 UTC) #118

Thanks for your review. Fixed comments. And can I land it?

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller_unittest.cc (right):

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:156: class TestWidget :
public Widget {
On 2017/06/29 19:02:10, msw wrote:
> nit: |GestureTestWidget| and comment "A test widget that counts gesture
events."

Done.

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:535: owner_.reset(new
TestWidget);
On 2017/06/29 19:02:10, msw wrote:
> nit: owner = base::MakeUnique<TestWidget>(); (optionally ditto elsewhere)

Done.

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1133: // Gesture triggered
outside the menu.
On 2017/06/29 19:02:10, msw wrote:
> nit: "The menu's owner should receive gestures triggered outside the menu."
> (move this below the new section testing behavior when the flag is false)

Done.

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1138:
controller->set_send_gesture_events_to_owner(true);
On 2017/06/29 19:02:10, msw wrote:
> optional nit: test that gesture events are not sent to the owner when the flag
> is not set, ie. before setting the flag, do something like this:
>   // Gesture events should not be forwarded if the flag is not set.
>   EXPECT_EQ(CountOwnerOnGestureEvent(), 0);
>   EXPECT_FALSE(controller->send_gesture_events_to_owner());
>   controller->OnGestureEvent(sub_menu, &event);
>   EXPECT_EQ(CountOwnerOnGestureEvent(), 0);

Done.

https://codereview.chromium.org/2876203003/diff/300001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller_unittest.cc:1148: // ET_GESTURE_END
disabled preserve_current_gesture_for_owner. Then
On 2017/06/29 19:02:10, msw wrote:
> nit: ET_GESTURE_END resets the |send_gesture_events_to_owner| flag, so further
> gesture events should not be sent to the owner.

Done.

msw

lgtm with a nit; please wait for xiyuan or sadrul to reply before landing. https://codereview.chromium.org/2876203003/diff/320001/ui/views/controls/menu/menu_controller_unittest.cc ...

3 years, 5 months ago (2017-06-30 20:22:39 UTC) #119

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 4 months ago (2017-08-04 01:08:03 UTC) #120

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/340001

3 years, 4 months ago (2017-08-04 01:08:10 UTC) #121

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 4 months ago (2017-08-04 03:15:27 UTC) #122

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 4 months ago (2017-08-04 03:15:28 UTC) #123

minch1

Hi, sadrul@, xiyuan@, what's the further suggestion for this cl? https://codereview.chromium.org/2876203003/diff/320001/ui/views/controls/menu/menu_controller_unittest.cc File ui/views/controls/menu/menu_controller_unittest.cc (right): https://codereview.chromium.org/2876203003/diff/320001/ui/views/controls/menu/menu_controller_unittest.cc#newcode1139 ...

3 years, 4 months ago (2017-08-04 17:22:35 UTC) #124

sadrul

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu/menu_controller.cc#newcode840 ui/views/controls/menu/menu_controller.cc:840: if (send_gesture_events_to_owner()) { This should happen at the very ...

3 years, 3 months ago (2017-08-30 20:31:40 UTC) #126

minch1

Hi, Sadrul, please check the new ps. Thanks. https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu/menu_controller.cc File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu/menu_controller.cc#newcode840 ui/views/controls/menu/menu_controller.cc:840: if ...

3 years, 3 months ago (2017-08-31 23:21:03 UTC) #127

Hi, Sadrul, please check the new ps. Thanks.

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:840: if
(send_gesture_events_to_owner()) {
On 2017/08/30 20:31:39, sadrul wrote:
> This should happen at the very beginning?

Removed to the entry of OnGestureEvent, then we can have a quick return if we
need to send gesture events back to the owner.

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.cc:134: owner_ = parent;
On 2017/08/30 20:31:39, sadrul wrote:
> What if |owner_| is destroyed after the menu comes up? If we expect it won't
> happen, can we validate that (e.g. add some DCHECK()s somewhere)?

Added DCHECK(owner()) when we need to send events back in
MenuController::OnGestureEvent. In HideMenuHost checked the owner_ before
transfer events back to the owner.

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.cc:151: #if !defined(OS_MACOSX)
On 2017/08/30 20:31:39, sadrul wrote:
> Do we need the OS_MACOSX checks?

I think so. Since GestureRecognizerImplMac currently is stub implementation of
GestureRecognizer for Mac.

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.cc:169: }
On 2017/08/30 20:31:39, sadrul wrote:
> An alternate approach, although probably hackier than this:
> 
>   internal::TransferGesture(owner_, this);
>   SetCapture();
>   internal::TransformGesture(this, owner_);
> 
> We shouldn't need to worry about owner_ going away, or transfering gesture
> events back to owner_ etc. that way.

Do you mean to do all of these in ShowMenuHost? Tried it. But it seems that if
we transfer the events back to owner_ and keep MenuHost set capture at the same
time. Both owner_ and MenuController will not receive the touch and gesture
events.

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-08-31 23:21:35 UTC) #128

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/360001

3 years, 3 months ago (2017-08-31 23:21:39 UTC) #129

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 3 months ago (2017-08-31 23:59:09 UTC) #130

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_headless_rel/builds/157885)

3 years, 3 months ago (2017-08-31 23:59:10 UTC) #131

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-09-01 00:56:09 UTC) #132

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/360001

3 years, 3 months ago (2017-09-01 00:56:20 UTC) #133

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-09-01 02:19:57 UTC) #134

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/380001

3 years, 3 months ago (2017-09-01 02:20:10 UTC) #135

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 3 months ago (2017-09-01 02:28:19 UTC) #136

Description was changed from

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is created.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is created.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. The context menu of the launcher item is changed to be the same as
the context menu when touching white space on the shelf. Make sure
the touch events on the launcher item will not influence the dragging
of the shelf items.

BUG=710601
==========

to

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is opened.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is opened.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. Add set_run_types() in ShelfView, since only shelf button need to
transfer events from menu back to the owner.

BUG=710601,748253
==========

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-09-01 02:35:36 UTC) #137

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/400001

3 years, 3 months ago (2017-09-01 02:35:40 UTC) #138

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 3 months ago (2017-09-01 03:33:31 UTC) #139

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 3 months ago (2017-09-01 03:33:32 UTC) #140

minch1

Hi, I just changed the logic in ShelfView. I think we only need to transfer ...

3 years, 3 months ago (2017-09-01 03:37:34 UTC) #141

xiyuan

On 2017/09/01 03:37:34, minch1 wrote: > Hi, I just changed the logic in ShelfView. I ...

3 years, 3 months ago (2017-09-01 17:13:54 UTC) #142

minch1

On 2017/09/01 17:13:54, xiyuan wrote: > On 2017/09/01 03:37:34, minch1 wrote: > > Hi, I ...

3 years, 3 months ago (2017-09-01 17:55:20 UTC) #143

xiyuan

On 2017/09/01 17:55:20, minch1 wrote: > On 2017/09/01 17:13:54, xiyuan wrote: > > On 2017/09/01 ...

3 years, 3 months ago (2017-09-01 18:02:31 UTC) #144

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-09-01 20:44:19 UTC) #145

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/420001

3 years, 3 months ago (2017-09-01 20:44:31 UTC) #146

minch1

On 2017/09/01 18:02:31, xiyuan wrote: > On 2017/09/01 17:55:20, minch1 wrote: > > On 2017/09/01 ...

3 years, 3 months ago (2017-09-01 20:49:43 UTC) #147

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 3 months ago (2017-09-01 22:05:28 UTC) #149

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 3 months ago (2017-09-01 22:05:30 UTC) #150

minch1

Description was changed from ========== Make shelf item can be dragged when context menu is ...

3 years, 3 months ago (2017-09-05 17:04:10 UTC) #151

Description was changed from

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is opened.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is opened.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.
4. Add set_run_types() in ShelfView, since only shelf button need to
transfer events from menu back to the owner.

BUG=710601,748253
==========

to

==========
Make shelf item can be dragged when context menu is opened.

Current context menu of shelf,
1) Allow dragging before context menu is created.
2) No dragging after context menu is opened.

In order to keep the same as Android OS,
1) Allow dragging during the same press (don't release the finger)
after context menu is opened.
2) Close the context menu once start dragging.

Changes in this cl,
1. Add |SEND_UNHANDLED_GESTURES_TO_OWNER| in MenuRunner. This means the
owner hopes that menu can help send the unhandled gesture events to the
owner instead of dropping them. e.g, the context menu of the shelf
items, and this is only true if shelf is not autohide.
2. Transfer events from owner to menu host if the owner needs gesture 
events. vice verse. e.g. Shelf items hope can be dragged after long 
press which will create the context menu.
3. Handle the LONG_TAP events on shelf items. Since gesture events 
been transferred from shelf to MenuHost, but we don't want it will show
the context menu again.

BUG=710601,748253
==========

sadrul

Sorry, I do not actively look at rietveld anymore. Please feel free to ping over ...

3 years, 3 months ago (2017-09-07 19:58:28 UTC) #152

minch1

3 years, 3 months ago (2017-09-07 20:54:56 UTC) #153

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/2876203003/diff/340001/ui/views/controls/menu...
ui/views/controls/menu/menu_host.cc:134: owner_ = parent;
On 2017/09/07 19:58:28, sadrul wrote:
> On 2017/08/31 23:21:02, minch1 wrote:
> > On 2017/08/30 20:31:39, sadrul wrote:
> > > What if |owner_| is destroyed after the menu comes up? If we expect it
won't
> > > happen, can we validate that (e.g. add some DCHECK()s somewhere)?
> > 
> > Added DCHECK(owner()) when we need to send events back in
> > MenuController::OnGestureEvent. In HideMenuHost checked the owner_ before
> > transfer events back to the owner.
> 
> If |owner_| is destroyed, the DCHECK() won't hit though, right? So that
DCHECK()
> is not really verifying that we are using an existing owner.

Do you mean that |parent| is destroyed after assigned to |owner_| here? But in
HideMenuHost we may still transfer events back to a not exist |parent|?

minch1

The CQ bit was checked by minch@chromium.org to run a CQ dry run

3 years, 3 months ago (2017-09-07 23:40:35 UTC) #154

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876203003/440001

3 years, 3 months ago (2017-09-07 23:40:41 UTC) #155

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 3 months ago (2017-09-08 01:11:42 UTC) #156

commit-bot: I haz the power

3 years, 3 months ago (2017-09-08 01:11:44 UTC) #157

Dry run: This issue passed the CQ dry run.

Issue 2876203003: Make shelf item can be dragged when context menu is opened.

Description

Patch Set 1 #

Patch Set 2 : Transfer events only when the widget set capture. #

Patch Set 3 : TransferEvents only if it is needed. #

Patch Set 4 : Fixed comments. #

Patch Set 5 : Fixed comments. #

Patch Set 6 : Remove useless code. #

Patch Set 7 : Remove useless include. #

Patch Set 8 : Fixed comments. #

Patch Set 9 : Fixed comments. #

Patch Set 10 : Stop listening events if sees GESTURE_END. #

Patch Set 11 : Rebased and renamed. #

Patch Set 12 : Addressed xiyuan's comments. #

Patch Set 13 : rebased #

Patch Set 14 : Add ut. #

Patch Set 15 : Fixed comments and rebased. #

Patch Set 16 : Fixed tests. #

Patch Set 17 : Fixed msw's comments. #

Patch Set 18 : Fixed nit. #

Patch Set 19 : Fixed comments. #

Patch Set 20 : Add set_run_types() in ShelfView. #

Patch Set 21 : Removed the logic for app list button. #

Patch Set 22 : Remove run_types_ flag. #

Patch Set 23 : Add widget observer for |owner_| in MenuHost. #

Messages