New issue
Advanced search Search tips

Issue 876510 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Team-Accessibility



Sign in to add a comment

CrOS Shortcut Viewer a11y: full screen + docked magnifier not following keyboard focus

Project Member Reported by leberly@chromium.org, Aug 21

Issue description

70.0.3524.2 (Official Build) canary (64-bit)
Google_Eve.9584.160.0

Steps to repro:
# Enable full screen magnifier using search + ctrl + m
# Open Shortcuts using ctrl + alt + /
# Make sure magnification is high enough such that the search bar and the category tabs aren't on the screen at the same time. You can do that with ctrl + alt + brightness up or just change the screen resolution per this document: https://support.google.com/chromebook/answer/6320705?hl=en

# Press tab to move back and forth between the two areas that get keyboard focus 
Expected: magnifier moves area magnified to match keyboard focus
Actual: magnifier doesn't track keyboard focus

Doesn't repro on other apps such as the help app
 
Summary: CrOS Shortcut Viewer a11y: full screen + docked magnifier not following keyboard focus (was: CrOS Shortcut Viewer a11y: full screen magnifier not following keyboard focus)
Applies to both fullscreen and docked magnifiers. 
Checking again with flag disabled: 

70.0.3524.2 (Official Build) canary (64-bit)
Google_Eve.9584.160.0
#ash-keyboard-shortcut-viewer-app set to 'Disabled' 

It is improved but there is still a problem:
If you tab to the categories on the left, it moves the magnifier. However, if you tab back up to the search box, focus doesn't follow unless you type something in the box. Keeping bug open.  
Labels: Proj-Mash-KSV
Labels: -Proj-Mash-KSV
Sorry for the confusion, "mash" means the app version. If it still repros with the app disabled, it's not a "mash" problem.
Owner: afakhry@chromium.org
Status: Assigned (was: Available)
Cc: msw@chromium.org jamescook@chromium.org wutao@chromium.org lpalmaro@chromium.org
James, focus changes aren't followed in the magnifiers with the app-version of the KSH. I guess this means that `AshFocusManagerFactory::Delegate::OnDidChangeFocus()` [1] doesn't get called for stand alone apps.

How can we observe focus changes system-wide?

[1]: https://cs.chromium.org/chromium/src/ash/accelerators/ash_focus_manager_factory.cc?type=cs&q=AshFocusManagerFactory::Delegate::OnDidChangeFocus&g=0&l=39
Labels: Proj-Mash-KSV
The app (running in a separate process) will have a separate FocusManager instance.
It seems like we may need each individual FocusManager to ping the accessibility magnifier over mojo when focus changes.
This does seem like a Mash issue.
Cc: dmazz...@chromium.org afakhry@chromium.org
Owner: zork@chromium.org
Hey Zach, could someone from the accessibility team could take over here? I'd ping Dominic, but he's ooo.
James or I can review or help with design. We could take over, but I'm hoping a11y has cycles for this.

One possible solution is adding a per-app-process views::FocusChangeListener so each views app, like KSV, reports its focus changes (and focused view bounds) to the magnifier system. It would be somewhat similar to ash/accessibility/focus_ring_controller.*
Magnification is on our roadmap for 2019, but I don't see us having cycles before then.
Labels: a11y-q2-18
Google Chrome	72.0.3593.0 (Official Build) dev (64-bit)
Firmware Version	Google_Caroline.7820.384.0

Re-testing as part of our assessment. Since it's been two milestones, I did not try changing the flag and just tested the UI out of the box. If that's an issue, please just let me know. 

Both issues still reproduce per steps above. 
Labels: a11y-LocalApps
Cc: zork@chromium.org
Labels: Proj-Mash-SingleProcess
Owner: msw@chromium.org
This also affects SingleProcessMash, I might be able to start looking soon.
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48cb8d08fcb450c698a06a391ea7597ca7cabfda

commit 48cb8d08fcb450c698a06a391ea7597ca7cabfda
Author: Mike Wasserman <msw@chromium.org>
Date: Wed Dec 19 16:47:41 2018

mash: Fix a11y magnifier following View focus in SingleProcessMash

Install a delegate with all FocusManagerFactory instances for Ash.
Remove |desktop_widget| ( for moot Win8 Ash  http://crbug.com/246821  )

Bug:  876510 
Test: Magnifier follows view focus in single process mash.
Change-Id: I90c0de2676cf4f00464771553b0f34251686456b
Reviewed-on: https://chromium-review.googlesource.com/c/1382505
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617856}
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ash/accelerators/ash_focus_manager_factory.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ash/accelerators/ash_focus_manager_factory.h
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/cocoa/bridged_native_widget_unittest.mm
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/focus/focus_manager_factory.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/focus/focus_manager_factory.h
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/focus/focus_manager_unittest.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/mus/desktop_window_tree_host_mus.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/native_widget_aura.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/native_widget_delegate.h
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/native_widget_mac.mm
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/widget.cc
[modify] https://crrev.com/48cb8d08fcb450c698a06a391ea7597ca7cabfda/ui/views/widget/widget.h

Status: Started (was: Assigned)
The magnifier now follows View focus on SingleProcessMash, but:
1) The magnifier does not follow text caret movement while typing in SingleProcessMash
2) We'll need a View focus fix for multi-process Mash.
Status: Fixed (was: Started)
This is fixed for SingleProcessMash after https://chromium-review.googlesource.com/c/1387669 (I marked the wrong bug...)
Status: Started (was: Fixed)
My SPM caret cl fixed the fullscreen magnifier, but not the docked magnifier.
Project Member

Comment 18 by bugdroid1@chromium.org, Jan 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15f20f48842d18ebd868891cd44a7a7e34864713

commit 15f20f48842d18ebd868891cd44a7a7e34864713
Author: Mike Wasserman <msw@chromium.org>
Date: Tue Jan 08 01:27:04 2019

mash: Fix a11y docked magnifier following caret bounds

DockedMagnifierController observes IMEBridge like MagnifierController:
  https://chromium-review.googlesource.com/c/chromium/src/+/1387669

Bug:  876510 
Test: Docked magnifier follows caret bounds while typing in SPM, KSV.
Change-Id: I9f7b0ca4b3c7f8faf357124e26c519b95e73a677
Reviewed-on: https://chromium-review.googlesource.com/c/1399285
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620566}
[modify] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/BUILD.gn
[modify] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/docked_magnifier_controller.cc
[modify] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/docked_magnifier_controller.h
[modify] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/magnification_controller_unittest.cc
[rename] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/magnifier_utils.cc
[rename] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/magnifier_utils.h
[rename] https://crrev.com/15f20f48842d18ebd868891cd44a7a7e34864713/ash/magnifier/magnifier_utils_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment