Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361993003:
Draw underlay behind android apps using talkback (Closed)
DescriptionDraw underlay behind android apps using talkback
When spoken-feedback is enabled, shell_surface's shadow_underlay will
fill the screen for active android windows. When in this mode, the
underlay will consume all events and play an earcon to indicate that
the event was located outside the android window.
BUG=649524
TEST=unit-test and tested on device
Committed: https://crrev.com/9f31b7767effd9ea9e17f07820b94fe895687dea
Cr-Commit-Position: refs/heads/master@{#422651}
Patch Set 1 : Draw underlay behind android apps using talkback #
Total comments: 4
Patch Set 2 : fixed bug, added more to unit-test, addressed comment #Patch Set 3 : Properly show shadow on app launch #
Total comments: 22
Patch Set 4 : Addressed some CL comments and added some comments #
Total comments: 12
Patch Set 5 : More CL comments #Patch Set 6 : Resolved merge conflicts #
Total comments: 7
Patch Set 7 : CL comments #
Total comments: 8
Patch Set 8 : underlay always handles events #
Dependent Patchsets: Messages
Total messages: 62 (39 generated)
The CQ bit was checked by erosky@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 checked by erosky@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 checked by erosky@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
erosky@chromium.org changed reviewers: + hshi@chromium.org, oshima@chromium.org, reveman@chromium.org, skuhne@chromium.org
corresponds to b/31440313
https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:112: .Intersects(gfx::Rect(shadow_underlay_->layer()->size())); This feels convoluted. It seems equivalent to the simpler form: gfx::Rect(shadow_underlay_->layer()->size()).Contains(local_point) https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:130: if (!surface->HitTestRect(gfx::Rect(local_point, gfx::Size(1, 1)))) ditto. Why can't we just use HitTestPoint(local_point) here?
The CQ bit was checked by erosky@chromium.org to run a CQ dry run
https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:112: .Intersects(gfx::Rect(shadow_underlay_->layer()->size())); On 2016/09/23 21:29:05, hshi1 wrote: > This feels convoluted. It seems equivalent to the simpler form: > > gfx::Rect(shadow_underlay_->layer()->size()).Contains(local_point) Done. https://codereview.chromium.org/2361993003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:130: if (!surface->HitTestRect(gfx::Rect(local_point, gfx::Size(1, 1)))) On 2016/09/23 21:29:05, hshi1 wrote: > ditto. Why can't we just use HitTestPoint(local_point) here? Surface doesn't have a hittest for just Point.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
The CQ bit was checked by erosky@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.
lgtm! Thanks for adding the unit test! https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:710: pt = gfx::Point(250, 250); You might want to create a new point "pt2" here.
Before I review the code, can you explain more why we want this specifically for android apps and not for regular chrome apps? I looked at the bug but couldn't find any details.
On 2016/09/27 09:57:19, reveman wrote: > Before I review the code, can you explain more why we want this specifically for > android apps and not for regular chrome apps? I looked at the bug but couldn't > find any details. The goal of this is to notify the user when they touch outside the window boundary. Currently, if you start a drag outside the window and move your finger over the window, talkback won't activate because the android window doesn't receive the events (chrome has given focus to the down target). As a result, a user relying on talkback can get lost. The desired short-term solution (for now) would be to maximize ARC windows when talkback is on so that they could get all the events. Unfortunately, most ARC apps can't maximize/resize by default; so the least we can do is make a sound if the user missed the window. Additionally, we have to consume all the outside events: chromeos-side touch-exploration is disabled while an ARC app is active, so outside events could activate other windows. Regarding "spoken-feedback = touchview" only for ARC apps: I'm not sure what the reasoning is for that, I think it's the closest to "maximized" we can get without actually maximizing. I'll update the bug with some of this context.
https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:94: class CustomWindowTargeter : public aura::WindowTargeter { Does this really need to know about the shadow underlay? Can we instead pass widget as ctor argument and just fallback to |window| as target if spoken feedback is enabled, widget is active and ash::wm::GetWindowState(window)->allow_set_bounds_in_maximized() is true? And maybe move these checks into a helper function so you can also use it below.. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:140: aura::Window* shadow_underlay_; nit: blank line before DISALLOW_.. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:337: ash::WmShell::Get()->system_tray_notifier()) { Do we have to check ash::WmShell::HasInstance() && ash::WmShell::Get()->system_tray_notifier() here? Why are they null when this is called? https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:878: if (event->IsLocatedEvent() && event->target() == shadow_underlay_ && Same here. Can this use "event->target() == widget_->GetNativeWindow() && HelperIsActiveCannotChangeSizeAndSpokenFeedbackIsEnabled" instead of relying on the shadow underlay? https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:881: ui::EventType sound_types[] = { nit: s/ui::EventType sound_types/const ui::EventType kEarconEventTypes/ https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:886: for (ui::EventType e : sound_types) { nit: I think this would be a good place to use std::find instead of a loop to make the code a bit easier to read. E.g. bool is_earcon_event_type = std::find(std::begin(kEarconEventTypes), std::end(kEarconEventTypes), event->type()) != std::end(kEarconEventTypes); if (is_earcon_event_type) { ... } https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1340: // 1) the window is in immersive fullscreen or spoken_feedback/talkback nit: s/spoken_feedback/spoken feedback/ and remove reference to "talkback" as this code is not arc specific https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1346: if ((widget_->IsFullscreen() || underlay_capture_events) && use helper function? https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1348: window->layer()->GetTargetTransform().IsIdentity()) { is this needed as part of this patch? https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.h:308: bool audio_feedback_ = false; Does this state need to be cached here? Can we just use ash::WmShell::Get()->accessibility_delegate()->IsSpokenFeedbackEnabled() directly where needed instead?
The CQ bit was checked by erosky@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...
I added some comments to clarify what's going on. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:94: class CustomWindowTargeter : public aura::WindowTargeter { On 2016/09/28 16:11:50, reveman wrote: > Does this really need to know about the shadow underlay? Can we instead pass > widget as ctor argument and just fallback to |window| as target if spoken > feedback is enabled, widget is active and > ash::wm::GetWindowState(window)->allow_set_bounds_in_maximized() is true? And > maybe move these checks into a helper function so you can also use it below.. This still needs to know about the shadow_underlay_ in order to return it as an alternate event target (in FindTargetForEvent) (it'd be a child of |window|, among others, so finding it from just |window| wouldn't be trivial). https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:140: aura::Window* shadow_underlay_; On 2016/09/28 16:11:51, reveman wrote: > nit: blank line before DISALLOW_.. Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:337: ash::WmShell::Get()->system_tray_notifier()) { On 2016/09/28 16:11:50, reveman wrote: > Do we have to check ash::WmShell::HasInstance() && > ash::WmShell::Get()->system_tray_notifier() here? Why are they null when this is > called? Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:878: if (event->IsLocatedEvent() && event->target() == shadow_underlay_ && On 2016/09/28 16:11:50, reveman wrote: > Same here. Can this use "event->target() == widget_->GetNativeWindow() && > HelperIsActiveCannotChangeSizeAndSpokenFeedbackIsEnabled" instead of relying on > the shadow underlay? widget_->GetNativeWindow() represents the actual app which means we wouldn't want to intercept the event (event would probably not even get here since it'd be taken by Touch or Pointer. This condition is only to catch if we've touched outside the window (on the shadow_underlay). Checking for IsActive() here is unnecessary because the shadow_underlay is only "visible" when the widget is active. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:881: ui::EventType sound_types[] = { On 2016/09/28 16:11:51, reveman wrote: > nit: s/ui::EventType sound_types/const ui::EventType kEarconEventTypes/ Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:886: for (ui::EventType e : sound_types) { On 2016/09/28 16:11:50, reveman wrote: > nit: I think this would be a good place to use std::find instead of a loop to > make the code a bit easier to read. E.g. > > bool is_earcon_event_type = std::find(std::begin(kEarconEventTypes), > > std::end(kEarconEventTypes), > event->type()) != > std::end(kEarconEventTypes); > if (is_earcon_event_type) { > ... > } Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1340: // 1) the window is in immersive fullscreen or spoken_feedback/talkback On 2016/09/28 16:11:50, reveman wrote: > nit: s/spoken_feedback/spoken feedback/ and remove reference to "talkback" as > this code is not arc specific Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1346: if ((widget_->IsFullscreen() || underlay_capture_events) && On 2016/09/28 16:11:50, reveman wrote: > use helper function? This is the only place where the check happens, so I'm not sure breaking it into a helper would be cleaner. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.cc:1348: window->layer()->GetTargetTransform().IsIdentity()) { On 2016/09/28 16:11:50, reveman wrote: > is this needed as part of this patch? Yeah, otherwise the shadow won't initially show-up if the app is started while chromevox is already on. This happened because android doesn't know about the animation and therefore doesn't commit the surface after the animation finishes. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface.h:308: bool audio_feedback_ = false; On 2016/09/28 16:11:51, reveman wrote: > Does this state need to be cached here? Can we just use > ash::WmShell::Get()->accessibility_delegate()->IsSpokenFeedbackEnabled() > directly where needed instead? Done. https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2361993003/diff/80001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:710: pt = gfx::Point(250, 250); On 2016/09/27 06:40:59, Mr4D wrote: > You might want to create a new point "pt2" here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:100: if (window->parent()) where did this check go? can you instead keep this code as is and just add the shadow_underlay_ check below but before the surface hit test? https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:124: ui::EventTarget* FindTargetForEvent(ui::EventTarget* root, Why do we need to override this? What would it take to have the default implementation return the right thing? https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:146: aura::Window* shadow_underlay_; Instead of tracking this state here and having to worry about it potentially getting out of sync and doing the static_casts to set it. Can you just rename ShellSurface::shadow_underlay_for_test to ShellSurface::shadow_underlay and use that to get the shadow underlay? You can replace this with "views::Widget* const widget_;" and initialize it in the ctor in a way that's consistent with CustomFrameView above. And the following static_cast can be used to get the shell surface from the widget: static_cast<ShellSurface*>(widget_->widget_delegate()->GetContentsView()) https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:883: // it (usually when in spoken_feedback mode). Handle the event (to prevent nit: s/spoken_feedback/spoken feedback/ https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:888: const ui::EventType kEarconEventTypes[] = { nit: are these elements sorted in a way that I'm failing to see? maybe sort them by EventType enum order or alphabetically if not https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:1323: shadow_underlay_->SetTargetHandler(this); Can we use a different aura::WindowDelegate for this instead? ie. add a UnderlayWindowDelegate class above to keep the event handling separate from the native window?
The CQ bit was checked by erosky@chromium.org to run a CQ dry run
https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:100: if (window->parent()) On 2016/09/30 05:59:10, reveman wrote: > where did this check go? can you instead keep this code as is and just add the > shadow_underlay_ check below but before the surface hit test? ConvertPointToTarget already does the check, but I'll put it back to reduce changes. https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:124: ui::EventTarget* FindTargetForEvent(ui::EventTarget* root, On 2016/09/30 05:59:10, reveman wrote: > Why do we need to override this? What would it take to have the default > implementation return the right thing? The default implementation clips the event at the window bounds. We're explicitly looking for events outside the window bounds here (shadow_surface_ is a child window which is larger than its parent) https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:146: aura::Window* shadow_underlay_; On 2016/09/30 05:59:10, reveman wrote: > Instead of tracking this state here and having to worry about it potentially > getting out of sync and doing the static_casts to set it. Can you just rename > ShellSurface::shadow_underlay_for_test to ShellSurface::shadow_underlay and use > that to get the shadow underlay? > > You can replace this with "views::Widget* const widget_;" and initialize it in > the ctor in a way that's consistent with CustomFrameView above. And the > following static_cast can be used to get the shell surface from the widget: > > static_cast<ShellSurface*>(widget_->widget_delegate()->GetContentsView()) It can't get out of sync because there is a 1-to-1-to-1 relationship between ShellSurface, CustomWidgetDelegate, and shadow_underlay_. I'll do what you suggest though. It seems like a roundabout way to get shadow_underlay_, but it is more consistent with the other classes. https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:883: // it (usually when in spoken_feedback mode). Handle the event (to prevent On 2016/09/30 05:59:10, reveman wrote: > nit: s/spoken_feedback/spoken feedback/ Done. https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:888: const ui::EventType kEarconEventTypes[] = { On 2016/09/30 05:59:10, reveman wrote: > nit: are these elements sorted in a way that I'm failing to see? maybe sort them > by EventType enum order or alphabetically if not Done. https://codereview.chromium.org/2361993003/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:1323: shadow_underlay_->SetTargetHandler(this); On 2016/09/30 05:59:10, reveman wrote: > Can we use a different aura::WindowDelegate for this instead? ie. add a > UnderlayWindowDelegate class above to keep the event handling separate from the > native window? Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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...)
The CQ bit was checked by erosky@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...
lgtm with some more nits https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:117: aura::Window::ConvertPointToTarget(window->parent(), shadow_underlay, It's confusing that one piece of code assumes window->parent() is non-null and one piece (below) does not. Please make it consistent. If you change the code below then please reduce it to one ConvertPointToTarget call. https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:130: ui::EventTarget* FindTargetForEvent(ui::EventTarget* root, FYI, sub-surface can have bounds outside the surface so this is broken for that case but it was broken before too so no need to fix as part of this patch. https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:152: views::Widget* widget_; nit: views::Widget* const widget_; https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.h:298: std::unique_ptr<ui::EventHandler> shadow_underlay_handler_; nit: keep "event" in the name, shadow_underlay_event_handler_ or underlay_event_handler_
The CQ bit was checked by erosky@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...
https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:117: aura::Window::ConvertPointToTarget(window->parent(), shadow_underlay, On 2016/09/30 20:51:10, reveman wrote: > It's confusing that one piece of code assumes window->parent() is non-null and > one piece (below) does not. Please make it consistent. If you change the code > below then please reduce it to one ConvertPointToTarget call. Done. https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:130: ui::EventTarget* FindTargetForEvent(ui::EventTarget* root, On 2016/09/30 20:51:10, reveman wrote: > FYI, sub-surface can have bounds outside the surface so this is broken for that > case but it was broken before too so no need to fix as part of this patch. Acknowledged. https://codereview.chromium.org/2361993003/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:152: views::Widget* widget_; On 2016/09/30 20:51:10, reveman wrote: > nit: views::Widget* const widget_; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:44: #if defined(OS_CHROMEOS) Do you need this? I believe this file is compiled only on ChromeOS. https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); I think shadow underlay should always consume events so that touching there shouldn't activate or do something on other windows because they're not visible.
https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:44: #if defined(OS_CHROMEOS) On 2016/10/01 at 02:00:09, oshima wrote: > Do you need this? I believe this file is compiled only on ChromeOS. Compiling this only on ChromeOS is by choice and technically not something we're limited by. Initially this code was not only compiled on ChromeOS and that might change again in the future again. I'd like to keep these ifdefs that are consistent with the USE_OZONE ifdefs in display.cc. https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); On 2016/10/01 at 02:00:10, oshima wrote: > I think shadow underlay should always consume events so that touching there shouldn't activate or do something on other windows because they're not visible. +1
See my comment - but please wait for Oshima to lgtm this as well. https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); The purpose of this is that blind people can touch the screen anywhere (even outside of the window) and still get a system reaction. As such the shadow underlay should react to touch (it's an accessibility feature and we cannot simply full screen M apps since they might die).
https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); On 2016/10/01 02:00:10, oshima wrote: > I think shadow underlay should always consume events so that touching there > shouldn't activate or do something on other windows because they're not visible. Doing that would include things like immersive-mode portrait. It makes sense, to me, in that case as-well, but just want to make sure that's what you meant.
https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); On 2016/10/03 17:44:02, erosky wrote: > On 2016/10/01 02:00:10, oshima wrote: > > I think shadow underlay should always consume events so that touching there > > shouldn't activate or do something on other windows because they're not > visible. > > Doing that would include things like immersive-mode portrait. It makes sense, to > me, in that case as-well, but just want to make sure that's what you meant. Yes, we want this for both cases.
https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2361993003/diff/160001/components/exo/shell_s... components/exo/shell_surface.cc:1358: shadow_underlay_->set_ignore_events(!underlay_capture_events); On 2016/10/03 19:57:01, oshima wrote: > On 2016/10/03 17:44:02, erosky wrote: > > On 2016/10/01 02:00:10, oshima wrote: > > > I think shadow underlay should always consume events so that touching there > > > shouldn't activate or do something on other windows because they're not > > visible. > > > > Doing that would include things like immersive-mode portrait. It makes sense, > to > > me, in that case as-well, but just want to make sure that's what you meant. > > > Yes, we want this for both cases. Done.
The CQ bit was checked by erosky@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...
lgtm
The CQ bit was unchecked by erosky@chromium.org
The CQ bit was checked by erosky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hshi@chromium.org, reveman@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/2361993003/#ps180001 (title: "underlay always handles events")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Draw underlay behind android apps using talkback When spoken-feedback is enabled, shell_surface's shadow_underlay will fill the screen for active android windows. When in this mode, the underlay will consume all events and play an earcon to indicate that the event was located outside the android window. BUG=649524 TEST=unit-test and tested on device ========== to ========== Draw underlay behind android apps using talkback When spoken-feedback is enabled, shell_surface's shadow_underlay will fill the screen for active android windows. When in this mode, the underlay will consume all events and play an earcon to indicate that the event was located outside the android window. BUG=649524 TEST=unit-test and tested on device Committed: https://crrev.com/9f31b7767effd9ea9e17f07820b94fe895687dea Cr-Commit-Position: refs/heads/master@{#422651} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9f31b7767effd9ea9e17f07820b94fe895687dea Cr-Commit-Position: refs/heads/master@{#422651} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
