Add new permission dialog for accessibility events. [chromium/src : master]
Dominic Mazzoni (Gerrit)
@msramek, could you help me with next steps here or point me to
a better reviewer?
I'm hoping to add a new permission to support an experimental API
I'm working on implementing, the Accessibility Object Model.
One portion of the API allows the developer to add special
event listeners for accessibility events, but the spec requires that
the user grant permission before these event listeners are called,
because it potentially reveals sensitive information about the user.
This is my first attempt at an implementation, but please assume I
know nothing; I'd appreciate it if you could ask for clarification
where needed and guide me towards the right way to do things.
In particular, one question I have is about whether I should expose
the permission in PermissionDescriptor.idl. I think I may need to do
that so that I could grant the permission in a layout test (see the
failing test on the trybots), but my concern is that we don't want
to expose this permission if the Accessibility Object Model feature
is not enabled. Any thoughts?
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Martin Šrámek (Gerrit)
Martin Šrámek would like Raymes Khoury and Mounir Lamouri to review this change.
Add new permission dialog for accessibility events.
This is required as part of the Accessibility Object Model
phase 2 spec.
Bug: 746524
Change-Id: I5b63921663dee00e609ddfaf3aab5577229294c1
---
M chrome/app/generated_resources.grd
M chrome/browser/BUILD.gn
A chrome/browser/accessibility/accessibility_permission_context.cc
A chrome/browser/accessibility/accessibility_permission_context.h
M chrome/browser/permissions/permission_manager.cc
M chrome/browser/permissions/permission_request_impl.cc
M components/content_settings/core/browser/content_settings_registry.cc
M components/content_settings/core/common/content_settings.cc
M components/content_settings/core/common/content_settings_types.h
M components/vector_icons/BUILD.gn
A components/vector_icons/accessibility.icon
M content/browser/permissions/permission_service_impl.cc
M content/public/browser/permission_type.h
M third_party/WebKit/Source/modules/accessibility/AXObject.cpp
M third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
M third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h
M third_party/WebKit/public/platform/WebFeaturePolicyFeature.h
M third_party/WebKit/public/platform/modules/permissions/permission.mojom
18 files changed, 195 insertions(+), 4 deletions(-)
Martin Šrámek (Gerrit)
Hi Dominic!
I can help with the content settings layer where the permission decisions are stored.
From privacy perspective, I can also mention the requirement that all content settings need to be represented in chrome://settings/content (or the Site Settings menu on Android), so users can audit and delete them. Doesn't have to be in this CL, of course.
However, I don't know that much about the permissions layer which handles the interaction between a website, a permission prompt, and the content settings layer.
So I would forward you to Raymes and Mounir. Both seem to be OOO, but Mounir returns next week.
2 comments:
File components/content_settings/core/common/content_settings.cc:
Patch Set #1, Line 65:
CONTENT_SETTINGS_TYPE_ACCESSIBILITY_EVENTSYou'll want to add this entry here as well: https://cs.chromium.org/chromium/src/tools/metrics/histograms/enums.xml?q=ContentType
File components/content_settings/core/common/content_settings_types.h:
Patch Set #1, Line 95:
Websitenit: This is added to ContentSettingsRegistry, as opposed to WebsiteSettingsRegistry, so it's a content setting, not a website setting :) (The comment next to TYPE_SOUND is also wrong.)
This distinction arose quite late in the history of the codebase, so the nomenclature is not quite clear from the code, sorry for that.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
From privacy perspective, I can also mention the requirement that all content settings need to be represented in chrome://settings/content (or the Site Settings menu on Android), so users can audit and delete them. Doesn't have to be in this CL, of course.
Thanks! Since this is currently behind a Blink flag, do we need to add a
content-side flag for this feature so that we can only show that setting
if the feature is enabled, or maybe just query the Blink flag? I'm just
wondering if there are any best practices for content settings for
experimental features?
Martin Šrámek (Gerrit)
Patch Set 1:
From privacy perspective, I can also mention the requirement that all content settings need to be represented in chrome://settings/content (or the Site Settings menu on Android), so users can audit and delete them. Doesn't have to be in this CL, of course.
Thanks! Since this is currently behind a Blink flag, do we need to add a
content-side flag for this feature so that we can only show that setting
if the feature is enabled, or maybe just query the Blink flag? I'm just
wondering if there are any best practices for content settings for
experimental features?
That's a good question :) I personally am not aware of any best practices. I also think this is a more general question - how to hide a feature behind a flag if it spans all layers, from Blink to Chrome. I assume querying the Blink flag is most reasonable if possible.
Dominic Mazzoni (Gerrit)
+timloh
Timothy Loh (Gerrit)
Some quick thoughts about the CL below, although I need a bit more time to digest the permission itself since I haven't at all looked at accessibility stuff before now.
- You probably need to also add a PermissionRequestType and update PermissionUtil::GetRequestType(), GetPermissionRequestString(), and the histograms.xml file.
- Is this permission going to be supported on Android? If so you need to update GetIconId/GetMessageText on PermissionRequestImpl.
- msramek@ already mentioned it needs to be in site settings but do we also want to have it in Page Info?
- Have the icon and strings been reviewed? I think srahim@ is responsible for strings, I'm not sure about icons.
Dominic Mazzoni (Gerrit)
Ready for another look, feedback is addressed and
I have a test working now.
2 comments:
Patch Set #1, Line 65:
CONTENT_SETTINGS_TYPE_ACCESSIBILITY_EVENTS
You'll want to add this entry here as well: https://cs.chromium. […]
Done
File components/content_settings/core/common/content_settings_types.h:
Patch Set #1, Line 95:
Contentnit: This is added to ContentSettingsRegistry, as opposed to WebsiteSettingsRegistry, so it's a cont […]
Done
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
6 comments:
File chrome/browser/accessibility/accessibility_permission_context.cc:
Patch Set #7, Line 13:
AccessibilityPermissionContext::~AccessibilityPermissionContext() {}nit: `= default;`
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #7, Line 200:
bool CanCallAOMEventListeners();style: add `const`
Patch Set #7, Line 201:
// This is called when an accessibility event is triggered and there arestyle: leave empty line
Patch Set #7, Line 280:
void OnPermissionStatusChange(mojom::PermissionStatus);style: shouldn't this be above the member variables?
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:
Patch Set #7, Line 1321:
return accessibility_event_permission_ == mojom::PermissionStatus::GRANTED;Where is this called? I'm a bit worried about sync methods returning async info.
File third_party/WebKit/Source/modules/permissions/Permissions.cpp:
Patch Set #7, Line 106:
return CreatePermissionDescriptor(PermissionName::ACCESSIBILITY_EVENTS);Could you do like the device permissions above and reject with a TypeError when the feature isn't enabled?
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
5 comments:
File chrome/browser/accessibility/accessibility_permission_context.cc:
Patch Set #7, Line 13:
AccessibilityPermissionContext::~AccessibilityPermissionContext() = default;nit: `= default;`
Done
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #7, Line 200:
bool CanCallAOMEventListeners() const;style: add `const`
Done
style: leave empty line
Done
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:
Patch Set #7, Line 1321:
return accessibility_event_permission_ == mojom::PermissionStatus::GRANTED;Where is this called? I'm a bit worried about sync methods returning async info.
It's only called from AXObject::DispatchEventToAOMEventListeners.
My concern was that we need to check the permission every time
there's an accessibility event dispatch, because the feature
we're trying to protect is the ability for the app to capture
accessibility events, since that definitively identifies the
user as a person using assistive technology.Wouldn't it add a lot of latency to make an asynchronous
call to check the permission on every input event? Would
it ever block?I don't want it to block, if it prompts the user I want it
to just keep going as if the permission was denied, then we'll
catch the next event.And I don't want it to add any latency 99% of the time.
If I'm wrong and the permission check should be instant the
vast majority of the time but only have a bit of latency the
first time, then I could probably simplify this code by just
having it call permission_service_->GetPermission() every
time.
File third_party/WebKit/Source/modules/permissions/Permissions.cpp:
Patch Set #7, Line 106:
if (!RuntimeEnabledFeatures::AccessibilityObjectModelEnabled()) {Could you do like the device permissions above and reject with a TypeError when the feature isn't en […]
Yes, that works great, thanks. Done.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Alice Boxhall (Gerrit)
Patch set 9:Code-Review +1
2 comments:
File components/content_settings/core/common/content_settings_types.h:
Patch Set #9, Line 74:
ContentMerge issue?
File tools/metrics/histograms/enums.xml:
Patch Set #9, Line 5973:
<int value="38" label="Sensors setting"/>Odd rebase?
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
lgtm
Patch set 9:Code-Review +1
1 comment:
Patch Set #7, Line 1321:
return accessibility_event_permission_ == mojom::PermissionStatus::GRANTED;
It's only called from AXObject::DispatchEventToAOMEventListeners. […]
I think the rationale makes sense :)
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
2 comments:
Patch Set #9, Line 74:
ContentMerge issue?
This was on purpose, see earlier comment.
(There's a distinction between website settings and
content settings.)
File tools/metrics/histograms/enums.xml:
Patch Set #9, Line 5973:
<int value="38" label="Sensors setting"/>Odd rebase?
Also on purpose, it looks like it was missed in a previous
patch so I'm just adding it.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
+bauerb for components/content_settings
+mkwst for content/shell/browser/layout_test
and third_party/WebKit/public/platform
+jam for content/public/browser
+estade for components/vector_icons
(Note: this is an existing a11y icon from chrome os, but
it's just a placeholder and may change in launch review)
Dominic Mazzoni (Gerrit)
And one more, +torne for android_webview/
Evan Stade (Gerrit)
vector_icons lgtm
Richard Coles (Gerrit)
android_webview lgtm
Patch set 10:Code-Review +1
Mike West (Gerrit)
+mkwst for content/shell/browser/layout_test
and third_party/WebKit/public/platform
LGTM.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Loh (Gerrit)
Looks mostly good, but I had some comments before that you might have missed:
- Is this permission going to be supported on Android? If so you need to update GetIconId/GetMessageText on PermissionRequestImpl.
- Are we going to add this permission to Page Info?
- Has the string been reviewed (I think srahim@ is responsible for strings)?
Also there was some discussion at https://github.com/WICG/aom/issues/81 about considering alternatives to permission prompts, how sure are you at this point we'll move forwards with this as a permission?
Bernhard Bauer (Gerrit)
content settings LGTM
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
John Abd-El-Malek (Gerrit)
Kentaro Hara (Gerrit)
1 comment:
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #10, Line 273:
mojo::Binding<mojom::blink::PermissionObserver> permission_observer_binding_;dcheng@: We need to reset these mojo pointers when the associated document gets detached, right? i.e., we need to make AXObjectCacheBase inherit from ContextLifecycleObserver and clear the mojo pointers in ContextDestroyed.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
3 comments:
File chrome/browser/accessibility/accessibility_permission_context.h:
Patch Set #11, Line 20:
const GURL& requesting_origin,something something origin something something passing as a URL something something kitten
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #10, Line 273:
mojo::Binding<mojom::blink::PermissionObserver> permission_observer_binding_;
dcheng@: We need to reset these mojo pointers when the associated document gets detached, right? i. […]
I think that is a safer default (to block mojo calls after a context is destroyed), since that matches the behavior of legacy IPC. Unless we have a good reason not to do that here, I think we should match the standard convention.
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:
Patch Set #10, Line 1331:
document_->GetExecutionContext()->GetSecurityOrigin(),+raymes, is there a bug already filed for not passing the origin from the renderer to the browser for PermissionService?
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
Patch set 11:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/604830/11
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/604830/11
Bot data: {"action": "start", "triggered_at": "2017-08-22T15:03:51.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "1052b4e7b3241b06bd99bc8418a095afc817050b"}
Dominic Mazzoni (Gerrit)
Sorry, +2 by mistake
Patch set 11:-Commit-Queue
Evan Stade (Gerrit)
actually +1
Patch set 11:Code-Review +1
Dominic Mazzoni (Gerrit)
- Is this permission going to be supported on Android? If so you need to update GetIconId/GetMessageText on PermissionRequestImpl.
This is done now
- Are we going to add this permission to Page Info?
Yes, I was going to do that in a follow-up since this is
so large already.
- Has the string been reviewed (I think srahim@ is responsible for strings)?
No, and I consider the current text to just be a straw man
proposal, but since this isn't launching publicly yet,
I was planning to review all strings together when we get
to Intent to Ship.
Right now end-users shouldn't see these strings unless they
enable a runtime flag.
Also there was some discussion at https://github.com/WICG/aom/issues/81 about considering alternatives to permission prompts, how sure are you at this point we'll move forwards with this as a permission?
I'm pretty confident a permission prompt is needed, and Apple feels
pretty strongly about this too. We're definitely open to hearing
other ideas, but in talks with members of the accessibility
community this was a top concern.
3 comments:
Patch Set #11, Line 20:
const GURL& requesting_origin,something something origin something something passing as a URL something something kitten
Sorry, I don't understand your comment here!
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #10, Line 273:
mojo::Binding<mojom::blink::PermissionObserver> permission_observer_binding_;
I think that is a safer default (to block mojo calls after a context is destroyed), since that match […]
OK, I made it a ContextLifecycleObserver, but note that
AXObjectCacheImpl is owned by Document already, and I'm not
sure there's any point in having an AXObjectCache on an
unattached document.Right now we clear AXObjectCache in Document::Shutdown,
what if we just clear it when it's being unattached?
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #7, Line 280:
// the browser.
style: shouldn't this be above the member variables?
Reordered and moved to the bottom of this file. A lot of Blink
classes seem to mix functions and member variables, but I'd like
to clean up the whole header in a follow-up.
Timothy Loh (Gerrit)
Patch set 12:Code-Review +1
2 comments:
File chrome/browser/accessibility/accessibility_permission_context.cc:
Patch Set #12, Line 15:
ContentSetting AccessibilityPermissionContext::GetPermissionStatusInternal(This override looks redundant.
File chrome/browser/permissions/permission_request_impl.cc:
Patch Set #12, Line 95:
case CONTENT_SETTINGS_TYPE_ACCESSIBILITY_EVENTS:Sorry if I wasn't clear earlier, there's a separate branches for Android in this function and GetMessageText below is only called on Android.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Loh (Gerrit)
1 comment:
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:
Patch Set #10, Line 1331:
mojom::blink::PermissionName::ACCESSIBILITY_EVENTS),+raymes, is there a bug already filed for not passing the origin from the renderer to the browser fo […]
raymes@ is OOO until next week but sammc@ has talked about this before and might know?
Sam McNally (Gerrit)
1 comment:
Patch Set #10, Line 1331:
mojom::blink::PermissionName::ACCESSIBILITY_EVENTS),
raymes@ is OOO until next week but sammc@ has talked about this before and might know?
https://bugs.chromium.org/p/chromium/issues/detail?id=698985
Dominic Mazzoni (Gerrit)
2 comments:
Patch Set #12, Line 15:
ContentSetting AccessibilityPermissionContext::GetPermissionStatusInternal(This override looks redundant.
Patch Set #12, Line 95:
case CONTENT_SETTINGS_TYPE_ACCESSIBILITY_EVENTS:
Sorry if I wasn't clear earlier, there's a separate branches for Android in this function and GetMes […]
Ah, thanks. Added.
Daniel Cheng (Gerrit)
lgtm
Patch set 13:Code-Review +1
2 comments:
File chrome/browser/accessibility/accessibility_permission_context.h:
Patch Set #11, Line 20:
DISALLOW_COPY_AND_ASSIGN(AccessibilityPermissionContext);
Sorry, I don't understand your comment here!
Sorry about the confusion. I'm just generally sad when there's a parameter of GURL type named "origin" =)
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:
Patch Set #10, Line 273:
AXID GetOrCreateAXID(AXObject*);OK, I made it a ContextLifecycleObserver, but note that […]
As far as I can tell, this is garbage collected though, right? What guarantee do we have that this will be destroyed in a prompt fashion?
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
1 comment:
Patch Set #10, Line 273:
AXID GetOrCreateAXID(AXObject*);
As far as I can tell, this is garbage collected though, right? What guarantee do we have that this w […]
I think that it is guaranteed that Document that is attached to a frame calls Shutdown before getting destructed. (This is not true for a Document not attached to a frame; e.g., Document created by DOMImplementation)
(That being said, (as I mentioned in other places), it's not really nice to ask developers to manually reset mojo pointers at ContextDestroyed or pre-finalizers -- it's error-prone. We should consider improving the infrastructure of mojo pointers so that the rest is automatically done.)
Dominic Mazzoni (Gerrit)
1 comment:
Patch Set #10, Line 273:
AXID GetOrCreateAXID(AXObject*);
I think that it is guaranteed that Document that is attached to a frame calls Shutdown before gettin […]
OK, for the time being it does seem safest to use ContextDestroyed
to reset the mojo pointers due to corner cases like a Document that's
not attached to a frame. I've fixed some bugs involving a
JS-created Document caught by clusterfuzz in the past, so it's better
to be safe.Would be great if that could be handled automatically by mojo.
To view, visit change 604830. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
WebKit/ LGTM
Patch set 14:Code-Review +1
Dominic Mazzoni (Gerrit)
Patch set 14:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Add new permission dialog for accessibility events.
This is required as part of the Accessibility Object Model
phase 2 spec.
Bug: 746524
Change-Id: I5b63921663dee00e609ddfaf3aab5577229294c1
Reviewed-on: https://chromium-review.googlesource.com/604830
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Timothy Loh <tim...@chromium.org>
Reviewed-by: Evan Stade <est...@chromium.org>
Reviewed-by: Richard Coles <to...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Reviewed-by: Bernhard Bauer <bau...@chromium.org>
Reviewed-by: John Abd-El-Malek <j...@chromium.org>
Reviewed-by: Alice Boxhall <abox...@chromium.org>
Reviewed-by: Mounir Lamouri <mlam...@chromium.org>
Commit-Queue: Dominic Mazzoni <dmaz...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496801}
---
M android_webview/browser/aw_permission_manager.cc
A chrome/android/java/res/drawable-hdpi/infobar_accessibility_events.png
A chrome/android/java/res/drawable-mdpi/infobar_accessibility_events.png
A chrome/android/java/res/drawable-xhdpi/infobar_accessibility_events.png
A chrome/android/java/res/drawable-xxhdpi/infobar_accessibility_events.png
A chrome/android/java/res/drawable-xxxhdpi/infobar_accessibility_events.png
M chrome/app/generated_resources.grd
M chrome/browser/BUILD.gn
A chrome/browser/accessibility/accessibility_permission_context.cc
A chrome/browser/accessibility/accessibility_permission_context.h
M chrome/browser/android/resource_id.h
M chrome/browser/permissions/permission_manager.cc
M chrome/browser/permissions/permission_request.h
M chrome/browser/permissions/permission_request_impl.cc
M chrome/browser/permissions/permission_util.cc
M components/content_settings/core/browser/content_settings_registry.cc
M components/content_settings/core/common/content_settings.cc
M components/content_settings/core/common/content_settings_types.h
M components/vector_icons/BUILD.gn
A components/vector_icons/accessibility.icon
M content/browser/permissions/permission_service_impl.cc
M content/public/browser/permission_type.h
M content/shell/browser/layout_test/layout_test_message_filter.cc
M third_party/WebKit/LayoutTests/accessibility/aom-actions.html
M third_party/WebKit/LayoutTests/http/tests/resources/permissions-helper.js
M third_party/WebKit/Source/core/dom/AXObjectCache.cpp
M third_party/WebKit/Source/core/dom/AXObjectCache.h
M third_party/WebKit/Source/core/dom/AXObjectCacheBase.cpp
M third_party/WebKit/Source/core/dom/AXObjectCacheBase.h
M third_party/WebKit/Source/modules/accessibility/AXObject.cpp
M third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
M third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h
M third_party/WebKit/Source/modules/accessibility/DEPS
M third_party/WebKit/Source/modules/permissions/PermissionDescriptor.idl
M third_party/WebKit/Source/modules/permissions/Permissions.cpp
M third_party/WebKit/public/platform/WebFeaturePolicyFeature.h
M third_party/WebKit/public/platform/modules/permissions/permission.mojom
M tools/metrics/histograms/enums.xml
38 files changed, 429 insertions(+), 136 deletions(-)