Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530073002:
Add kAccessibilityFocusFallsbackToWidget property (Closed)
|
Created:
4 years ago by yawano Modified:
4 years ago CC:
reveman, aboxhall+watch_chromium.org, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, je_julie, msw+watch_chromium.org, nektar+watch_chromium.org, rouslan+bubble_chromium.org, sky, tfarina, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
Committed: https://crrev.com/7f0cf5b355d668d98b8097e850eb33a9b86a727a
Cr-Commit-Position: refs/heads/master@{#437469}
Patch Set 1 #Patch Set 2 : Change test cases #
Total comments: 4
Patch Set 3 : Address comments. #Patch Set 4 : Remove unnecessary blank line. #
Total comments: 6
Patch Set 5 : Add comments and use DCHECK #Patch Set 6 : Fix condition in DCHECK #
Total comments: 2
Patch Set 7 : Check whether the window is arc or not #
Total comments: 7
Patch Set 8 : Add kAccessibilityFocusFallsbackToWidgetKey #Patch Set 9 : Fix AxAuraObjCache #
Total comments: 2
Patch Set 10 : Fix style. #
Total comments: 2
Patch Set 11 : Fix a nit #
Messages
Total messages: 78 (42 generated)
The CQ bit was checked by yawano@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yawano@chromium.org changed reviewers: + dmazzoni@chromium.org, msw@chromium.org
dmazzoni@: PTAL chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc and ui/views/accessibility/ax_aura_obj_cache.cc Especially, please take a look whether changes in spoken_feedback_browsertest.cc are okay or not. msw@: PTAL ui/views/bubble/bubble_dialog_delegate.cc Thank you!
Description was changed from
==========
Make root view of BubbleWidget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of BubbleWidget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
==========
to
==========
Make root view of BubbleWidget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of BubbleWidget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
The CQ bit was checked by yawano@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.
Rather than marking the bubble dialog root view as accessibility focusable, would it be possible to mark the Exo window used for Arc++ as special in some way and not focus that one? I think Arc++ is really the special case here. In every other case it's probably better for us to consider the root view to be focused, not just for bubbles. https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:503: EXPECT_EQ(", window", speech_monitor_.GetNextUtterance()); Instead of adding this expectation, which may just break again soon, change the next line to a loop that loops until "Entered window overview mode" is matched. https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:538: EXPECT_EQ("webView", speech_monitor_.GetNextUtterance()); Same, let's get rid of this expectation and just loop until "Find in page" is heard
The CQ bit was checked by yawano@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 yawano@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...
yawano@chromium.org changed reviewers: + reveman@chromium.org, sky@chromium.org - msw@chromium.org
Reviewers: -msw@, +sky@, +reveman@ sky@: PTAL ui/views/widget/widget.cc. reveman@: PTAL components/exo/shell_surface.cc Thank you! dmazzoni@: Thank you for the review! How about to make root view of widget as accessibility focusable by default and make it of ShellSurfaceWidget non-focusable? I think this is more generic approach than adding isShellSurfaceWidget branch in AxAuraObjCache. https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:503: EXPECT_EQ(", window", speech_monitor_.GetNextUtterance()); On 2016/11/29 17:50:17, dmazzoni wrote: > Instead of adding this expectation, which may just > break again soon, change the next line to a loop > that loops until "Entered window overview mode" > is matched. Done. https://codereview.chromium.org/2530073002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:538: EXPECT_EQ("webView", speech_monitor_.GetNextUtterance()); On 2016/11/29 17:50:17, dmazzoni wrote: > Same, let's get rid of this expectation and just loop > until "Find in page" is heard Done.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2530073002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2530073002/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:334: root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); Can you clarify why you want this? The change here means *every* widget will have it's root view accessible. Many widgets are not activatable, or things you wouldn't want to have focus (say tooltips for example). Many bubbles also don't want focus. In other words this doesn't seem like the right change, but I could be misunderstanding what this does and why you want it.
https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1024: if (root_view != nullptr) { nit: s/root_view != nullptr/root_view/ when is this null? DCHECK instead? https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1025: root_view->SetFocusBehavior(views::View::FocusBehavior::NEVER); Can you explain in the description of this change why we want this for all exo shell surfaces? I'm failing to see what any of this has to do with BubbleWidgets as mentioned in the description..
On 2016/11/30 17:00:40, reveman wrote: > https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... > File components/exo/shell_surface.cc (right): > > https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... > components/exo/shell_surface.cc:1024: if (root_view != nullptr) { > nit: s/root_view != nullptr/root_view/ > > when is this null? DCHECK instead? > > https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... > components/exo/shell_surface.cc:1025: > root_view->SetFocusBehavior(views::View::FocusBehavior::NEVER); > Can you explain in the description of this change why we want this for all exo > shell surfaces? I'm failing to see what any of this has to do with BubbleWidgets > as mentioned in the description.. Yeah, the change description should say something about not ignoring the exo shell window for accessibility focus
Description was changed from
==========
Make root view of BubbleWidget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of BubbleWidget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Make root view of Widget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of Widget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
Description was changed from
==========
Make root view of Widget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of Widget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Make root view of widget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of widget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
Sorry, I forgot to update description. I've added comments in the code. More background about this change. We have a change which makes automation to falls back to root view if no view is focused. - https://crrev.com/2416103003 But this caused issue for Android window. If it puts focus on root view, root view of ShellSurfaceWidget becomes consuming key events. It means that Android window no longer receive them. I think it's better to fix in more generic way rather than adding isShellSurfaceWidget branch in AxAuraObjCache. But I saw a few tests becomes failing after I've changed root view of every widget accessibility focusable. This issue is regression from M56, and we want to merge this later. So I want to keep this CL small if possible. Might it be better to do quick fix by adding isShellSurfaceWidget in the cache? We will be able to explore better fix later. dmazzoni@, WDYT? https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1024: if (root_view != nullptr) { On 2016/11/30 17:00:40, reveman wrote: > nit: s/root_view != nullptr/root_view/ > > when is this null? DCHECK instead? Done. I don't come up case where it becomes null. DCHECK will be enough. https://codereview.chromium.org/2530073002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1025: root_view->SetFocusBehavior(views::View::FocusBehavior::NEVER); Added comment. https://codereview.chromium.org/2530073002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2530073002/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:334: root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); Added comment.
https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget... ui/views/widget/widget.cc:336: root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); Again, might this result in trying to focus widgets that we normally don't want focused?
components/exo/shell_surface.cc lgtm
https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget... ui/views/widget/widget.cc:336: root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); On 2016/12/01 00:36:59, sky wrote: > Again, might this result in trying to focus widgets that we normally don't want > focused? Yes.
On 2016/12/01 00:47:08, yawano wrote: > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget.cc > File ui/views/widget/widget.cc (right): > > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget... > ui/views/widget/widget.cc:336: > root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); > On 2016/12/01 00:36:59, sky wrote: > > Again, might this result in trying to focus widgets that we normally don't > want > > focused? > > Yes. You haven't motivated why you need this for *all* widgets, rather than just android windows. Can't the fix be in the android side?
On 2016/12/01 03:18:46, sky wrote: > On 2016/12/01 00:47:08, yawano wrote: > > > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget.cc > > File ui/views/widget/widget.cc (right): > > > > > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget... > > ui/views/widget/widget.cc:336: > > root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); > > On 2016/12/01 00:36:59, sky wrote: > > > Again, might this result in trying to focus widgets that we normally don't > > want > > > focused? > > > > Yes. > > You haven't motivated why you need this for *all* widgets, rather than just > android windows. Can't the fix be in the android side? No, this should be in Chrome side, IIUC. Chrome consumes key events before they are passed to Android. Root view of widget was non-focusable, but accessibility service has fallen back to the root view of widget if no view is focused. I think this change is to make FocusBehavior match with what accessibility service (or automation API) is actually doing. I understand making changes to all widgets will affect large part. If we don't make this for all widgets, we will add isShellSurfaceWidget branch in AxAuraObjCache. Handing it as exceptional case in AxAuraObjCache. I don't stick with current approach if there is a concern for making this change to all widgets especially as we want to merge this to M56 later.
On Wed, Nov 30, 2016 at 7:41 PM, <yawano@chromium.org> wrote: > On 2016/12/01 03:18:46, sky wrote: >> On 2016/12/01 00:47:08, yawano wrote: >> > >> > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget.cc >> > File ui/views/widget/widget.cc (right): >> > >> > >> > https://codereview.chromium.org/2530073002/diff/100001/ui/views/widget/widget... >> > ui/views/widget/widget.cc:336: >> > root_view_->SetFocusBehavior(View::FocusBehavior::ACCESSIBLE_ONLY); >> > On 2016/12/01 00:36:59, sky wrote: >> > > Again, might this result in trying to focus widgets that we normally >> > > don't >> > want >> > > focused? >> > >> > Yes. >> >> You haven't motivated why you need this for *all* widgets, rather than >> just >> android windows. Can't the fix be in the android side? > > No, this should be in Chrome side, IIUC. > Chrome consumes key events before they are passed to Android. > > Root view of widget was non-focusable, but accessibility service has fallen > back > to the root view of widget if no view is focused. > I think this change is to make FocusBehavior match with what accessibility > service (or automation API) is actually doing. Dominic, can you provide input here. Why is the accessibility service falling back to the rootview? Again, this is the wrong change for *all* widgets. I'm ok with a change for just android, but that shouldn't be in widget. -Scott > > I understand making changes to all widgets will affect large part. > If we don't make this for all widgets, we will add isShellSurfaceWidget > branch > in AxAuraObjCache. > Handing it as exceptional case in AxAuraObjCache. > I don't stick with current approach if there is a concern for making this > change > to all widgets especially as we want to merge this to M56 later. > > https://codereview.chromium.org/2530073002/ -- 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 2016/12/01 15:54:39, sky wrote: > > No, this should be in Chrome side, IIUC. > > Chrome consumes key events before they are passed to Android. > > > > Root view of widget was non-focusable, but accessibility service has fallen > > back > > to the root view of widget if no view is focused. > > I think this change is to make FocusBehavior match with what accessibility > > service (or automation API) is actually doing. > > Dominic, can you provide input here. Why is the accessibility service > falling back to the rootview? Again, this is the wrong change for > *all* widgets. I'm ok with a change for just android, but that > shouldn't be in widget. Here's my understanding. As background, AXAuraObjCache::GetFocusedView() is used to determine what View is focused, globally, for the purposes of Chrome OS accessibility. In r427103 (https://codereview.chromium.org/2416103003) I updated it to better handle the case where a window gets activated, but no View in that window has focus - this was happening for things like notification bubbles, for example. In that case we consider the root view of that widget to be focused - for the purposes of accessibility. It's essentially the same as saying the window has focus since we don't have accessibility objects for widgets or windows, we just use the root view. This broke ARC++ accessibility because we were relying on the fact that when an ARC++ window was active, we'd consider *no* View to be focused, which allows the ARC++ window to handle all of its own accessibility. (ChromeVox disables its key handlers and shuts up.) To fix it, we want to restore the old behavior for the Exo Shell window, in that AXAuraObjCache::GetFocusedView() should return null. But every other Widget should return its root view still. I don't think we have to solve this using SetFocusBehavior. We just want to add some tag to the Exo Shell window that allow us to distinguish it, and then check for that in AXAuraObjCache::GetFocusedView() so it can return null. We could have the root view override GetAccessibleNodeData and have it return INVISIBLE in the state flag, then have GetFocusedView ignore invisible views. That'd be pretty reasonable semantically. Alternatively there may be some way to put a tag on the Window, or as a hack just to check for the window's name directly.
On 2016/12/01 21:23:08, dmazzoni wrote: > On 2016/12/01 15:54:39, sky wrote: > > > No, this should be in Chrome side, IIUC. > > > Chrome consumes key events before they are passed to Android. > > > > > > Root view of widget was non-focusable, but accessibility service has fallen > > > back > > > to the root view of widget if no view is focused. > > > I think this change is to make FocusBehavior match with what accessibility > > > service (or automation API) is actually doing. > > > > Dominic, can you provide input here. Why is the accessibility service > > falling back to the rootview? Again, this is the wrong change for > > *all* widgets. I'm ok with a change for just android, but that > > shouldn't be in widget. > > Here's my understanding. > > As background, AXAuraObjCache::GetFocusedView() is used to determine what > View is focused, globally, for the purposes of Chrome OS accessibility. In > r427103 (https://codereview.chromium.org/2416103003) I updated it to > better handle the case where a window gets activated, but no View in that > window has focus - this was happening for things like notification bubbles, > for example. In that case we consider the root view of that widget to be > focused - for the purposes of accessibility. It's essentially the same as saying > the window has focus since we don't have accessibility objects for widgets or > windows, we just use the root view. > > This broke ARC++ accessibility because we were relying on the fact that > when an ARC++ window was active, we'd consider *no* View to be > focused, which allows the ARC++ window to handle all of its own accessibility. > (ChromeVox disables its key handlers and shuts up.) > > To fix it, we want to restore the old behavior for the Exo Shell window, in that > AXAuraObjCache::GetFocusedView() should return null. But every other > Widget should return its root view still. > > I don't think we have to solve this using SetFocusBehavior. > > We just want to add some tag to the Exo Shell window that allow us to > distinguish it, and then check for that in AXAuraObjCache::GetFocusedView() > so it can return null. > > We could have the root view override GetAccessibleNodeData and have it > return INVISIBLE in the state flag, then have GetFocusedView ignore > invisible views. That'd be pretty reasonable semantically. > > Alternatively there may be some way to put a tag on the Window, or as a hack > just to check for the window's name directly. Thank you! Marking the root view of ShellSurfaceWidget as invisible makes sense semantically. I'll change this CL in that way. If the change becomes big, I'll try another way (checking window name directly, etc) as we want to merge this to M56 later.
Description was changed from
==========
Make root view of widget accessibility focusable
- Changed AxAuraObjCache::GetFocusedView to falls back to accessibility
focusable root view if no view is focused in widget.
- Changed root view of widget accessibility focusable.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Do not return root view of widget as focused view if it's in arc window
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
The CQ bit was checked by yawano@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...
yawano@chromium.org changed reviewers: - reveman@chromium.org, sky@chromium.org
reveman@, sky@: moving from reviewers to CC. Changed to directly check whether the window is arc or not in AxAuraObjCache. Checking the window is arc or not same as we are doing in c/b/memory/tab_manager_delegate_chromeos.cc. I've first tried to set root view of AxNodeData as invisible, but it seems require larger change as Widget::CreateRootView returns internal::RootView, i.e. cannot put the logic in shell_surface.cc. dmazzoni@: PTAL. Thank you!
Description was changed from
==========
Do not return root view of widget as focused view if it's in arc window
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Do not return root view of widget as focused view if it's in arc window
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm This looks like a reasonable fix to merge, then it'd be nice to find a way to do this without sniffing the window class name. Hopefully that can be done as part of the suggested refactoring to tab_manager_delegate_chromeos.cc.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:31: if (!window || window->GetName() != kExoShellSurfaceWindowName) Why do you need to check the name *and* the kAppIdKey? Can't you just check kAppIdKey?
https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:31: if (!window || window->GetName() != kExoShellSurfaceWindowName) On 2016/12/02 17:49:56, sky wrote: > Why do you need to check the name *and* the kAppIdKey? Can't you just check > kAppIdKey? Also, can't you set an explicit property rather than having to do string comparison? I'm thinking a specific property to identify what you need.
https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:31: if (!window || window->GetName() != kExoShellSurfaceWindowName) On 2016/12/02 17:50:40, sky wrote: > On 2016/12/02 17:49:56, sky wrote: > > Why do you need to check the name *and* the kAppIdKey? Can't you just check > > kAppIdKey? > > Also, can't you set an explicit property rather than having to do string > comparison? I'm thinking a specific property to identify what you need. As far as I tested, checking only kAppIdKey would be enough for the current version of Chrome OS. But I think checking both name and kAppIdKey is better since it's more semantic. We want to consider the window which meets the both 2 conditions as special case here. - Window is using ExoShellSurface - Checking name - Window is for Arc - Checking application id My concern for only checking application id is that someone might add an arc window which doesn't use ExoShellSurface in the future. For specific property, does it mean to add new property or change value of an existing property? I think there should be a way to do it in the way. But, IMO, it's better if we don't need to have IsArcWindow branch in this file, i.e. this shouldn't be handled as an exceptional case.
Ok, makes sense
reveman@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:31: if (!window || window->GetName() != kExoShellSurfaceWindowName) On 2016/12/02 at 17:50:40, sky wrote: > On 2016/12/02 17:49:56, sky wrote: > > Why do you need to check the name *and* the kAppIdKey? Can't you just check > > kAppIdKey? > > Also, can't you set an explicit property rather than having to do string comparison? I'm thinking a specific property to identify what you need. We already do this. exo::ShellSurface::GetMainSurface can be used to determine if a window is an exo shell surface widget: https://cs.chromium.org/chromium/src/components/exo/shell_surface.h?l=174 https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:31: if (!window || window->GetName() != kExoShellSurfaceWindowName) On 2016/12/05 at 02:16:36, yawano wrote: > On 2016/12/02 17:50:40, sky wrote: > > On 2016/12/02 17:49:56, sky wrote: > > > Why do you need to check the name *and* the kAppIdKey? Can't you just check > > > kAppIdKey? > > > > Also, can't you set an explicit property rather than having to do string > > comparison? I'm thinking a specific property to identify what you need. > > As far as I tested, checking only kAppIdKey would be enough for the current version of Chrome OS. But I think checking both name and kAppIdKey is better since it's more semantic. > > We want to consider the window which meets the both 2 conditions as special case here. > > - Window is using ExoShellSurface - Checking name > - Window is for Arc - Checking application id > > My concern for only checking application id is that someone might add an arc window which doesn't use ExoShellSurface in the future. > > For specific property, does it mean to add new property or change value of an existing property? I think there should be a way to do it in the way. But, IMO, it's better if we don't need to have IsArcWindow branch in this file, i.e. this shouldn't be handled as an exceptional case. exo::ShellSurface::GetMainSurface can be used to more efficiently determine if a window is an exo shell surface widget: https://cs.chromium.org/chromium/src/components/exo/shell_surface.h?l=174 It think using that instead of a string compare is what sky@ asked for.
> exo::ShellSurface::GetMainSurface can be used to more efficiently determine if a > window is an exo shell surface widget: > https://cs.chromium.org/chromium/src/components/exo/shell_surface.h?l=174 > > It think using that instead of a string compare is what sky@ asked for. I don't think we can call exo::ShellSurface::GetMainSurface from ax_aura_obj_cache.cc. ax_aura_obj_cache.cc is under ui/views, and components/exo already has dependency to ui/views. We cannot have dependency from ui/views to components/exo.
https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:30: bool IsArcWindow(aura::Window* window) { Views shouldn't need to know about arc. Instead the arc code should set a property on the aura::Window to indicate this new behavior should be provided. Something like: widget->GetNativeView()->SetProperty(kAccessibilityFocusFallsbackToWidget, false);
Added kAccessibilityFocusFallsbackToWidget and changed AxAuraObjCache to check it. PTAL again as we've changed how to implement this. dmazzoni@: - chrome/browser/chromeos/accessibility - ui/views/accessibility sky@: ui/aura reveman: components/exo Thank you! https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/120001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:30: bool IsArcWindow(aura::Window* window) { Thank you! Seems good property for this behavior. Added kAccessibilityFocusFallsbackToWidget.
Description was changed from
==========
Do not return root view of widget as focused view if it's in arc window
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Add kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
Updated subject and description of this CL.
The CQ bit was checked by yawano@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
components/exo lgtm
LGTM - thanks!
The CQ bit was checked by yawano@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.
dmazzoni@: fixed ax_aura_obj_cache.cc. PTAL c/b/chromeos/accessibility and ui/views/accessibility. Thank you!
SLGTM https://codereview.chromium.org/2530073002/diff/160001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/160001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:170: } else { chromium style is no else after a return
Thank you! https://codereview.chromium.org/2530073002/diff/160001/ui/views/accessibility... File ui/views/accessibility/ax_aura_obj_cache.cc (right): https://codereview.chromium.org/2530073002/diff/160001/ui/views/accessibility... ui/views/accessibility/ax_aura_obj_cache.cc:170: } else { On 2016/12/08 16:07:18, sky wrote: > chromium style is no else after a return Done.
lgtm, thanks! https://codereview.chromium.org/2530073002/diff/180001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2530073002/diff/180001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:28: // A property key to store whether accessibility focus fallsback to widget or nit: "falls back" (two words)
Thank you! https://codereview.chromium.org/2530073002/diff/180001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2530073002/diff/180001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:28: // A property key to store whether accessibility focus fallsback to widget or On 2016/12/09 03:48:37, dmazzoni wrote: > nit: "falls back" (two words) Done.
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, sky@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2530073002/#ps200001 (title: "Fix a nit")
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": 200001, "attempt_start_ts": 1481255629484890,
"parent_rev": "3fb9d23a2fe0e6505ce01bfe4f7d0b0b89be26be", "commit_rev":
"3bec912b543461f16453a54d2a517dc10d94d800"}
Message was sent while issue was closed.
Description was changed from
==========
Add kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Add kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from
==========
Add kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
==========
to
==========
Add kAccessibilityFocusFallsbackToWidget property
- Added kAccessibilityFocusFallsbackToWidget property to aura::Window.
- Default value of kAccessibilityFocusFallsbackToWidget is true.
- kAccessibilityFocusFallsbackToWidget is set to false for arc window.
BUG=661061
TEST=manually tested as described in the issue; User can navigate
elements in Android window by using TalkBack.
interactive_ui_tests:SpokenFeedbackTest
Committed: https://crrev.com/7f0cf5b355d668d98b8097e850eb33a9b86a727a
Cr-Commit-Position: refs/heads/master@{#437469}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/7f0cf5b355d668d98b8097e850eb33a9b86a727a Cr-Commit-Position: refs/heads/master@{#437469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
