Implement the macOS accessibility AXTextOperation parameterized attribute. [chromium/src : main]
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kMacAccessibilityTextOperation, base::FEATURE_ENABLED_BY_DEFAULT);Samar SunkariaThis should be disabled by default.
To step back, what is your plan to roll this out?
For typical Chrome features, we have a rollout process which involves a series of additional reviews. For something smaller like this, it would ideally include a documented test plan. As I understand it, this API mainly targeted Voice Control on Mac. The full articulation of the support would have to be address in such a plan.
All this said, I understand your usage may not be along these lines.
David TsengSince this is an isolated change (i.e., it does not modify any existing AX APIs), I was hoping we could have it be enabled by default and, in the unlikely case, if we run into issues in the wild, disable the gate.
I am not familiar with the rollout process and would really appreciate some help with the plan. Is there precedent for how new Mac AX attributes have been introduced in the past?
For something smaller like this, it would ideally include a documented test plan. As I understand it, this API mainly targeted Voice Control on Mac.
Do we have similar test plans for other AX APIs? It's difficult to know which assistive apps use this API or will continue to do so in the future, which makes creating a valid test plan hard.
In particular, Voice Control seems to use `AXSearchTextWithCriteria` as a first step for finding text related to its text operations (like select "hey" or uppercase "hey"). [That API was introduced alongside `AXTextOperation`](https://github.com/WebKit/WebKit/commit/18e2f22b0bfdb0b0c00a074fe21a6cf3e4433221#diff-9fb71b6fa7156160059b0216d05933e621d422df2b20f72ad7399eb946b8ba04). Chromium does not implement that API, and without it, Voice Control fails to execute the commands I listed above (error says: "hey" not found), therefore we can't use it for our testing.
This is a part of the reason why I really wanted (and have added) end-to-end browser tests, because they emulate AX calls like an external client.
I understand your usage may not be along these lines.
Our use case is similar in principle. We want to use AXTextOperation for its text-marker based bulk text replacement capabilities.
In general, I'm open to whatever approach you think is best to move forward.
Samar SunkariaWe're in somewhat uncharted territory here as (particularly for Mac), there have been few external contributions for accessibility.
I do have various examples of API-based contributions for other platforms but would have to see about how to share them.
Generally speaking, we would have started with some higher level deliverable e.g. support Voice Control/Grammarly editing in simple inputs, text areas, content editables, and try to come up with user journeys. From there, maybe a design would have been written, and an associated test plan/suite also composed.
Practically speaking, I suppose we don't need that level of formalism, but I would like to see a spreadsheet of concrete test cases against Voice Control and any built-in a11y feature for Mac that we would consider in scope for this api. We can, obviously, include the Grammarly client in this mix as well.
We would then (hopefully) have folks (perhaps on your end) test a release production version of Chrome against such a suite. e.g. on dev channel with the feature flag turned on.
Agreed, using the Carbon C-apis to simulate the AT end of the interaction is great, but we'd ideally get more coverage with the full feature too.For release, Chrome features such as this one, can be rolled out to an increasing population of users from a Google server. This helps with monitoring for crashes, user reports, etc. It would also be a great chance to announce a dogfood e.g. if we ramp to 100% beta for the flag.
We would gradually ramp to various levels for stable.
This part we can help with from Google internal configs.
Samar SunkariaThank you for the detailed response.
I ended up reaching out to a WebKit engineer, asking them about which of Apple's ATs currently use `AXTextOperation`. After looking into it, they informed me that *none* of their ATs are currently using it; and that Voice Control uses the related `AXSelectTextWithCriteria` (note the word *Select*) API, which is [currently marked for deprecation](https://github.com/WebKit/WebKit/blob/2005be1668919990c9c1adcf639c03dfd77de6ff/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm#L3013) in favor of `AXSearchTextWithCriteria` (note the word *Search*) and `AXTextOperation`. `AXSelectTextWithCriteria` lets you search for some text AND perform the same text operations as `AXTextOperation` in the same API call. I was able to confirm that `AXSelectTextWithCriteria` was in fact being called.
[`AXTextOperation` and `AXSearchTextWithCriteria` originated from `AXSelectTextWithCriteria`](https://github.com/WebKit/WebKit/commit/18e2f22b0bfdb0b0c00a074fe21a6cf3e4433221). In fact, internally `AXSelectTextWithCriteria` [calls the same code paths as a call to `AXSearchTextWithCriteria` followed by a call to `AXTextOperation`](https://github.com/WebKit/WebKit/blob/2005be1668919990c9c1adcf639c03dfd77de6ff/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm#L3022). If WebKit does decided to deprecate/remove `AXSelectTextWithCriteria`, Voice Control should be able to switch over to `AXTextOperation`.
As things stand today, it seems like Grammarly is the biggest client of AXTextOperation in WebKit.
This does mean that there is a significantly smaller risk associated with introducing this change. That said, we will create a spreadsheet with Grammarly client specific test cases that anyone (internal or external to Grammarly) will be able to validate. Once this change is available in a dev channel, we will validate it against the test suite. We're also committed to serving as the point of contact for any issues that arise with this API, as well as actively monitoring for regressions.
David TsengI have updated the feature to be disabled by default.
Samar Sunkaria>in favor of `AXSearchTextWithCriteria` (note the word *Search*) and `AXTextOperation`.
Got it. Didn't read the reference links before replying, but effectively, select text -> search + text operation. Did the WebKit contact (presumably Dominic Mazzoni) mention a timeline of some sort? Given Grammarly's interest here, would it be reasonable to say you could help with the Voice Control support once WebKit migrates to search + text operation e.g. for Mac 27?
>We're also committed to serving as the point of contact for any issues that arise with this API, as well as actively monitoring for regressions.
Sounds good and thanks for this commitment. We can follow up in the tracking bug with the sheet and summarize this thread there.
David TsengThank you. I have posted a comment in the tracking bug with a [link to the test cases](https://coda.io/d/Support-AXTextOperation-in-Chromium_dxS65DpyOQz/AXTextOperation-Chromium-Test-Cases-for-Grammarly_suALvQ1V#Test-Cases_tutwp2kM/r1&columnId=c-5j8d3uAxI8) for Grammarly.
For this CL, what should be the next steps?
1. I have another CL that pulls out the CharacterRangeMapper (https://crrev.com/c/6797330). Should we land it independently and then rebase this CL, or should we land this as one CL and close the other one? For the former, it would be nice to get another review so that we can land it, and for the latter, I can rebase this CL immediately (+ fix the merge conflict).
2. How can I trigger CI jobs to ensure everything builds properly across all platforms and validate that none of the tests fail?
3. Should I add the `MacAccessibilityTextOperation` feature to `chrome://flags` as [described here](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md)?
---
As for supporting text editing with Voice Control, all that Chromium seemingly needs to implement right now is `AXSearchTextWithCriteria` (and through it `AXSelectTextWithCriteria`). As for deprecating `AXSelectTextWithCriteria`, no timeline was mentioned to me (different contact btw, I can mention their name over DMs). Though the pragmatic solution here would be to implement `AXSelectTextWithCriteria` through a combination of `AXSelectTextWithCriteria` + `AXTextOperation`, which would allow Chromium to work with Voice Control on older versions of macOS as well.
To test that I wrote up a quick and dirty implementation of `AXSearchTextWithCriteria` and tested it with Voice Control. It seemed to work! https://drive.google.com/file/d/1z_IHGSfYPBGOaGPwQbgAJckXb09_mWmJ/view?usp=sharing
(The actual implementation of `AXSearchTextWithCriteria` would require implementing a "Find" operation on the cached AX tree. My quick & dirty implementation was simply extracting all the text from the page and then doing a string search + converting the offsets back to an AXRange.)
Samar Sunkaria1. I have another CL that pulls out the CharacterRangeMapper (https://crrev.com/c/6797330). Should we land it independently and then rebase this CL, or should we land this as one CL and close the other one? For the former, it would be nice to get another review so that we can land it, and for the latter, I can rebase this CL immediately (+ fix the merge conflict).
Sg on landing multiple changes behind the same flag; also likely easier for reviewers. Go ahead and add Alexander Surkov to the other change.
2. How can I trigger CI jobs to ensure everything builds properly across all platforms and validate that none of the tests fail?If you're not yet a committer or don't have try bot access, a committer can run them for you.[1] I went ahead and kicked off a dry run..
3. Should I add the `MacAccessibilityTextOperation` feature to `chrome://flags` as [described here](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md)?Sure; we could also consider naming it something else that could encompass this area e.g. improved Mac OS accessibility editing.
---As for supporting text editing with Voice Control, all that Chromium seemingly needs to implement right now is `AXSearchTextWithCriteria` (and through it `AXSelectTextWithCriteria`). As for deprecating `AXSelectTextWithCriteria`, no timeline was mentioned to me (different contact btw, I can mention their name over DMs). Though the pragmatic solution here would be to implement `AXSelectTextWithCriteria` through a combination of `AXSelectTextWithCriteria` + `AXTextOperation`, which would allow Chromium to work with Voice Control on older versions of macOS as well.
This generally sounds good i.e. supporting `AXSelectTextWithCriteria`. Feel free to mention me as well if that helps.
To test that I wrote up a quick and dirty implementation of `AXSearchTextWithCriteria` and tested it with Voice Control. It seemed to work! https://drive.google.com/file/d/1z_IHGSfYPBGOaGPwQbgAJckXb09_mWmJ/view?usp=sharingGreat!
(The actual implementation of `AXSearchTextWithCriteria` would require implementing a "Find" operation on the cached AX tree. My quick & dirty implementation was simply extracting all the text from the page and then doing a string search + converting the offsets back to an AXRange.)If we could do this all browser side (e.g. by directly recursing the BrowserAccessibility/AXTree), that seems like a reasonable approach to start.
1. https://chromium.googlesource.com/chromium/src/+/master/docs/infra/trybot_usage.md
David TsengI have pushed a change to resolve the merge conflict + added a chrome flag.
Go ahead and add Alexander Surkov to the other change
[Alex mentioned that they may not be the best reviewer for that change](https://chromium-review.googlesource.com/c/chromium/src/+/6781482/comments/9808442b_aa737490). I can add them again if you suggest, or perhaps you or someone else could take a look.
If you're not yet a committer or don't have try bot access, a committer can run them for you.[1] I went ahead and kicked off a dry run..
Thank you for that. The jobs seemed to have failed due to the merge conflict. (And no, I'm not a committer)
---
For `AXSearchTextWithCriteria`, I want to clarify that we aren't planning to add support for it to Chromium since Grammarly currently doesn't use that API. We also won't be able to validate that it functions correctly or actively monitor it for regressions, like we can for AXTextOperations.
Samar SunkariaFor `AXSearchTextWithCriteria`, I want to clarify that we aren't planning to add support for it to Chromium since Grammarly currently doesn't use that API. We also won't be able to validate that it functions correctly or actively monitor it for regressions, like we can for AXTextOperations.
But, you will add support for the other (deprecated) select* api?
Unfortunately, the reason why I brought up built-in Mac accessibility features is because these api's more or less target those features and not really 3p applications.
Without Voice Control support, it's not clear to me that Chrome satisfies enough of the api surface especially for the thinly documented ones here for web which Apple can modify at will in WebKit.I think if Grammarly were willing to do the initial implementation (which I think you've already started), I can meet you halfway on the support in terms of fixing regressions for Voice Control specifically.
On the other cl, sure, feel free to add me.
David TsengI talked about this internally, and we can commit to submitting changes for `AXSearchTextWithCriteria` and `AXSelectTextWithCriteria` if that makes a stronger case for landing the AXTextOperation API.
We will put up those changes in a separate CL (I am currently working on it), but we hope that we can land and release `AXTextOperation` independently. This also means that we would want different feature flags for `AX[Search/Select]TextWithCriteria` (because we know that Voice Control does not directly use `AXTextOperation`, so if we run into issues with Voice Control, we would only need disable the search/select text criteria APIs). How does that sound to you?
It would be nice to get yours or someone else's opinion on what's the best approach for efficiently walking the BrowserAccessibility tree to find a string. (Or if the less efficient GetTextContentUTF16 + NSString search is good enough).
Also, I have added you to the CL here: https://crrev.com/c/6797330
And I have pushed a fix for the failed chromeos build + test failure. Now that we have a change in `about_flags.cc`, I anticipate running into merge conflicts frequently.---
But, you will add support for the other (deprecated) select* api?
We weren't planning to originally, no. Now we are. BTW, it seems like [Chromium already declares support for `AXSelectTextWithCriteria`](https://source.chromium.org/chromium/chromium/src/+/main:ui/accessibility/platform/browser_accessibility_cocoa.mm;drc=4497bff67a809a49fdb7f3fa7635329021000e2f;l=2336) even though it is not actually supported. I suppose this was unintentional.
[...] these api's more or less target those features and not really 3p applications.
[...] which Apple can modify at will in WebKit.I guess it's unrelated to our discussion, but I do want to touch on this. While there is some truth to this, in practice, these API are considered to be available to 3p apps, and are often widely used by Mac AX clients (AXTextOperation might not be the most commonly used, but I'm generally talking about all the AX APIs that aren't exposed through NSAccessibility). WebKit has historically been careful about breaking or removing these APIs (without ample warning) even if they were only intended for their AX clients. And in the case of AXTextOperation, WebKit folks are aware that Grammarly uses it (we have submitted bug fixes in the past).
I talked about this internally, and we can commit to submitting changes for `AXSearchTextWithCriteria` and `AXSelectTextWithCriteria` if that makes a stronger case for landing the AXTextOperation API.
We will put up those changes in a separate CL (I am currently working on it), but we hope that we can land and release `AXTextOperation` independently. This also means that we would want different feature flags for `AX[Search/Select]TextWithCriteria` (because we know that Voice Control does not directly use `AXTextOperation`, so if we run into issues with Voice Control, we would only need disable the search/select text criteria APIs). How does that sound to you?
So long as we structure the code and the flag guards to be independent, we can have two separate flags for search/select and text operation.
It would be nice to get yours or someone else's opinion on what's the best approach for efficiently walking the BrowserAccessibility tree to find a string. (Or if the less efficient GetTextContentUTF16 + NSString search is good enough).
Rather than precomputing all text in a frame/a11y tree, doing an incremental tree walk, into static text nodes of the BrowserAccessibility tree, matching against their text (accessible name) is likely the better approach. We have some machinery that may fit all of the needs here in u/a/p/one_shot_tree_search.*.
Also, I have added you to the CL here: https://crrev.com/c/6797330
Great; yes, I recall it. You would have better luck adding other owners to the change (work your way up the directory tree if necessary) in order to get some locality of expertise for those files/directories.
I guess it's unrelated to our discussion, but I do want to touch on this. While there is some truth to this, in practice, these API are considered to be available to 3p apps, and are often widely used by Mac AX clients (AXTextOperation might not be the most commonly used, but I'm generally talking about all the AX APIs that aren't exposed through NSAccessibility). WebKit has historically been careful about breaking or removing these APIs (without ample warning) even if they were only intended for their AX clients. And in the case of AXTextOperation, WebKit folks are aware that Grammarly uses it (we have submitted bug fixes in the past).
They are available true, but generally speaking, the motivation behind the api's is to support users with disabilities. While I understand and think some level of usage beyond those original terms is healthy and positive, I'd stop short of adding support that excludes those accessibility services/features as the origin.
The added dimension for Chrome is we don't have the Apple internals to keep up with a lot of their changes and have a somewhat fragile codebase (as you also pointed out above) for Mac a11y.
It's also untrue that APple doesn't break api's. A number of major Mac (OS) releases broke text editing with VoiceOver due to usage of additional parameterized attributes.
All this said not to discourage, but hopefully to align on our shared reality of things as they look to us with the many caveats therein.
Related details
- Alexander Surkov
- Antonio Sartori
- Koji Ishii
- Samar Sunkaria
- Tzarial
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Sunkaria (Gerrit)
Samar Sunkaria added 1 comment![]( "Open in Gerrit")
Hi,
I have received all the required reviews on my other CL. Can you please help me land it? https://crrev.com/c/6797330.
We have some machinery that may fit all of the needs here in u/a/p/one_shot_tree_search.*.
Thank you for the suggestion. I looked into `one_shot_tree_search` and it only searches for the text in a particular node and not across nodes (which we need for a "find" operation). I ended up using the `ui::AXRange::GetText` as inspiration for walking the tree and searching the text (in many ways it's similar to a text/character iterator). I will create a CL when it's ready.
Related details
- Alexander Surkov
- Antonio Sartori
- David Tseng
- Koji Ishii
- Tzarial
Samar Sunkaria (Gerrit)
Hi David, wanted to follow up on this.
I am waiting on https://crrev.com/c/6797330. Like I mentioned, I have received the required reviews, but I don't have the permissions to land my changes. Once it lands, I can rebase this CL and resolve the conflict.
As for AXSearchTextWithCriteria and Voice Control support, I have prepared some changes locally that depend on this CL. I will be create a new CL for it once this one lands.
Thanks
Related details
- Alexander Surkov
Katie Dektar (Gerrit)
Katie Dektar added 1 comment![]( "Open in Gerrit")
Looks like David is already reviewing; also I don't feel that I have the right expertise to review this CL.
Related details
- Alexander Surkov
- David Tseng
- Koji Ishii
- Samar Sunkaria
- Tzarial
Samar Sunkaria (Gerrit)
Samar Sunkaria added 1 comment![]( "Open in Gerrit")
Looks like David is already reviewing; also I don't feel that I have the right expertise to review this CL.
Thank you for the response Katie. I’m hoping to get a follow up from David.
Related details
- Alexander Surkov
- David Tseng
- Katie Dektar
- Koji Ishii
- Tzarial
Samar Sunkaria (Gerrit)
Samar Sunkaria added 1 comment![]( "Open in Gerrit")
Samar SunkariaLooks like David is already reviewing; also I don't feel that I have the right expertise to review this CL.
Thank you for the response Katie. I’m hoping to get a follow up from David.
Sorry for adding you as a reviewer again Katie. I have not been able to reach David, can you please advise me on the next steps.
Related details
- Alexander Surkov
- David Tseng
- Katie D
- Koji Ishii
- Tzarial
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
Apologies for the delay; I was hoping to have better news for how to roll this out.
We're still resourcing long term ownership over Mac a11y, so that is still tbd. Things are in the works but I don't want to set any expectations as I honestly don't know myself as I'm not really the decider here (anymore).
I don't see any reason to hold this change up given it is flagged though.
Related details
- Alexander Surkov
- Katie D
- Koji Ishii
- Samar Sunkaria
- Tzarial
David Tseng (Gerrit)
David Tseng voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Samar Sunkaria (Gerrit)
Samar Sunkaria added 1 comment![]( "Open in Gerrit")
Apologies for the delay; I was hoping to have better news for how to roll this out.
We're still resourcing long term ownership over Mac a11y, so that is still tbd. Things are in the works but I don't want to set any expectations as I honestly don't know myself as I'm not really the decider here (anymore).
I don't see any reason to hold this change up given it is flagged though.
Thank you for following up :)
I have rebased my changes and validated that they still work.
I also have some staged changes for text editing via Voice Control, which I plan to post once this one lands (though I'll be OOO next week). Until the ownership of Mac a11y is clarified, would you be the point of contact for the following CLs and for enabling the gates? If not, whom should I reach out to?
Related details
- Alexander Surkov
- David Tseng
- Katie D
- Koji Ishii
- Tzarial
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 3 comments![]( "Open in Gerrit")
Thanks Samar, it looks like it's been a long road on this one, thanks for sticking it through! Schema changes mostly LGTM, but we should remove the tools/json_schema_compiler/test/converted_schemas/ files from this change (I've explained that in more detail below).
// Arguments for the replaceRanges action supplied to performAction.Aside: Huh. I'd never looked closely at this schema, but it's kind of strange that we have all these dictionaries defining types which are not directly referenced elsewhere in the schema file. It appears intended to be passed as the second argument (`opt_args`) to `performAction`, which says it takes a generic `object` type.
As a small future improvement, we could change this to instead use a Union type for `opt_args` that declares all the valid types e.g.
`(PerformCustomActionParams or SetSelectionParams or ...)`
This would let you rely on the built in type validation for callers that the extensions bindings supply, so long as there's not anything else funky going on with some action params not being defined and just relying on the generic object.
But that should come as a separate followup change and not be loaded into this one.
// Arguments for the replaceRanges action supplied to performAction.
dictionary ReplaceRangesParams {
DOMString[] replacementStrings;
long[] startAnchorIds;
long[] startOffsets;
long[] endAnchorIds;
long[] endOffsets;
};
Sorry for the confusion with the files in this folder, but they don't need to be updated as they were a snapshot of the schemas files at the time of the conversion from IDL -> WebIDL (to ensure they were converted cleanly). This highlights that I should add a README and PRESUBMIT to this folder to make this clear.
(Also side note: removing these files from the patch set will appear to wipe out individual +1s, but those are just CR+1s and any owners +1's you have already gotten remain sticky and don't need to be re-applied. You will need one more CR+1 again after, which I'll be able to provide).
Related details
- Alexander Surkov
- David Tseng
- Koji Ishii
- Samar Sunkaria
- Tzarial
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Sunkaria (Gerrit)
Samar Sunkaria added 3 comments![]( "Open in Gerrit")
Thanks Samar, it looks like it's been a long road on this one, thanks for sticking it through! Schema changes mostly LGTM, but we should remove the tools/json_schema_compiler/test/converted_schemas/ files from this change (I've explained that in more detail below).
Thank you, Tim. Yes, this has indeed been a long one :)
I have removed the changes in converted_schemas.
// Arguments for the replaceRanges action supplied to performAction.Aside: Huh. I'd never looked closely at this schema, but it's kind of strange that we have all these dictionaries defining types which are not directly referenced elsewhere in the schema file. It appears intended to be passed as the second argument (`opt_args`) to `performAction`, which says it takes a generic `object` type.
As a small future improvement, we could change this to instead use a Union type for `opt_args` that declares all the valid types e.g.
`(PerformCustomActionParams or SetSelectionParams or ...)`
This would let you rely on the built in type validation for callers that the extensions bindings supply, so long as there's not anything else funky going on with some action params not being defined and just relying on the generic object.But that should come as a separate followup change and not be loaded into this one.
>we could change this to instead use a Union type for opt_args that declares all the valid types e.g.
(PerformCustomActionParams or SetSelectionParams or ...)
That sounds like a nice improvement to have :)
// Arguments for the replaceRanges action supplied to performAction.
dictionary ReplaceRangesParams {
DOMString[] replacementStrings;
long[] startAnchorIds;
long[] startOffsets;
long[] endAnchorIds;
long[] endOffsets;
};
Sorry for the confusion with the files in this folder, but they don't need to be updated as they were a snapshot of the schemas files at the time of the conversion from IDL -> WebIDL (to ensure they were converted cleanly). This highlights that I should add a README and PRESUBMIT to this folder to make this clear.
(Also side note: removing these files from the patch set will appear to wipe out individual +1s, but those are just CR+1s and any owners +1's you have already gotten remain sticky and don't need to be re-applied. You will need one more CR+1 again after, which I'll be able to provide).
- Alexander Surkov
- David Tseng
- Koji Ishii
- Tim
- Tzarial
Tim (Gerrit)
Tim voted and added 1 comment![]( "Open in Gerrit")
- Alexander Surkov
- David Tseng
- Koji Ishii
- Samar Sunkaria
- Tzarial
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Sunkaria (Gerrit)
Samar Sunkaria added 2 comments![]( "Open in Gerrit")
Samar SunkariaCould this CL be split?
Koji IshiiThe key changes here are implemented in four places:
- browser_accessibility_cocoa (Mac API),
- browser_accessibility_manager (serializing the operation),
- ax_object (performing the operation), and
- character_range_mapper (needed for performing the operation correctly).
These changes by themselves are fairly small and isolated from the rest of the codebase. Most of the heft of the CL can be attributed to testing these changes.
But I'm happy to break it down along those boundaries if it makes sense to land them in isolation (which may involve temporarily having some unused code in the repo).
Samar SunkariaThis may vary by who you ask, but I think in general 2.5k LOC is a bit too big. Big patches have disadvantages such as lowering the review quality, may require reviewers to review multiple times, etc., that it's better to split, if doing so is feasible.
For example, I might review files I own, but if reviewers in other directories requested to add/remove one file, I'll have to review it again, even if files I review don't change at all.
It's true that there are some cases where splitting complicates things. In that case, reviewers can accept it, but I think this isn't in such situation, is this?
Samar SunkariaFair point. I'll start splitting out the components. I'll keep this CL open and rebase it as those components land. (Unless this is handled differently, please feel free to comment).
Samar SunkariaPulled out the CharacterRangeMapper into: https://crrev.com/c/6797330
Done
// Arguments for the replaceRanges action supplied to performAction.Samar SunkariaAside: Huh. I'd never looked closely at this schema, but it's kind of strange that we have all these dictionaries defining types which are not directly referenced elsewhere in the schema file. It appears intended to be passed as the second argument (`opt_args`) to `performAction`, which says it takes a generic `object` type.
As a small future improvement, we could change this to instead use a Union type for `opt_args` that declares all the valid types e.g.
`(PerformCustomActionParams or SetSelectionParams or ...)`
This would let you rely on the built in type validation for callers that the extensions bindings supply, so long as there's not anything else funky going on with some action params not being defined and just relying on the generic object.But that should come as a separate followup change and not be loaded into this one.
>we could change this to instead use a Union type for opt_args that declares all the valid types e.g.
(PerformCustomActionParams or SetSelectionParams or ...)That sounds like a nice improvement to have :)
Done
Related details
- Alexander Surkov
- David Tseng
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
Samar SunkariaApologies for the delay; I was hoping to have better news for how to roll this out.
We're still resourcing long term ownership over Mac a11y, so that is still tbd. Things are in the works but I don't want to set any expectations as I honestly don't know myself as I'm not really the decider here (anymore).
I don't see any reason to hold this change up given it is flagged though.
Thank you for following up :)
I have rebased my changes and validated that they still work.
I also have some staged changes for text editing via Voice Control, which I plan to post once this one lands (though I'll be OOO next week). Until the ownership of Mac a11y is clarified, would you be the point of contact for the following CLs and for enabling the gates? If not, whom should I reach out to?
>Until the ownership of Mac a11y is clarified, would you be the point of contact for the following CLs and for enabling the gates?
Existing OWNERS should be able to do a review, but enabling flags will require the official Mac POC. That's coming online, but I don't have a precise timeline. We're at least able to say there's someone allocated there (which is why I really didn't respond for some time).
I'll start adding the person to the follow up changes and will likely be helping them with reviews as they ramp up.
Related details
- Alexander Surkov
- Koji Ishii
- Samar Sunkaria
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samar Sunkaria (Gerrit)
Samar Sunkaria added 2 comments![]( "Open in Gerrit")
Samar SunkariaCould this CL be split?
Koji IshiiThe key changes here are implemented in four places:
- browser_accessibility_cocoa (Mac API),
- browser_accessibility_manager (serializing the operation),
- ax_object (performing the operation), and
- character_range_mapper (needed for performing the operation correctly).
These changes by themselves are fairly small and isolated from the rest of the codebase. Most of the heft of the CL can be attributed to testing these changes.
But I'm happy to break it down along those boundaries if it makes sense to land them in isolation (which may involve temporarily having some unused code in the repo).
Samar SunkariaThis may vary by who you ask, but I think in general 2.5k LOC is a bit too big. Big patches have disadvantages such as lowering the review quality, may require reviewers to review multiple times, etc., that it's better to split, if doing so is feasible.
For example, I might review files I own, but if reviewers in other directories requested to add/remove one file, I'll have to review it again, even if files I review don't change at all.
It's true that there are some cases where splitting complicates things. In that case, reviewers can accept it, but I think this isn't in such situation, is this?
Samar SunkariaFair point. I'll start splitting out the components. I'll keep this CL open and rebase it as those components land. (Unless this is handled differently, please feel free to comment).
Samar SunkariaPulled out the CharacterRangeMapper into: https://crrev.com/c/6797330
Done
@ko...@chromium.org can you plase take a look at the one remaining file in blink editing?
Samar SunkariaThanks Samar, it looks like it's been a long road on this one, thanks for sticking it through! Schema changes mostly LGTM, but we should remove the tools/json_schema_compiler/test/converted_schemas/ files from this change (I've explained that in more detail below).
Thank you, Tim. Yes, this has indeed been a long one :)
I have removed the changes in converted_schemas.
- Alexander Surkov
- Koji Ishii
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tim
| Code-Review | +1 |
1 comment
WebIDL file LGTM! Sorry I swapped that one out from under you while you were still working on this!
Related details
- Alexander Surkov
- Koji Ishii
- Samar Sunkaria