Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919213002:
Share DialogDelegateView:: and BubbleDialogDelegateView::'s impls of (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 7 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShare DialogDelegateView:: and BubbleDialogDelegateView::'s impls of
GetAccessibleState.
BUG=none
Committed: https://crrev.com/b436ef13f84d43abf54ea11d2def64d1395324e3
Cr-Commit-Position: refs/heads/master@{#390458}
Patch Set 1 #
Messages
Total messages: 19 (7 generated)
estade@chromium.org changed reviewers: + dtseng@chromium.org
Another question: why does DialogDelegateView need GetAccessibleWindowRole() in addition to GetAccessibleState()? Can we just implement GetAccessibleWindowTitle() and delete GetAccessibleState()?
estade@chromium.org changed reviewers: + dmazzoni@chromium.org
+dmazzoni since dtseng is OOO
lgtm > Another question: why does DialogDelegateView need GetAccessibleWindowRole() in > addition to GetAccessibleState()? Can we just implement > GetAccessibleWindowTitle() and delete GetAccessibleState()? I think it's DialogDelegate that has GetAccessibleWindowRole and not DialogDelegateView, right? The reason is because we don't create separate accessibility nodes for the window and the root view, we just let the root view own the native accessibility object and it gets its role (either "window" or "dialog", typically) from its window via that delegate method.
On 2016/04/27 23:04:27, dmazzoni wrote: > lgtm > > > Another question: why does DialogDelegateView need GetAccessibleWindowRole() > in > > addition to GetAccessibleState()? Can we just implement > > GetAccessibleWindowTitle() and delete GetAccessibleState()? > > I think it's DialogDelegate that has GetAccessibleWindowRole and not > DialogDelegateView, right? The reason is because we don't create separate > accessibility nodes for the window and the root view, we just let the root view > own the native accessibility object and it gets its role (either "window" or > "dialog", typically) from its window via that delegate method. DialogDelegateView is a DialogDelegate, so my question would be why it needs GetAccessibleState() when its parent already implements GetAccessibleWindowRole.
On 2016/04/27 23:15:09, Evan Stade wrote: > On 2016/04/27 23:04:27, dmazzoni wrote: > > lgtm > > > > > Another question: why does DialogDelegateView need GetAccessibleWindowRole() > > in > > > addition to GetAccessibleState()? Can we just implement > > > GetAccessibleWindowTitle() and delete GetAccessibleState()? > > > > I think it's DialogDelegate that has GetAccessibleWindowRole and not > > DialogDelegateView, right? The reason is because we don't create separate > > accessibility nodes for the window and the root view, we just let the root > view > > own the native accessibility object and it gets its role (either "window" or > > "dialog", typically) from its window via that delegate method. > > DialogDelegateView is a DialogDelegate, so my question would be why it needs > GetAccessibleState() when its parent already implements GetAccessibleWindowRole. Yes, it's possible that DialogDelegateView::GetAccessibleState is redundant. Possibly it wasn't when it was first added. Looking again, though, it's possible that DialogDelegateView::ViewHierarchyChanged would have to be modified to walk up the parent chain and find the ancestor that's a dialog to fire the notification on.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@chromium.org changed reviewers: + msw@chromium.org
+msw for owners
lgtm
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919213002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b436ef13f84d43abf54ea11d2def64d1395324e3 Cr-Commit-Position: refs/heads/master@{#390458}
Message was sent while issue was closed.
Description was changed from ========== Share DialogDelegateView:: and BubbleDialogDelegateView::'s impls of GetAccessibleState. BUG=none ========== to ========== Share DialogDelegateView:: and BubbleDialogDelegateView::'s impls of GetAccessibleState. BUG=none Committed: https://crrev.com/b436ef13f84d43abf54ea11d2def64d1395324e3 Cr-Commit-Position: refs/heads/master@{#390458} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
