Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809073002:
cros: allow aura window not considered activatable for pointer event (Closed)
|
Created:
3 years, 8 months ago by Qiang(Joe) Xu Modified:
3 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: allow aura window not considered activatable for pointer event
Changes:
Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event.
This is a stage one implementation for crbug.com/657525.
BUG=657525
TEST=added test coverage
Review-Url: https://codereview.chromium.org/2809073002
Cr-Commit-Position: refs/heads/master@{#467849}
Committed: https://chromium.googlesource.com/chromium/src/+/e47fb498dce50d65d9c3b55fafdbffbe50ab6920
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase and fix conflicts #
Total comments: 4
Patch Set 3 : feedback #
Total comments: 6
Patch Set 4 : feedback from oshima and bugtracker #
Total comments: 8
Patch Set 5 : feedback from sky #Patch Set 6 : ui::Event* #
Total comments: 10
Patch Set 7 : rebase #Patch Set 8 : feedback from sky #
Total comments: 2
Patch Set 9 : const ui::Event* #
Total comments: 2
Messages
Total messages: 67 (41 generated)
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cros: allow aura window not considered activatable for click event BUG=657525 ========== to ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. BUG=657525 TEST=added test coverage ==========
warx@chromium.org changed reviewers: + oshima@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. BUG=657525 TEST=added test coverage ========== to ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. This is a stage one for crbug.com/657525 BUG=657525 TEST=added test coverage ==========
Description was changed from ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. This is a stage one for crbug.com/657525 BUG=657525 TEST=added test coverage ========== to ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ==========
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
warx@chromium.org changed reviewers: + sky@chromium.org
Hi all, PTAL, thanks! https://codereview.chromium.org/2809073002/diff/1/ash/wm/ash_focus_rules.h File ash/wm/ash_focus_rules.h (left): https://codereview.chromium.org/2809073002/diff/1/ash/wm/ash_focus_rules.h#ol... ash/wm/ash_focus_rules.h:23: bool IsWindowConsideredActivatable(aura::Window* window) const; This is actually not used anywhere. I think we should remove this API.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you outline where you are planning on going with this?
On 2017/04/11 16:50:21, sky wrote: > Could you outline where you are planning on going with this? The next step would be adding an app.currentWindowInternal.setActivateOnClick API. That is defining SetActivateOnClick in NativeAppWindow and implement it only in ChromeNativeAppWindowViewsAuraAsh, where we get the native window and set this window property. The other app window api procedure is pretty like this CL: https://codereview.chromium.org/26427002.
https://codereview.chromium.org/2809073002/diff/20001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/20001/ui/aura/client/aura_con... ui/aura/client/aura_constants.h:40: AURA_EXPORT extern const WindowProperty<bool>* const kActivateOnClickKey; This doesn't have to be in aura because the user is ash impl of the app window. You can probably move this to AshFocusRule, with a utility function to set this property on the window. https://codereview.chromium.org/2809073002/diff/20001/ui/wm/core/focus_contro... File ui/wm/core/focus_controller.cc (right): https://codereview.chromium.org/2809073002/diff/20001/ui/wm/core/focus_contro... ui/wm/core/focus_controller.cc:359: if (rules_->CanFocusWindow(GetToplevelWindow(window))) { Can we just pass the event here?
Patchset #3 (id:40001) has been deleted
Updated, PTAL, thanks https://codereview.chromium.org/2809073002/diff/20001/ui/aura/client/aura_con... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/20001/ui/aura/client/aura_con... ui/aura/client/aura_constants.h:40: AURA_EXPORT extern const WindowProperty<bool>* const kActivateOnClickKey; On 2017/04/11 18:57:52, oshima wrote: > This doesn't have to be in aura because the user is ash impl of the app window. > You can probably move this to AshFocusRule, with a utility function to set this > property on the window. Done. https://codereview.chromium.org/2809073002/diff/20001/ui/wm/core/focus_contro... File ui/wm/core/focus_controller.cc (right): https://codereview.chromium.org/2809073002/diff/20001/ui/wm/core/focus_contro... ui/wm/core/focus_controller.cc:359: if (rules_->CanFocusWindow(GetToplevelWindow(window))) { On 2017/04/11 18:57:52, oshima wrote: > Can we just pass the event here? Done.
https://codereview.chromium.org/2809073002/diff/60001/ash/wm/ash_focus_rules.cc File ash/wm/ash_focus_rules.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ash/wm/ash_focus_rules.... ash/wm/ash_focus_rules.cc:89: if (event->IsMouseEvent()) don't we need to filter other events such as touch/pointer? https://codereview.chromium.org/2809073002/diff/60001/ash/wm/window_manager_u... File ash/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ash/wm/window_manager_u... ash/wm/window_manager_unittest.cc:378: generator.GestureTapAt(w2->bounds().CenterPoint()); We may want to include gesture. I'm to exclude it for now, but please ask yamaguchi@ if that's necessary. He may actually want to control the type of events to filter out. https://codereview.chromium.org/2809073002/diff/60001/ui/wm/core/focus_contro... File ui/wm/core/focus_controller.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ui/wm/core/focus_contro... ui/wm/core/focus_controller.cc:356: if (rules_->CanFocusWindow(GetToplevelWindow(window))) { sorry if it wasn't clear. I was suggesting to change CanFocusWindow(aura::Window*) to CanFocusWindow(window, event) as having two seems to be redundant.
What is the expectation of that property? That clicks never activate the window? Who is going to use this property? -Scott On Tue, Apr 11, 2017 at 11:19 AM, <warx@chromium.org> wrote: > On 2017/04/11 16:50:21, sky wrote: > > Could you outline where you are planning on going with this? > > The next step would be adding an app.currentWindowInternal. > setActivateOnClick > API. > That is defining SetActivateOnClick in NativeAppWindow and implement it > only in > ChromeNativeAppWindowViewsAuraAsh, where we get the native window and set > this > window property. > The other app window api procedure is pretty like this CL: > https://codereview.chromium.org/26427002. > > https://codereview.chromium.org/2809073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
What is the expectation of that property? That clicks never activate the window? Who is going to use this property? -Scott On Tue, Apr 11, 2017 at 11:19 AM, <warx@chromium.org> wrote: > On 2017/04/11 16:50:21, sky wrote: > > Could you outline where you are planning on going with this? > > The next step would be adding an app.currentWindowInternal. > setActivateOnClick > API. > That is defining SetActivateOnClick in NativeAppWindow and implement it > only in > ChromeNativeAppWindowViewsAuraAsh, where we get the native window and set > this > window property. > The other app window api procedure is pretty like this CL: > https://codereview.chromium.org/26427002. > > https://codereview.chromium.org/2809073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/11 21:00:05, sky wrote: > What is the expectation of that property? That clicks never activate the > window? Who is going to use this property? > > -Scott > > On Tue, Apr 11, 2017 at 11:19 AM, <mailto:warx@chromium.org> wrote: > > > On 2017/04/11 16:50:21, sky wrote: > > > Could you outline where you are planning on going with this? > > > > The next step would be adding an app.currentWindowInternal. > > setActivateOnClick > > API. > > That is defining SetActivateOnClick in NativeAppWindow and implement it > > only in > > ChromeNativeAppWindowViewsAuraAsh, where we get the native window and set > > this > > window property. > > The other app window api procedure is pretty like this CL: > > https://codereview.chromium.org/26427002. > > > > https://codereview.chromium.org/2809073002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Right now the only usage will be Files app window. The expectation would be if a click is from file dragging operation, do not set activation. But I know few about Files app. As oshima suggested, I will have a chat with yamaguchi@.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:90001) has been deleted
Patchset #4 (id:70001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for click events. Add a new API in FocusRules which is more general than click event as IsWindowConsideredActivatableForEvent and implement it in AshFocusRules. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ========== to ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ==========
Patchset #5 (id:130001) has been deleted
Patchset #4 (id:110001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi all, PTAL, thanks! https://codereview.chromium.org/2809073002/diff/60001/ash/wm/ash_focus_rules.cc File ash/wm/ash_focus_rules.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ash/wm/ash_focus_rules.... ash/wm/ash_focus_rules.cc:89: if (event->IsMouseEvent()) On 2017/04/11 20:54:40, oshima wrote: > don't we need to filter other events such as touch/pointer? > Touch event should be handled on OnGestureEvent. From the implementation in FocusController, now I filter IsMouseEvent and IsGestureEvent here. https://codereview.chromium.org/2809073002/diff/60001/ash/wm/window_manager_u... File ash/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ash/wm/window_manager_u... ash/wm/window_manager_unittest.cc:378: generator.GestureTapAt(w2->bounds().CenterPoint()); On 2017/04/11 20:54:40, oshima wrote: > We may want to include gesture. I'm to exclude it for now, but please ask > yamaguchi@ if that's necessary. He may actually want to control the type of > events to filter out. Touch dragging is needed. So I include the gesture event. https://codereview.chromium.org/2809073002/diff/60001/ui/wm/core/focus_contro... File ui/wm/core/focus_controller.cc (right): https://codereview.chromium.org/2809073002/diff/60001/ui/wm/core/focus_contro... ui/wm/core/focus_controller.cc:356: if (rules_->CanFocusWindow(GetToplevelWindow(window))) { On 2017/04/11 20:54:40, oshima wrote: > sorry if it wasn't clear. I was suggesting to change > CanFocusWindow(aura::Window*) > > to > > CanFocusWindow(window, event) > > as having two seems to be redundant. done. But I don't know which way is better compared with default parameter as CanFocusWindow(aura::Window*, ui::Event* = nullptr). It seems there are not many default parameter usage in chromium.
https://codereview.chromium.org/2809073002/diff/1/ui/aura/client/aura_constan... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/1/ui/aura/client/aura_constan... ui/aura/client/aura_constants.h:40: AURA_EXPORT extern const WindowProperty<bool>* const kActivateOnClickKey; I prefer the property here. That way clients don't need to use ash directly. Remember that for mus+ash only ash will use the code in src/ash, clients can use src/ash/public, but not files such as src/ash/wm/ash_focus_rules.h https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules.h File ash/wm/ash_focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:21: // Set whether |window| should be activated on pointer. On Chrome OS, window pointer -> point events https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:24: // activation rule for |window|. Document what the default is. https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); Can you use const ui::Event& in all these functions? That makes it clear the function shouldn't modify the value.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:170001) has been deleted
Updated, PTAL, thanks https://codereview.chromium.org/2809073002/diff/1/ui/aura/client/aura_constan... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/1/ui/aura/client/aura_constan... ui/aura/client/aura_constants.h:40: AURA_EXPORT extern const WindowProperty<bool>* const kActivateOnClickKey; On 2017/04/12 19:49:16, sky wrote: > I prefer the property here. That way clients don't need to use ash directly. > Remember that for mus+ash only ash will use the code in src/ash, clients can use > src/ash/public, but not files such as src/ash/wm/ash_focus_rules.h Done. https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules.h File ash/wm/ash_focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:21: // Set whether |window| should be activated on pointer. On Chrome OS, window On 2017/04/12 19:49:16, sky wrote: > pointer -> point events Done. https://codereview.chromium.org/2809073002/diff/150001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:24: // activation rule for |window|. On 2017/04/12 19:49:16, sky wrote: > Document what the default is. Done. https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); On 2017/04/12 19:49:16, sky wrote: > Can you use const ui::Event& in all these functions? That makes it clear the > function shouldn't modify the value. done. I create another API that is bool CanFocusWindow(aura::Window* window, const ui::Event& event) const; So that any call places with CanFocusWindow(window) can still be used.
Description was changed from ========== cros: allow aura window not considered activatable for click event Changes: Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ========== to ========== cros: allow aura window not considered activatable for pointer event Changes: Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ==========
https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); On 2017/04/12 23:00:42, Qiang(Joe) Xu wrote: > On 2017/04/12 19:49:16, sky wrote: > > Can you use const ui::Event& in all these functions? That makes it clear the > > function shouldn't modify the value. > > done. I create another API that is bool CanFocusWindow(aura::Window* window, > const ui::Event& event) const; > > So that any call places with CanFocusWindow(window) can still be used. Sorry, I gave you bad advice. I assumed the event was never null. If it can be null then go back to what you had (only one CanFocusWindow).
Return to ui::Event* now, PTAL, thanks! https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/150001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); On 2017/04/12 23:07:40, sky (OOO) wrote: > On 2017/04/12 23:00:42, Qiang(Joe) Xu wrote: > > On 2017/04/12 19:49:16, sky wrote: > > > Can you use const ui::Event& in all these functions? That makes it clear the > > > function shouldn't modify the value. > > > > done. I create another API that is bool CanFocusWindow(aura::Window* window, > > const ui::Event& event) const; > > > > So that any call places with CanFocusWindow(window) can still be used. > > Sorry, I gave you bad advice. I assumed the event was never null. If it can be > null then go back to what you had (only one CanFocusWindow). Done.
https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules.h File ash/wm/ash_focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:21: // Set whether |window| should be activated on point events. On Chrome OS, point -> pointer https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:25: static void SetActivateOnPointer(aura::Window* window, Remove this function and instead set property directly. https://codereview.chromium.org/2809073002/diff/210001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/210001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:40: // not. Document that default is true, which means windows are activated when a pointer down event occurs on them. https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules.h File ui/wm/core/focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules... ui/wm/core/focus_rules.h:33: // For CanFocusWindow(), nullptr window is supported, because nullptr is a optional: in comments I think null is fine. https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules... ui/wm/core/focus_rules.h:35: // |event| can be provided to check whether for the |event| the default focus How about: If |event| is non-null it is the event triggering the focus change.
Patchset #7 (id:230001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated based on comments, PTAL, thanks https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules.h File ash/wm/ash_focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:21: // Set whether |window| should be activated on point events. On Chrome OS, On 2017/04/17 16:08:47, sky wrote: > point -> pointer don by removing this. https://codereview.chromium.org/2809073002/diff/210001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.h:25: static void SetActivateOnPointer(aura::Window* window, On 2017/04/17 16:08:47, sky wrote: > Remove this function and instead set property directly. Done. https://codereview.chromium.org/2809073002/diff/210001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2809073002/diff/210001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:40: // not. On 2017/04/17 16:08:48, sky wrote: > Document that default is true, which means windows are activated when a pointer > down event occurs on them. Done. https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules.h File ui/wm/core/focus_rules.h (right): https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules... ui/wm/core/focus_rules.h:33: // For CanFocusWindow(), nullptr window is supported, because nullptr is a On 2017/04/17 16:08:48, sky wrote: > optional: in comments I think null is fine. Done. https://codereview.chromium.org/2809073002/diff/210001/ui/wm/core/focus_rules... ui/wm/core/focus_rules.h:35: // |event| can be provided to check whether for the |event| the default focus On 2017/04/17 16:08:48, sky wrote: > How about: > If |event| is non-null it is the event triggering the focus change. Done.
https://codereview.chromium.org/2809073002/diff/270001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/270001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); Sorry, one last question. Can you make this a const ui::Event* here and FocusRules?
PTAL, thanks https://codereview.chromium.org/2809073002/diff/270001/ui/wm/core/focus_contr... File ui/wm/core/focus_controller.h (right): https://codereview.chromium.org/2809073002/diff/270001/ui/wm/core/focus_contr... ui/wm/core/focus_controller.h:119: void WindowFocusedFromInputEvent(aura::Window* window, ui::Event* event); On 2017/04/17 18:24:38, sky wrote: > Sorry, one last question. Can you make this a const ui::Event* here and > FocusRules? yes, we can. Updated. Thanks.
LGTM - thanks
Hi oshima, would you also take a look?
On 2017/04/19 00:04:14, Qiang(Joe) Xu wrote: > Hi oshima, would you also take a look? oshima: friendly ping
https://codereview.chromium.org/2809073002/diff/290001/ash/wm/ash_focus_rules.cc File ash/wm/ash_focus_rules.cc (right): https://codereview.chromium.org/2809073002/diff/290001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.cc:79: if (event && (event->IsMouseEvent() || event->IsGestureEvent()) && We don't need to check touch event?
https://codereview.chromium.org/2809073002/diff/290001/ash/wm/ash_focus_rules.cc File ash/wm/ash_focus_rules.cc (right): https://codereview.chromium.org/2809073002/diff/290001/ash/wm/ash_focus_rules... ash/wm/ash_focus_rules.cc:79: if (event && (event->IsMouseEvent() || event->IsGestureEvent()) && On 2017/04/27 20:50:17, oshima wrote: > We don't need to check touch event? yes, touch event is actually handled in https://cs.chromium.org/chromium/src/ui/wm/core/focus_controller.cc?l=140, which is also why OnTouchEvent overridden body is empty.
thank you for clarification. lgtm
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 290001, "attempt_start_ts": 1493331653933630,
"parent_rev": "ff836a3e8ea62f70165a57dd5f0e831acaaca7d8", "commit_rev":
"e47fb498dce50d65d9c3b55fafdbffbe50ab6920"}
Message was sent while issue was closed.
Description was changed from ========== cros: allow aura window not considered activatable for pointer event Changes: Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage ========== to ========== cros: allow aura window not considered activatable for pointer event Changes: Add a window property for set/get whether an aura window is considered activatable for pointer window activation. Modify CanFocusWindow in FocusRules to allow optional check for ui::Event. This is a stage one implementation for crbug.com/657525. BUG=657525 TEST=added test coverage Review-Url: https://codereview.chromium.org/2809073002 Cr-Commit-Position: refs/heads/master@{#467849} Committed: https://chromium.googlesource.com/chromium/src/+/e47fb498dce50d65d9c3b55fafdb... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:290001) as https://chromium.googlesource.com/chromium/src/+/e47fb498dce50d65d9c3b55fafdb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
