[a11y] Prevent incorrect text edit reporting for spin buttons on macOS. [chromium/src : main]
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 5 comments![]( "Open in Gerrit")
ScopedBlinkAXEventIntent scoped_blink_ax_event_intent(Carlos Marcelo Barrera NolascoDid you get the chance to see how the existing intents are wired up and if this is the right place for this to be added? (e.g. if we are doubling the way we track intent sin some cases, if the scopes for the intents are not overlapping, why we didn't get typihng intents before, etc)?
I'm not familiar with this call path enough to comment, and maybe OWNERS here can verify.
These intents are no longer necessary as I've created new intents for increment/decrement. This change was removed.
Why here?
When using the arrows, the user is performing a Value Change on a form control, it's not a "Command" in the editing sense, it bypasses the Editor module entirely and calls the value update logic directly. Therefore, it never touches the Apply() method of the CompositeEditCommand base class, which is the method that adds the intents for edits due to typing. Instead, we can attach the increment/decrement intents to the following methods that handle those scenarios.
case ax::mojom::Command::kDecrement:@dts...@chromium.org I think AXTextEditType::kInsert is the best available text edit type for this action.
@nek...@chromium.org I've noticed that you implemented this enum in this CL https://chromium-review.googlesource.com/c/chromium/src/+/2424166. There's a comment asking not to renumber this enum; however, I think it's out of sync. Checking its WebKit counterpart (https://github.com/WebKit/WebKit/blob/386d6232aa5bbe3ad751b37f5761ef1d681f5949/Source/WebCore/accessibility/AXTextStateChangeIntent.h#L40-L41), there's a value that hasn't been taken into account (Replace) which should be 7 (AttributesChange should change to 8). Is there a reason why this value wasn't considered (perhaps it didn't exist at the time of the Chrome implementation)? If there's no compelling reason to omit that value, can I update the enum?
// Maps AXNode IDs to the intents associated with events in the current batch.Carlos Marcelo Barrera NolascoMaybe be a little more explicit e.g. events received in OnAccessibilityEvents? And which events e.g. AXTreeUpdate.events or generated events etc?
I've updated the comment to reflect the purpose.
if (!is_typing) {Carlos Marcelo Barrera NolascoWhich intent do we get when the value changes due to up/down arrow changing the value?
The other way we can possibly handle this is to trace the exact point where the spin button changes the value and just explicitly mark it in the node. That's likely an even more robust way to differentiate here I think.
I don't have a clear sense of what that looks like, but maybe you could consider it e.g. using intents there too.Otherwise, the current change definitely looks more reasonable than before.
There were no intents when the value changed for the text field due to increment/decrement, I've created the new intents for this scenario and used them when the increment/decrement is handled.
Related details
- David Tseng
- Nektarios Paisios
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 2 comments![]( "Open in Gerrit")
case AXEventGenerator::Event::VALUE_IN_TEXT_FIELD_CHANGED:I wonder if we should move the logic to AXEventGenerator. i.e. if we see an increment, decrement intent, then the generated event should be a new increment, decrement generated event?
If you agree, this could also avoid us doing more bookkeeping with active_batch_intents_ (there's some already on AXTree, AXEventGenerator).
(This would be a cross platform change though, so feel free to think it through/push back/defer to another change).
batch_intents[event.id] = event.event_intents;Note that intents are set for *both* AXTreeUpdate and AXEvent (the two collections in a single AXUpdatesAndEvents).
You only store the latter, but we really want both. When an AXTreeUpdate gets unserialized, that is the path for the generated event. An AXEvent is the path when Blink fires an explicit event.
If the AXUpdatesAndEvents has no explicit events, then we would not actually process any of this.
Related details
- Carlos Marcelo Barrera Nolasco
- Nektarios Paisios
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
case ax::mojom::Command::kDecrement:@dts...@chromium.org I think AXTextEditType::kInsert is the best available text edit type for this action.
@nek...@chromium.org I've noticed that you implemented this enum in this CL https://chromium-review.googlesource.com/c/chromium/src/+/2424166. There's a comment asking not to renumber this enum; however, I think it's out of sync. Checking its WebKit counterpart (https://github.com/WebKit/WebKit/blob/386d6232aa5bbe3ad751b37f5761ef1d681f5949/Source/WebCore/accessibility/AXTextStateChangeIntent.h#L40-L41), there's a value that hasn't been taken into account (Replace) which should be 7 (AttributesChange should change to 8). Is there a reason why this value wasn't considered (perhaps it didn't exist at the time of the Chrome implementation)? If there's no compelling reason to omit that value, can I update the enum?
@dts...@chromium.org I think AXTextEditType::kInsert is the best available text edit type for this action.
Ack. Looks like you introduced increment/decrement to Command.
@nek...@chromium.org I've noticed that you implemented this enum in this CL https://chromium-review.googlesource.com/c/chromium/src/+/2424166. There's a comment asking not to renumber this enum; however, I think it's out of sync. Checking its WebKit counterpart (https://github.com/WebKit/WebKit/blob/386d6232aa5bbe3ad751b37f5761ef1d681f5949/Source/WebCore/accessibility/AXTextStateChangeIntent.h#L40-L41), there's a value that hasn't been taken into account (Replace) which should be 7 (AttributesChange should change to 8). Is there a reason why this value wasn't considered (perhaps it didn't exist at the time of the Chrome implementation)? If there's no compelling reason to omit that value, can I update the enum?
nektar@ isn't working on CHromium or at Google anymore.
The internal enums (CHrome's Command mojo enum here I guess) isn't public (e.g. defined in a Mac sdk). Both webkit and blink probably pull from similar W3C specs, so the values resemble each other, but I think it's ok to have different enum values. Both webkit and blink at some point have to map these directly or indirectly to a public Mac a11y api.
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco voted and added 8 comments![]( "Open in Gerrit")
Votes added by Carlos Marcelo Barrera Nolasco
| Commit-Queue | +1 |
8 comments
ScopedBlinkAXEventIntent scoped_blink_ax_event_intent(Carlos Marcelo Barrera NolascoDid you get the chance to see how the existing intents are wired up and if this is the right place for this to be added? (e.g. if we are doubling the way we track intent sin some cases, if the scopes for the intents are not overlapping, why we didn't get typihng intents before, etc)?
I'm not familiar with this call path enough to comment, and maybe OWNERS here can verify.
These intents are no longer necessary as I've created new intents for increment/decrement. This change was removed.
Done
Why here?
When using the arrows, the user is performing a Value Change on a form control, it's not a "Command" in the editing sense, it bypasses the Editor module entirely and calls the value update logic directly. Therefore, it never touches the Apply() method of the CompositeEditCommand base class, which is the method that adds the intents for edits due to typing. Instead, we can attach the increment/decrement intents to the following methods that handle those scenarios.
Done
David Tseng@dts...@chromium.org I think AXTextEditType::kInsert is the best available text edit type for this action.
@nek...@chromium.org I've noticed that you implemented this enum in this CL https://chromium-review.googlesource.com/c/chromium/src/+/2424166. There's a comment asking not to renumber this enum; however, I think it's out of sync. Checking its WebKit counterpart (https://github.com/WebKit/WebKit/blob/386d6232aa5bbe3ad751b37f5761ef1d681f5949/Source/WebCore/accessibility/AXTextStateChangeIntent.h#L40-L41), there's a value that hasn't been taken into account (Replace) which should be 7 (AttributesChange should change to 8). Is there a reason why this value wasn't considered (perhaps it didn't exist at the time of the Chrome implementation)? If there's no compelling reason to omit that value, can I update the enum?
@dts...@chromium.org I think AXTextEditType::kInsert is the best available text edit type for this action.
Ack. Looks like you introduced increment/decrement to Command.
@nek...@chromium.org I've noticed that you implemented this enum in this CL https://chromium-review.googlesource.com/c/chromium/src/+/2424166. There's a comment asking not to renumber this enum; however, I think it's out of sync. Checking its WebKit counterpart (https://github.com/WebKit/WebKit/blob/386d6232aa5bbe3ad751b37f5761ef1d681f5949/Source/WebCore/accessibility/AXTextStateChangeIntent.h#L40-L41), there's a value that hasn't been taken into account (Replace) which should be 7 (AttributesChange should change to 8). Is there a reason why this value wasn't considered (perhaps it didn't exist at the time of the Chrome implementation)? If there's no compelling reason to omit that value, can I update the enum?
nektar@ isn't working on CHromium or at Google anymore.The internal enums (CHrome's Command mojo enum here I guess) isn't public (e.g. defined in a Mac sdk). Both webkit and blink probably pull from similar W3C specs, so the values resemble each other, but I think it's ok to have different enum values. Both webkit and blink at some point have to map these directly or indirectly to a public Mac a11y api.
Done
// If the node is an <input type="number"> (identified as a spin button) andCarlos Marcelo Barrera NolascoI haven't dug into the history of when the surrounding code landed, but
// TODO(nektar) Remove this once editing intents are implemented,
// and the actual inserted and deleted text is passed over from Blink.is really the right way to do this.
Many editing intents did land (see
u/a/mojom/ax_event_intent.mojom
u/a/ax_enums.mojom
).We should be able to do a bit better e.g. detect value or selection changes based on typing vs insertions by Blink. I don't think we pass over the inserted, deleted text though, but should at least be able to know if the text was inserted/deleted as opposed to typed.
Carlos Marcelo Barrera NolascoI've changed the fix implementation to account for event intents. With the current state of the intents, we can detect when a change in value was caused by typing (either deletion or insertion). Since the increment/decrement action doesn't have an associated intent, if the field value has changed and the intent array is empty, it means the change wasn't caused by typing.
Done
// Maps AXNode IDs to the intents associated with events in the current batch.Carlos Marcelo Barrera NolascoMaybe be a little more explicit e.g. events received in OnAccessibilityEvents? And which events e.g. AXTreeUpdate.events or generated events etc?
I've updated the comment to reflect the purpose.
Done
case AXEventGenerator::Event::VALUE_IN_TEXT_FIELD_CHANGED:I wonder if we should move the logic to AXEventGenerator. i.e. if we see an increment, decrement intent, then the generated event should be a new increment, decrement generated event?
If you agree, this could also avoid us doing more bookkeeping with active_batch_intents_ (there's some already on AXTree, AXEventGenerator).(This would be a cross platform change though, so feel free to think it through/push back/defer to another change).
Since this is a cross-platform change, I would prefer a separate change to be made. Do you agree?
if (!is_typing) {Carlos Marcelo Barrera NolascoWhich intent do we get when the value changes due to up/down arrow changing the value?
The other way we can possibly handle this is to trace the exact point where the spin button changes the value and just explicitly mark it in the node. That's likely an even more robust way to differentiate here I think.
I don't have a clear sense of what that looks like, but maybe you could consider it e.g. using intents there too.Otherwise, the current change definitely looks more reasonable than before.
There were no intents when the value changed for the text field due to increment/decrement, I've created the new intents for this scenario and used them when the increment/decrement is handled.
Done
batch_intents[event.id] = event.event_intents;Note that intents are set for *both* AXTreeUpdate and AXEvent (the two collections in a single AXUpdatesAndEvents).
You only store the latter, but we really want both. When an AXTreeUpdate gets unserialized, that is the path for the generated event. An AXEvent is the path when Blink fires an explicit event.
If the AXUpdatesAndEvents has no explicit events, then we would not actually process any of this.
Thanks for the heads up, I've updated the code.
Related details
- David Tseng
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")
case AXEventGenerator::Event::VALUE_IN_TEXT_FIELD_CHANGED:Carlos Marcelo Barrera NolascoI wonder if we should move the logic to AXEventGenerator. i.e. if we see an increment, decrement intent, then the generated event should be a new increment, decrement generated event?
If you agree, this could also avoid us doing more bookkeeping with active_batch_intents_ (there's some already on AXTree, AXEventGenerator).(This would be a cross platform change though, so feel free to think it through/push back/defer to another change).
Since this is a cross-platform change, I would prefer a separate change to be made. Do you agree?
Sure; I mean, the generated event could just be ignored or treated equall to a value in text field generated event on other platforms. It's all in if you think that approach would be cleaner and valuable for other platforms as well.
Related details
- Carlos Marcelo Barrera Nolasco
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. |
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 1 comment![]( "Open in Gerrit")
case AXEventGenerator::Event::VALUE_IN_TEXT_FIELD_CHANGED:Carlos Marcelo Barrera NolascoI wonder if we should move the logic to AXEventGenerator. i.e. if we see an increment, decrement intent, then the generated event should be a new increment, decrement generated event?
If you agree, this could also avoid us doing more bookkeeping with active_batch_intents_ (there's some already on AXTree, AXEventGenerator).(This would be a cross platform change though, so feel free to think it through/push back/defer to another change).
David TsengSince this is a cross-platform change, I would prefer a separate change to be made. Do you agree?
Sure; I mean, the generated event could just be ignored or treated equall to a value in text field generated event on other platforms. It's all in if you think that approach would be cleaner and valuable for other platforms as well.
I've moved the logic to AXEventGenerator to handle this and generate two new events one for increment and one for decrement. For other platforms I am treating the new events as value in text field changed events, so it should not be disruptive.
Related details
- David Tseng
- Devlin Cronin
- James Cook
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. |
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Devlin, would you mind taking the extensions owners review for this? I'm pretty sure this is a private API, but not 100% sure, and I don't want to get it wrong.
This comment says the API should be named private but isn't:
https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/api/_manifest_features.json;drc=dd2c073b4c6e20368c527fdeb29e3092ae959574;l=49
Carlos, if you need me for non-extensions files (which I doubt, I mostly own ash and chromeos) just add me back as a reviewer. Thanks.
Related details
- Carlos Marcelo Barrera Nolasco
- David Tseng
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 3 comments![]( "Open in Gerrit")
tree->event_data() && node->data().IsTextField()) {Can we move this entire block into
} else if (node->data().IsTextField()) {
below and integrate it there?
That way, we can see exactly how the logic resolves into value in text field changed, vs this new generated event.
AddEvent(node, Event::EDITABLE_TEXT_CHANGED);I don't think we should add this. Was it to ensure other platforms still work? The original event was value in text field changed (we can if def this for !OS_MAC specifically, or have all platforms handle the new increment/decrement events explicitly the same way as value in text field changed for now; they can adopt it in the future if needed).
I'd probably go with the latter even though it'll be a little more code.
// For spin buttons, we handle value changes specifically inIs this primarily called through the kName change in AXEventGenerator::OnStringAttributeChanged?
Did the kName actually change for you when incrementing/decrementing the spinner?
Related details
- Carlos Marcelo Barrera Nolasco
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 4 comments![]( "Open in Gerrit")
for (const auto& intent : ActiveEventIntents()) {@dts...@chromium.org This method creates an empty event_intents vector and passes it to the serialization queue, completely ignoring any active intents on the stack.
Mouse clicks on the spin button UI trigger a direct `kValue` update. This calls `MarkAXObjectDirty`, which hits this helper method. Since the helper method hardcodes an empty vector, the `ScopedBlinkAXEventIntent` is ignored.
This change is made to capture the intents from the cache.
tree->event_data() && node->data().IsTextField()) {Can we move this entire block into
} else if (node->data().IsTextField()) {
below and integrate it there?That way, we can see exactly how the logic resolves into value in text field changed, vs this new generated event.
We cannot move this code block to the `} else if (node->data().IsTextField()) {` statement because the spin button never reaches it because spin button controls return `true` to `IsRangeValueSupported()`. Instead, I moved it inside the first `if (node->data().IsRangeValueSupported())` statement, which is more appropriate.
AddEvent(node, Event::EDITABLE_TEXT_CHANGED);I don't think we should add this. Was it to ensure other platforms still work? The original event was value in text field changed (we can if def this for !OS_MAC specifically, or have all platforms handle the new increment/decrement events explicitly the same way as value in text field changed for now; they can adopt it in the future if needed).
I'd probably go with the latter even though it'll be a little more code.
This event is no longer necessary. I was using it because in the `AXEventGenerator::FireValueInTextFieldChangedEventIfNecessary` method, I had already suppressed the sending of the `EDITABLE_TEXT_CHANGED` and `VALUE_IN_TEXT_FIELD_CHANGED` events for any interaction with spin buttons. The code for that method has been updated to suppress the events only if the intents are increment or decrement, so it's no longer necessary to include it here. For other platforms I’ve already updated the `browser_accessibility_manager_*` files in order to handle the new increment/decrement events in the same way as value in text field changed.
// For spin buttons, we handle value changes specifically inIs this primarily called through the kName change in AXEventGenerator::OnStringAttributeChanged?
Did the kName actually change for you when incrementing/decrementing the spinner?
It is not being called through the kName change in `AXEventGenerator::OnStringAttributeChanged`, but by the `AXEventGenerator::OnNodeCreated` method.
The native `<input type="number"> `contains an Inner Editor and The text inside that editor is represented by a StaticText node, when the user increments or decrements the spin button, Blink destroys the internal StaticText node and creates a new one with the updated numeric value. In this case, `FireValueInTextFieldChangedEventIfNecessary` is called and the `VALUE_IN_TEXT_FIELD_CHANGED` event is added, which leads to the problem of announcing only the section of the string that changes instead of the entire value.
Related details
- David Tseng
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 4 comments![]( "Open in Gerrit")
if (intent.command != ax::mojom::Command::kInsert &&I'm not clear on this. It's also not clear this will scale well whenever we add new values to the command enum.
Do you know for sure we want to drop range events for every intent other than insert/delete?
// double-firing when child text nodes are updated.Why do we suppress almost all intents? Can we just suppress increment/decrement?
Also, can we do so for all roles?
Related details
- Carlos Marcelo Barrera Nolasco
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")
// For spin buttons, we handle value changes specifically inCarlos Marcelo Barrera NolascoIs this primarily called through the kName change in AXEventGenerator::OnStringAttributeChanged?
Did the kName actually change for you when incrementing/decrementing the spinner?
It is not being called through the kName change in `AXEventGenerator::OnStringAttributeChanged`, but by the `AXEventGenerator::OnNodeCreated` method.
The native `<input type="number"> `contains an Inner Editor and The text inside that editor is represented by a StaticText node, when the user increments or decrements the spin button, Blink destroys the internal StaticText node and creates a new one with the updated numeric value. In this case, `FireValueInTextFieldChangedEventIfNecessary` is called and the `VALUE_IN_TEXT_FIELD_CHANGED` event is added, which leads to the problem of announcing only the section of the string that changes instead of the entire value.
Ack.
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 9 comments![]( "Open in Gerrit")
for (const auto& intent : ActiveEventIntents()) {@dts...@chromium.org This method creates an empty event_intents vector and passes it to the serialization queue, completely ignoring any active intents on the stack.
Mouse clicks on the spin button UI trigger a direct `kValue` update. This calls `MarkAXObjectDirty`, which hits this helper method. Since the helper method hardcodes an empty vector, the `ScopedBlinkAXEventIntent` is ignored.
This change is made to capture the intents from the cache.
Done
tree->event_data() && node->data().IsTextField()) {Carlos Marcelo Barrera NolascoCan we move this entire block into
} else if (node->data().IsTextField()) {
below and integrate it there?That way, we can see exactly how the logic resolves into value in text field changed, vs this new generated event.
We cannot move this code block to the `} else if (node->data().IsTextField()) {` statement because the spin button never reaches it because spin button controls return `true` to `IsRangeValueSupported()`. Instead, I moved it inside the first `if (node->data().IsRangeValueSupported())` statement, which is more appropriate.
Done
Carlos Marcelo Barrera NolascoI don't think we should add this. Was it to ensure other platforms still work? The original event was value in text field changed (we can if def this for !OS_MAC specifically, or have all platforms handle the new increment/decrement events explicitly the same way as value in text field changed for now; they can adopt it in the future if needed).
I'd probably go with the latter even though it'll be a little more code.
This event is no longer necessary. I was using it because in the `AXEventGenerator::FireValueInTextFieldChangedEventIfNecessary` method, I had already suppressed the sending of the `EDITABLE_TEXT_CHANGED` and `VALUE_IN_TEXT_FIELD_CHANGED` events for any interaction with spin buttons. The code for that method has been updated to suppress the events only if the intents are increment or decrement, so it's no longer necessary to include it here. For other platforms I’ve already updated the `browser_accessibility_manager_*` files in order to handle the new increment/decrement events in the same way as value in text field changed.
Done
if (intent.command != ax::mojom::Command::kInsert &&I'm not clear on this. It's also not clear this will scale well whenever we add new values to the command enum.
Do you know for sure we want to drop range events for every intent other than insert/delete?
This if was unnecessary. Also I removed the returns from the previous if statements in order to keep the Event::RANGE_VALUE_CHANGED event.
return;nit: add the blank line below back.
Added
// double-firing when child text nodes are updated.Why do we suppress almost all intents? Can we just suppress increment/decrement?
Also, can we do so for all roles?
I've changed the implementation to suppress the VALUE_IN_TEXT_FIELD_CHANGED event just for increment/decrement, I've also removed the role filter. This is safe as spin buttons are the only elements that have those intents and is open for new elements that implements increment/decrement.
case AXEventGenerator::Event::VALUE_IN_TEXT_FIELD_CHANGED:Carlos Marcelo Barrera NolascoI wonder if we should move the logic to AXEventGenerator. i.e. if we see an increment, decrement intent, then the generated event should be a new increment, decrement generated event?
If you agree, this could also avoid us doing more bookkeeping with active_batch_intents_ (there's some already on AXTree, AXEventGenerator).(This would be a cross platform change though, so feel free to think it through/push back/defer to another change).
David TsengSince this is a cross-platform change, I would prefer a separate change to be made. Do you agree?
Carlos Marcelo Barrera NolascoSure; I mean, the generated event could just be ignored or treated equall to a value in text field generated event on other platforms. It's all in if you think that approach would be cleaner and valuable for other platforms as well.
I've moved the logic to AXEventGenerator to handle this and generate two new events one for increment and one for decrement. For other platforms I am treating the new events as value in text field changed events, so it should not be disruptive.
Done
nit: remove.
Removed
batch_intents[event.id] = event.event_intents;Carlos Marcelo Barrera NolascoNote that intents are set for *both* AXTreeUpdate and AXEvent (the two collections in a single AXUpdatesAndEvents).
You only store the latter, but we really want both. When an AXTreeUpdate gets unserialized, that is the path for the generated event. An AXEvent is the path when Blink fires an explicit event.
If the AXUpdatesAndEvents has no explicit events, then we would not actually process any of this.
Carlos Marcelo Barrera NolascoThanks for the heads up, I've updated the code.
Done
Related details
- David Tseng
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 6 comments![]( "Open in Gerrit")
Message
Thanks for working through changes; mostly looking good now.
6 comments
if (intent.command == ax::mojom::Command::kIncrement) {Since we're still playing around a bit with the naming here, do we foresee ever needing increment/decrement intents for other roles?
If so, then perhaps we don't need to restrict the generated event i.e. name it
VALUE_INCREMENTED.
If not, then perhaps we should rename the intents to
Command::kSpinButtonIncrement
and not need to check the role here.
AddEvent(node, Event::RANGE_VALUE_CHANGED);SHould this be added if we added a value in spin button incremented|decremented? Does it matter?
// descendant-driven event to avoid double-firing when child textMaybe clarify that this occurrs due to the call from on node created? (since you took the time to figure that out).
// Maps AXNode IDs to the intents associated with AXEvents fromRemove; no longer used.
Related details
- Carlos Marcelo Barrera Nolasco
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 6 comments![]( "Open in Gerrit")
if (intent.command == ax::mojom::Command::kIncrement) {Since we're still playing around a bit with the naming here, do we foresee ever needing increment/decrement intents for other roles?
If so, then perhaps we don't need to restrict the generated event i.e. name it
VALUE_INCREMENTED.If not, then perhaps we should rename the intents to
Command::kSpinButtonIncrementand not need to check the role here.
Updated
AddEvent(node, Event::RANGE_VALUE_CHANGED);SHould this be added if we added a value in spin button incremented|decremented? Does it matter?
This is not necessary for increment/decrement as they have their own intents.
// descendant-driven event to avoid double-firing when child textMaybe clarify that this occurrs due to the call from on node created? (since you took the time to figure that out).
Updated
nit: probably unnecessary addition.
Removed
// Maps AXNode IDs to the intents associated with AXEvents fromRemove; no longer used.
Removed
nit/ditto.
- David Tseng
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 voted and added 1 comment![]( "Open in Gerrit")
David Tseng added votes with a message
lgtm with a question/request.
| Code-Review | +1 |
1 comment
case ax::mojom::Command::kSpinButtonIncrement:Do we get a input type of kInsert? Is that what ultimately causes the right VoiceOver output?
Consider adding a test to make this more explicitly spelled out if possible.
Related details
- Carlos Marcelo Barrera Nolasco
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ff...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ff...@chromium.org
Reviewer source(s):
ff...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- David Baron
- Fred Shih
- James Cook
- Joey Arhar
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. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
Drive-by: this is so cool! That old behavior, ugh, this is so much better.
Related details
- Carlos Marcelo Barrera Nolasco
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Devlin, can you take this one for extensions owners? See my comment on patchset 9. Thanks.
Related details
- Carlos Marcelo Barrera Nolasco
- David Baron
- Fred Shih
- Joey Arhar
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. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
- Carlos Marcelo Barrera Nolasco
- David Baron
- Devlin Cronin
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Carlos, I asked Devlin if he could take a look for extensions, since this is an API change. I'm a junior extensions reviewer. :-)
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
`third_party/blink/renderer/core/html/forms/text_field_input_type.cc` LGTM too (although I don't think you actually needed both me and @jar...@chromium.org)
Related details
- Carlos Marcelo Barrera Nolasco
- Devlin Cronin
- Fred Shih
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
1 comment
- Carlos Marcelo Barrera Nolasco
- Fred Shih
Fred Shih (Gerrit)
Fred Shih voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Carlos Marcelo Barrera Nolasco
Code-Coverage
Code-Owners
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco added 12 comments![]( "Open in Gerrit")
Carlos Marcelo Barrera NolascoI'm afraid I cannot view the bug or the videos due to access restriction.
Having said that, did you also test to be sure that legitimate text edits and selection changes continue to be reported, such as arrowing left/right, selecting/unselecting text via keyboard navigation, using backspace/delete, typing new values, etc.?
With the change proposed by David (using intents to detect if the field value was modified by typing), we can effectively separate the two behaviors.
Using this approach, legitimate text edits fired by typing remain untouched and keep their current behavior.
Done
if (intent.command == ax::mojom::Command::kIncrement) {Carlos Marcelo Barrera NolascoSince we're still playing around a bit with the naming here, do we foresee ever needing increment/decrement intents for other roles?
If so, then perhaps we don't need to restrict the generated event i.e. name it
VALUE_INCREMENTED.If not, then perhaps we should rename the intents to
Command::kSpinButtonIncrementand not need to check the role here.
Updated
Done
if (intent.command != ax::mojom::Command::kInsert &&I'm not clear on this. It's also not clear this will scale well whenever we add new values to the command enum.
Do you know for sure we want to drop range events for every intent other than insert/delete?
Carlos Marcelo Barrera NolascoThis if was unnecessary. Also I removed the returns from the previous if statements in order to keep the Event::RANGE_VALUE_CHANGED event.
Done
Carlos Marcelo Barrera NolascoSHould this be added if we added a value in spin button incremented|decremented? Does it matter?
This is not necessary for increment/decrement as they have their own intents.
Done
// descendant-driven event to avoid double-firing when child textCarlos Marcelo Barrera NolascoMaybe clarify that this occurrs due to the call from on node created? (since you took the time to figure that out).
Updated
Done
Carlos Marcelo Barrera Nolasconit: add the blank line below back.
Added
Done
Carlos Marcelo Barrera NolascoWhy do we suppress almost all intents? Can we just suppress increment/decrement?
Also, can we do so for all roles?
I've changed the implementation to suppress the VALUE_IN_TEXT_FIELD_CHANGED event just for increment/decrement, I've also removed the role filter. This is safe as spin buttons are the only elements that have those intents and is open for new elements that implements increment/decrement.
Done
case ax::mojom::Command::kSpinButtonIncrement:Do we get a input type of kInsert? Is that what ultimately causes the right VoiceOver output?
Consider adding a test to make this more explicitly spelled out if possible.
I will add the test in a new CL
Carlos Marcelo Barrera Nolasconit: probably unnecessary addition.
Removed
Done
// Maps AXNode IDs to the intents associated with AXEvents fromCarlos Marcelo Barrera NolascoRemove; no longer used.
Removed
Done
Carlos Marcelo Barrera Nolasconit/ditto.
Removed
Done
Removed
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Carlos Marcelo Barrera Nolasco (Gerrit)
Carlos Marcelo Barrera Nolasco voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[a11y] Prevent incorrect text edit reporting for spin buttons on macOS.
When an <input type="number"> (spin button) changes value, and both
inserted and deleted text are present, VoiceOver can misinterpret this
as a text edit. This change ensures that for spin buttons, we return an
empty AXTextEdit in such cases, allowing VoiceOver to announce the full
new value rather than just the changed digits.
When a number is entered into a number field like <input type="number">
(spin button) and the user increments or decrements the value by
pressing the up/down arrow keys or clicking the field buttons, Chrome
automatically selects the entire value and updates it.
Subsequently, when communicating the value update, Chrome interprets it
as a change in the selected text and announces only the "delta." For
example, if it changes from 24 to 25, it only announces the 5 because
the 2 didn't change.
This change adds AX event intents for spin button steps and suppress
wrong notifications on Mac.
Blink now sends `kIncrement` and `kDecrement` accessibility event
intents when a spin button's value is changed via its step up/down
functionality.
[EDITED 03/27]
`AXEventGenerator` now checks for these intents and defines two new
generated events `VALUE_IN_SPIN_BUTTON_DECREMENTED` and
`VALUE_IN_SPIN_BUTTON_INCREMENTED`
If an increment or decrement event is detected,
`BrowserAccessibilityManagerMac::FireGeneratedEvent` simply ignores it,
so VoiceOver handles the value change and announces the full value. On
other platforms, if an increment or decrement event is detected, it is
treated as a regular `VALUE_IN_TEXT_FIELD_CHANGED` event (this is the
current behavior).
Legitimate text edits fired by typing remain untouched and keep their
current behavior.
Videos comparing current vs expected behavior were added in buganizer.
- M chrome/renderer/accessibility/read_anything/read_anything_app_model.cc
- M content/browser/accessibility/browser_accessibility_manager_android.cc
- M extensions/common/api/automation.webidl
- M third_party/blink/renderer/core/html/forms/text_field_input_type.cc
- M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
- M ui/accessibility/ax_enum_util.cc
- M ui/accessibility/ax_enums.mojom
- M ui/accessibility/ax_event_generator.cc
- M ui/accessibility/ax_event_generator.h
- M ui/accessibility/platform/automation/automation_api_util.cc
- M ui/accessibility/platform/ax_event_intent_mac.mm
- M ui/accessibility/platform/browser_accessibility_manager_auralinux.cc
- M ui/accessibility/platform/browser_accessibility_manager_mac.mm
- M ui/accessibility/platform/browser_accessibility_manager_win.cc
Code-Review: +1 by Devlin Cronin, +1 by Fred Shih, +1 by David Tseng, +1 by David Baron, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |