Do not fire events in detached child frames [chromium/src : main]
Aaron Leventhal (Gerrit)
Attention is currently required from: Daniel Libby.
Aaron Leventhal would like Daniel Libby to review this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree.
Enforce tree relationship rykes via additional CHECKs/DCHECKs.
Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 217 insertions(+), 99 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Daniel Libby.
1 comment:
Patchset:
Hi @dlibby, PTAL .. this fixes the "not responding" bug.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Daniel Libby.
Aaron Leventhal has uploaded this change for review.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree.
Enforce tree relationship rykes via additional CHECKs/DCHECKs.
Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 217 insertions(+), 99 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Benjamin Beaudry (Gerrit)
Attention is currently required from: Aaron Leventhal, Daniel Libby.
3 comments:
File content/browser/accessibility/browser_accessibility_manager.h:
Patch Set #20, Line 148:
initial_treeIf this is for tests, should we put that explicitly in the name? ie. initial_tree_for_testing
File content/browser/accessibility/browser_accessibility_manager.cc:
Patch Set #20, Line 21:
#include "browser_accessibility_manager.h"nit: should this be at the top of the includes?
File content/renderer/accessibility/render_accessibility_impl.cc:
nit: remove word
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Benjamin Beaudry (Gerrit)
Attention is currently required from: Aaron Leventhal, Daniel Libby.
1 comment:
Patchset:
Just some nits, otherwise looking great!
Should this fix all known "Chrome/Edge not responding issues"?
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Aaron Leventhal, Daniel Libby.
I don't think it'll fix the legit ones where it's actually Chrome or Edge not responding. But for those usually you hear it once and then voiceover recovers.
The ones I'm thinking this will fix is where voiceover won't stop saying the "not responding" message, and 7 have to restart VO. I'm hoping that it fixes all of that kind, but who knows?
1 comment:
Patchset:
I don't think it'll fix the legit ones where it's actually Chrome or Edge not responding. But for those usually you hear it once and then voiceover recovers.
The ones I'm thinking this will fix is where voiceover won't stop saying the "not responding" message, and 7 have to restart VO. I'm hoping that it fixes all of that kind, but who knows?
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Benjamin Beaudry (Gerrit)
Attention is currently required from: Aaron Leventhal, Daniel Libby.
1 comment:
Patchset:
I don't think it'll fix the legit ones where it's actually Chrome or Edge not responding. […]
That's what I meant. Great!
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Libby (Gerrit)
Attention is currently required from: Aaron Leventhal.
Patch set 20:Code-Review +1
3 comments:
Commit Message:
Patch Set #20, Line 12:
rykesTypo? (or I'm just not familiar with this term)
Patchset:
LGTM % nits.
File content/browser/accessibility/browser_accessibility_manager.cc:
Patch Set #20, Line 323:
reachableIs this supposed to be 'unreachable'?
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Aaron Leventhal.
Thanks for working on this.
This change should be done directly within AXTree::Unserialize and conveyed through AXTreeObserver I think. Maybe we could add AXTreeObserver::OnParentConnectionChanged.
As written, it continues to diverge the various subclasses of AXTreeManager. In particular, AutomationAXTreeWrapper/BrowserAccessibilityManager.
5 comments:
File content/browser/accessibility/browser_accessibility_manager.h:
Patch Set #20, Line 148:
const ui::AXTreeUpdate& initial_tree, // initial_tree for tests.nit: don't inline comments like this especially for screen reader viewers.
Maybe do:
const ui::AXTreeUpdate& initial_tree_for_testingPatch Set #20, Line 689:
// Add parent connection if missing (!connected_to_parent_tree_node_) and nownit: maybe rephrase by removing "and now available" which adds nothing and seems confusing.
File content/browser/accessibility/browser_accessibility_manager.cc:
Patch Set #20, Line 546:
// TODO(accessibility) Change AXEventGenerator() to avoid doing any workI think we should do this now (see other comment).
Patch Set #20, Line 1757:
// Can be null if the parent manager.nit: not a sentence.
Patch Set #20, Line 1771:
if (!delegate_) // delegate_ can be null in unit tests.nit: move comment to line above and |delegate_|.
Also, it would be nice to enforce this isn't nullptr outside of unit tests.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Aaron Leventhal.
One last note (maybe we could chat about this one when you're back):
If we can enforce things on the serialization end e.g. AXTreeSource/BlinkAXTreeSource, it should be enough to fix things. Namely, we shouldn't serialize a tree until it is connected to a parent ax tree. I see you do have some of that in this patch.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())I wonder if we should also check to see if we can get the parent frame's embedding token here.
I'll have to refresh my memory here, but I believe the browser process sends to this renderer the parent ax tree id as well. Maybe we could also check for that.
With those bits of data, we should be able to reduce this patch.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Yoshisato Yanagisawa (Gerrit)
Attention is currently required from: Aaron Leventhal.
Yoshisato Yanagisawa removed null from this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree.
Enforce tree relationship rykes via additional CHECKs/DCHECKs.
Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 217 insertions(+), 99 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Yoshisato Yanagisawa (Gerrit)
Attention is currently required from: Aaron Leventhal.
1 comment:
Patchset:
MPArch CL watch rotation: I guess GetParent here is within accessibility tree and is not affected by MPArch, but I expect Adithya and Dave, who might have worked on this area before to take a look.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Nektarios Paisios (Gerrit)
Attention is currently required from: Aaron Leventhal, David Tseng.
28 comments:
Patchset:
I am in favor of adding lots of DCHECKs, especially strategically located in order to catch bugs. But in this case, if we are not serializing child trees that are not connected to parent trees, why do we need to check for anything in the first place? How could a tree that is never serialized and sent ever be disconnected?
File content/browser/accessibility/browser_accessibility_cocoa.mm:
Patch Set #20, Line 1077:
id parent_id = NSAccessibilityUnignoredAncestor(Nit: id is not an actual "id" in the accessibility meaning of the word, e.g. "AXNodeId", "AXTreeId", etc. It is a pointer to an Objective C object. Calling this variable an "..._id" is misleading I think.
How about simply "unignored_parent"?Patch Set #20, Line 1078:
_owner->PlatformGetParent()->GetNativeViewAccessible());I never understood why we have yet another definition of ignored nodes on Mac. PlatformGetParent is supposed to return the unignored parent, if not for Mac's separate meaning of "ignored nodes" found in the "accessibilityIsIgnored" method. Should we really have a different meaning of "ignored" on Mac? I believe that the implementation of accessibilityIsIgnored is trivial so as to either be incorporated into PlatformGetParent, or in the logic that builds the AX tree in the first place.
All this is for a potential followup of course. Alexander might be willing to help?My observations are the current implementation of accessibilityIsignored removes objects with the kUnknown role from the tree and objects with the ignored state. Those objects should not be in the AX tree in the first place! So, why is it needed? :-)
Secondly, it removes unlabelled images from the tree. Aren't those exactly the images we need automatic image labelling to work on? How is any user going to find such images with VoiceOvr, and then feel the need to enable automatic image labelling, when all such images are removed by us?
Questions for thought and for possible future work by Igalia maybe?Patch Set #20, Line 1079:
DCHECK(parent_id);Thanks for the DCHECK.
Patch Set #20, Line 1658:
CHECK(manager);So essentially what we are checking is whether there is a root manager. There should always be a root manager. So, how about we make this explicit:
CHECK(manager) << "There should always be a root manager whenever an object is `instanceActive`.";
File content/browser/accessibility/browser_accessibility_manager.h:
Patch Set #20, Line 148:
const ui::AXTreeUpdate& initial_tree, // initial_tree for tests.
While this is currently the case, in the new Screen AI projects we'll be creating new trees on the fly. Let's not restrict ourselves with a comment that will soon get out of date, shall we?
I know it makes understanding the current state of the code easier, but it will be wrong soon.
Look in AXTreeManagerBase, for example.Patch Set #20, Line 692:
void EnsureParentConnectionOrRoot();Nit: I don't understand the "OrRoot" part. Maybe "EnsureParentConnectionIfNotRootManager" or "NotifyParentNodeIfPresent"?
Patch Set #20, Line 699:
BrowserAccessibilityManager* GetParentManager() const;Shall we also add in AXTreeManagerBase which will eventually replace all of these managers? Hopefully at least.
File content/browser/accessibility/browser_accessibility_manager.cc:
Patch Set #20, Line 331:
const BrowserAccessibilityManager* ancestor_manager = this;Why not use a "for" loop instead of the initialization to "this", then a while loop and then another assignment inside the "while" loop? I think it will cut down the number of lines and make the code simpler?
Patch Set #20, Line 412:
CHECK_EQ(parent_manager, alt_parent_manager);How could this ever fail? GetParentManager uses the parent_tree_id to retrieve the parent manager. So, they will always be equal, isn't it?
Patch Set #20, Line 414:
BrowserAccessibility* parent_node = parent_manager->GetFromAXNode(parent);Isn't there a way to get the current manager given an AXNode in AXTreeManagerBase? If so, could you add a TODO to fix when we are refactoring the managers/?
Patch Set #20, Line 415:
DCHECK_EQ(parent_node->manager(), parent_manager);How could this ever fail? The parent node (AKA the host node) is retrieved by first retrieving the parent manager and then asking the manager for all its host nodes and getting the one that host the current tree. I can't see how a disconnect will ever happen, given that the process of retrieving the parent (host) node involves checking whether the tree it hosts equals the current (child) tree.
Patch Set #20, Line 416:
DCHECK_NE(parent_node->manager(), this);Nit: Great check, but should this check either be moved to the base class or add a TODO to do that when refactoring?
Patch Set #20, Line 441:
// Was connected, and became the new root.Double nit: This manager was previously connected to a parent manager but now became the new root manager?
Patch Set #20, Line 560:
has_parent_id ? BrowserAccessibilityManager::FromID(parent_id) : nullptr;Shouldn't FromParentId already check whether ax_tree_id is an unknown ID? It would be excessive in my opinion for all callers to have to do that. What if one forgets?
Patch Set #20, Line 562:
CHECK(!has_parent_id) << "The root frame must be parentless, root url = "How could this ever fail? GetParentId and IsRootManager put together make the DCHECK impossible to hit because IsRootManager already checks the parent ID, isn't it?
Patch Set #20, Line 568:
CHECK(parent_manager)What if they are still not connected to parent? Should they always have a parent manager?
Patch Set #20, Line 571:
CHECK(connected_to_parent_tree_node_);Is this always going to be the case?
Patch Set #20, Line 1695:
DCHECK(GetRootAsAXNode());I think this is a good DCHECK!
Patch Set #20, Line 1720:
return parent_node;Would you mind checking that this is the same code as in AXTreeManagerBase, as we'll be deleting this really soon?
Patch Set #20, Line 1734:
DCHECK(GetParentTreeID() == ui::AXTreeIDUnknown());Doesn't IsRootTree already check for that? Should it?
Patch Set #20, Line 1740:
DCHECK(!connected_to_parent_tree_node_);Should GetParentManager do the DCHECK instead?
Patch Set #20, Line 1758:
// TODO(accessibility) Determine when this can be null.How could this ever be nullptr? This is important enough not to leave for a future date to investigate. Sounds really bad.
is_connected_to_parent_ == true should not lie.
Patch Set #20, Line 1771:
if (!delegate_) // delegate_ can be null in unit tests.
nit: move comment to line above and |delegate_|. […]
Can we ensure that all unit tests do not pass a nullptr manager? Is it hard to achieve this? I don't like unit tests using a different code path from real code.
File content/browser/accessibility/browser_accessibility_manager_unittest.cc:
Patch Set #20, Line 1417:
// Link the child trees to their parents.Nit: Parent trees?
Patch Set #20, Line 1422:
BrowserAccessibilityManager::Create(parent_update, nullptr));Why nullptr? Why would delegate ever need to be nullptr?
File content/browser/accessibility/browser_accessibility_unittest.cc:
Patch Set #20, Line 887:
BrowserAccessibilityManager::Create(child_tree_update, nullptr));I would really really be happy if we can find a way to have non-nullptr delegates in tests.
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 961:
// Don't serialize child trees that without an embedding token. These areNit: Don't serialize child trees without "
Extra "that".
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Aaron Leventhal.
I'll also note that BrowserAccessibilityManager might not be catching all RFH ax tree ids transitions correctly.
Namely, see
https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/automation_internal/automation_internal_api.cc;l=178
and
https://chromium-review.googlesource.com/c/chromium/src/+/2572729
which could lead to stale trees.
I think the key here is to enforce correct cross tree connections browser side but to also figure out the root causes of the detachment and fix accordingly. e.g. is there an observation missing which should update BrowserAccessibilityManager? Is there renderer side state which should prevent serialization updates sent to the browser?
Aaron Leventhal (Gerrit)
Attention is currently required from: Aaron Leventhal.
Aaron Leventhal uploaded patch set #21 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree.
Enforce tree relationship rules via additional CHECKs/DCHECKs.
Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 217 insertions(+), 99 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Patch set 23:Commit-Queue +1
35 comments:
Commit Message:
Patch Set #20, Line 12:
rykesTypo? (or I'm just not familiar with this term)
Done
File content/browser/accessibility/browser_accessibility_cocoa.mm:
Patch Set #20, Line 1077:
id parent_id = NSAccessibilityUnignoredAncestor(
Nit: id is not an actual "id" in the accessibility meaning of the word, e.g. […]
Done
Patch Set #20, Line 1078:
_owner->PlatformGetParent()->GetNativeViewAccessible());
I never understood why we have yet another definition of ignored nodes on Mac. […]
I filed an issue for Igalia to follow up on: https://crbug.com/1339975
Patch Set #20, Line 1658:
CHECK(manager);
So essentially what we are checking is whether there is a root manager. […]
Done
File content/browser/accessibility/browser_accessibility_manager.h:
Patch Set #20, Line 148:
const ui::AXTreeUpdate& initial_tree, // initial_tree for tests.
While this is currently the case, in the new Screen AI projects we'll be creating new trees on the f […]
Resolved by removing the comment, because as Nektarios says, it may be used outside of tests soon.
Patch Set #20, Line 148:
const ui::AXTreeUpdate& initial_tree, // initial_tree for tests.
nit: don't inline comments like this especially for screen reader viewers. […]
Done
Patch Set #20, Line 148:
initial_tree
If this is for tests, should we put that explicitly in the name? ie. […]
Nektarios mentions that will not just be for tests pretty soon.
Patch Set #20, Line 689:
// Add parent connection if missing (!connected_to_parent_tree_node_) and nownit: maybe rephrase by removing "and now available" which adds nothing and seems confusing.
Done
Patch Set #20, Line 692:
void EnsureParentConnectionOrRoot();
Nit: I don't understand the "OrRoot" part. […]
Done
Patch Set #20, Line 699:
BrowserAccessibilityManager* GetParentManager() const;
Shall we also add in AXTreeManagerBase which will eventually replace all of these managers? Hopefull […]
I think that can be done as part of that code merge, as needed.
File content/browser/accessibility/browser_accessibility_manager.cc:
Patch Set #20, Line 21:
#include "browser_accessibility_manager.h"
nit: should this be at the top of the includes?
Looks like a duplicate, actually.
Patch Set #20, Line 323:
reachableIs this supposed to be 'unreachable'?
Done
Patch Set #20, Line 331:
const BrowserAccessibilityManager* ancestor_manager = this;
Why not use a "for" loop instead of the initialization to "this", then a while loop and then another […]
Doesn't seem simpler to me, keeping as is.
Patch Set #20, Line 412:
CHECK_EQ(parent_manager, alt_parent_manager);
How could this ever fail? GetParentManager uses the parent_tree_id to retrieve the parent manager. […]
Done
Patch Set #20, Line 414:
BrowserAccessibility* parent_node = parent_manager->GetFromAXNode(parent);
Isn't there a way to get the current manager given an AXNode in AXTreeManagerBase? If so, could you […]
I'm not sure what you mean here.
Patch Set #20, Line 415:
DCHECK_EQ(parent_node->manager(), parent_manager);
How could this ever fail? The parent node (AKA the host node) is retrieved by first retrieving the p […]
You're right, it can't fail currently, but who knows if the implementation elsewhere will ever change? I think DCHECKs are good to ensure callers or called code don't change in unexpected ways.
Patch Set #20, Line 416:
DCHECK_NE(parent_node->manager(), this);
Nit: Great check, but should this check either be moved to the base class or add a TODO to do that w […]
I don't want to put a TODO for everything that needs to change after a huge refactor. I think we'll have to go through everything at that time.
Patch Set #20, Line 441:
// Was connected, and became the new root.
Double nit: This manager was previously connected to a parent manager but now became the new root ma […]
Correct. I've updated the comment with your longer text.
Patch Set #20, Line 546:
// TODO(accessibility) Change AXEventGenerator() to avoid doing any workI think we should do this now (see other comment).
I looked into it and have a draft CL. It's not a simple change, because there is a circular dependency. We need to call Unserialize() to get the document parent chain, and we need the document parent chain to know whether CanFireEvents() should return false. Meanwhile, Unserialize() is what feeds AXEventGenerator. If we do it at all, we should do it in a follow-up CL.
Patch Set #20, Line 560:
has_parent_id ? BrowserAccessibilityManager::FromID(parent_id) : nullptr;
Shouldn't FromParentId already check whether ax_tree_id is an unknown ID? It would be excessive in m […]
The benefit of doing it like this is that in some cases it's expected that the tree id can be unknown, but in other cases it cannot be unknown. But your suggestion made me realize we should add a check to ::FromID() to make sure unknown is not passed.
Patch Set #20, Line 562:
CHECK(!has_parent_id) << "The root frame must be parentless, root url = "
How could this ever fail? GetParentId and IsRootManager put together make the DCHECK impossible to h […]
What if the implementation of IsRootManager() changes? I believe the idea of the checks is that local code can describe what's expected, things that are valid even if there are changes to code it calls or that call it.
Patch Set #20, Line 568:
CHECK(parent_manager)What if they are still not connected to parent? Should they always have a parent manager?
They must always have a parent manager here because CanFireEvents() returns false unless there is a path to the root. I'll add a << message.
Patch Set #20, Line 571:
CHECK(connected_to_parent_tree_node_);Is this always going to be the case?
Yes, because of CanFireEvents() check above.
Patch Set #20, Line 1720:
return parent_node;
Would you mind checking that this is the same code as in AXTreeManagerBase, as we'll be deleting thi […]
I think you mean AXTreeManagerBase::GetHostNode() ?
I checked, and it was almost identical after my change.
Patch Set #20, Line 1734:
DCHECK(GetParentTreeID() == ui::AXTreeIDUnknown());Doesn't IsRootTree already check for that? Should it?
Done
Patch Set #20, Line 1740:
DCHECK(!connected_to_parent_tree_node_);Should GetParentManager do the DCHECK instead?
Done
Patch Set #20, Line 1757:
// Can be null if the parent manager.nit: not a sentence.
Done
Patch Set #20, Line 1758:
// TODO(accessibility) Determine when this can be null.
How could this ever be nullptr? This is important enough not to leave for a future date to investiga […]
It's not that connected_to_parent_tree_node_ lies, but rather that the child tree is serialized before the parent tree. I looked into trying to enforce that the child tree is serialized last, but I don't believe there's a good way to do that, because the child tree serialization occurs in a separate process, and doesn't know whether the parent has been serialized yet.
I've added a couple of DCHECKs here, but beyond that, I think it needs to tolerate out-of-order serialization.
Patch Set #20, Line 1771:
if (!delegate_) // delegate_ can be null in unit tests.
Can we ensure that all unit tests do not pass a nullptr manager? Is it hard to achieve this? I don't […]
I agree with Nektarios that this can't happen. I've filed a bug and assigned to Igalia. https://crbug.com/1340004
File content/browser/accessibility/browser_accessibility_manager_unittest.cc:
Patch Set #20, Line 1417:
// Link the child trees to their parents.Nit: Parent trees?
Done
Patch Set #20, Line 1422:
BrowserAccessibilityManager::Create(parent_update, nullptr));Why nullptr? Why would delegate ever need to be nullptr?
When there are multiple browser managers, then our checks expect the top one to be the root. It was a bit annoying to try to fix that, so I went with the path of least resistance, since the test still tests what it is supposed to test.
File content/browser/accessibility/browser_accessibility_unittest.cc:
Patch Set #20, Line 887:
BrowserAccessibilityManager::Create(child_tree_update, nullptr));I would really really be happy if we can find a way to have non-nullptr delegates in tests.
I did work on that, and ultimately decided that it wasn't worth the extra work for now.
File content/renderer/accessibility/render_accessibility_impl.cc:
nit: remove word
Done
Patch Set #20, Line 961:
// Don't serialize child trees that without an embedding token. These are
Nit: Don't serialize child trees without " […]
Done
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())I wonder if we should also check to see if we can get the parent frame's embedding token here. […]
I looked into it and could not make that work. It is simply that the child frame is in a different process, and has no knowledge of the parent and whether it is ready to be serialized. I think that what we need to do is make the browser-side code more tolerant of out-of-order serialization, where the child is serialized first.
Open to other suggestions, but if it's going to take a long time, I would prefer to land an improvement first, and then see what we can do about improving the system at a later time.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())I looked into it and could not make that work. […]
I think it's fine but we're most likely papering over some root cause and introducing a fair bit of complexity to BAM.
IIUC, you're seeing a child tree that is detached indefinitely (meaning VoiceOver keeps getting events on a detached tree).
I think investigation should be done through the codepath
https://source.chromium.org/search?q=renderframehostimpl%20setembeddingtoken&ss=chromiumwhere the browser propagates the child tree id to a parenting tree.
Also see
RFHI::
UpdateAXTreeData
and
GetAXTreeDataAll of this is assuming RFHI does the right thing and there's some bug in the propagation or bookkeeping of the ax tree ids.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())
I think it's fine but we're most likely papering over some root cause and introducing a fair bit of […]
Hi David, the child tree is not attached indefinitely. It gets attached as soon as the parent document is serialized.
The root cause of the bug is in AppKit. Once any event is fired in a place that is not attached to the root, it triggers the bug. If we wait until all of the parent documents are attached, the bug goes away and the document eventually loads.
We had the same bug in Views, and it was fixed here:
https://chromium-review.googlesource.com/c/chromium/src/+/3595084
One of the bugs listed in the CL description is bug 1221262, which refers to getting stuck saying "busy busy busy", that was the previous text VO would speak -- "[appname] is not responding" appears to be the same issue.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal has uploaded this change for review.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree.
Enforce tree relationship rules via additional CHECKs/DCHECKs.
Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 243 insertions(+), 103 deletions(-)
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
2 comments:
Patchset:
In bug 1298082, lgrey@ thought that switching to the new Mac A11y API would also be a way to fix these types of problems.
Most of changed lines in this CL is new CHECKs/DCHECKs. Ensuring a path to the root for all objects was helpful for Blink stability, and I think it helps add more predictability to our code paths. Enforcing predictability creates less complexity, IMO.
I debugged what happens in situations where the child iframe situation is being serialized before the parent. Perhaps mcnee@ can correct me here, but I didn't see a simple way to enforce that from the Blink side.
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())
Hi David, the child tree is not attached indefinitely. […]
@l
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #29 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. This prevents a bug in
AppKit that causes VoiceOver to get stuck repeating
"not responding" messages.
Enforce tree relationship rules via additional CHECKs/DCHECKs.
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 246 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Patch set 29:Commit-Queue +1
1 comment:
Patchset:
In bug 1298082, lgrey@ thought that switching to the new Mac A11y API would also be a way to fix the […]
BTW, we don't need github.com/password_reset to see the behavior where the child frame is serialized first. That also occurs in All/DumpAccessibilityTreeTest.AccessibilityIframeTransformNestedCrossProcess/blink.
David Tseng (Gerrit)
Attention is currently required from: Benjamin Beaudry, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())Hi David, the child tree is not attached indefinitely. […]
Thanks for the additional context. Could you put that in the commit description?
A request and a comment:
Could we report this to Apple? It's not clear if this is a VoiceOver issue or an AppKit one; VoiceOver is the one doing the announcement after all.
I wonder if we're re-creating the Mac accessibility objects after the tree is attached to its parenting host node and somehow VoiceOver is seeing a stale accessibility object.
Firing events on a detached tree shouldn't really be considered busy generally, but it'd be good to understand the definition from VoiceOver folks.I'll leave the particular review details to Nektarios since he's already thoroughly gone over it. My only remaining concern is that we eventually would want to move this logic to a currently non-existent concrete base class AXTreeManager.
I can see us doing this patch just for Mac rather than for all platforms. In either case, we should ensure this logic makes it to the ui/accessibility layer.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())
Thanks for the additional context. Could you put that in the commit description? […]
Thanks. I will do follow ups with Apple to report the bug and see whether moving to the new API would help. Perhaps you're correct that it's related to a stale caching bug on their side.
I've also created CL:3731609 to show what the simplification would look like if we could enforce document serialization order from the Blink side.
IMO for simplicity we should keep the code the same on all platforms. We occasionally have bugs on Windows platforms where the virtual buffer position is lost/reset. I could imagine that this fix will fix some of our intermittent issues like that on non-Mac platforms. And, I think it's better to have similar implementations across platforms. Finally, I think it's very helpful to e able to assume that we don't have unconnected islands of nodes in the tree -- this helped fix a lot of serialization issues in Blink last year.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal would like Nektarios Paisios to review this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. This prevents a bug in
AppKit that causes VoiceOver to get stuck repeating
"not responding" messages.
Enforce tree relationship rules via additional CHECKs/DCHECKs.
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 246 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())
Thanks. […]
"And, I think it's better to have similar implementations across platforms. Finally, I think it's very helpful to e able to assume that we"
To be clear, the patch is good for non-Chrome OS on web contents. For it to generalize to other trees not based on BrowserAccessibilityManager including all of Chrome OS, views, and others, we'd want this logic bubbled up to ui/accessibility.
So, this should be an intermediate state until we can get AXTreeManager in as a base concrete class e.g.
https://chromium-review.googlesource.com/c/chromium/src/+/3646162
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
1 comment:
File content/renderer/accessibility/render_accessibility_impl.cc:
Patch Set #20, Line 966:
if (!document.GetFrame()->GetEmbeddingToken())
"And, I think it's better to have similar implementations across platforms. […]
Agreed.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #30 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. Child trees will begin firing events as soon as all of their parent documents are serialized.
This prevents a bug in AppKit that causes VoiceOver to get stuck repeating "not responding" messages. The bug goes away as long as
events are only fired on objects that are attached to the root.
We had the same bug in Views, and it was fixed here:
https://chromium-review.googlesource.com/c/chromium/src/+/3595084
One of the bugs listed in the CL description is bug 1221262, which refers to getting stuck saying "busy busy busy", that was the previous message that VoiceOver would speak -- "[appname] is not responding" appears to be the same issue.
Also enforces tree relationship rules via additional CHECKs/DCHECKs.
This CL currently applies only to non-Chrome OS web contents. For it to generalize to other trees not based on BrowserAccessibilityManager including all of Chrome OS, views, and others, we will need to bubble
the logic up to ui/accessibility (AXTreeManagerBase).
So, this should be an intermediate state until we can get AXTreeManager in as a base concrete class e.g.
https://chromium-review.googlesource.com/c/chromium/src/+/3646162
Follow-ups:
- Notify Apple of issue and workaround
- Consider not generating events when CanFireEvents() returns false.
See CL:3726240
- Look into bfcache/focus check failure
- Clarify child tree serialization rules (CL:3727378)
- Move logic to AXTreeManagerBase
- Do not use/allow null delegates in BrowserAccessibilityManager (currently a workaround for unit tests): bug 1340004
- Explore whether Mac a11y needs separate concept of ignored nodes:
bug 1339975
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 266 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #31 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. Child trees will begin firing events as soon as all of their parent documents are serialized.
This prevents a bug in AppKit that causes VoiceOver to get stuck repeating "not responding" messages. The bug goes away as long as
events are only fired on objects that are attached to the root.
We had the same bug in Views, and it was fixed in CL:3595084.
One of the bugs listed in the CL description is bug 1221262, which refers to getting stuck saying "busy busy busy", that was the previous message that VoiceOver would speak -- "[appname] is not responding" appears to be the same issue.
Also enforces tree relationship rules via additional CHECKs/DCHECKs.
This CL currently applies only to non-Chrome OS web contents. For it to generalize to other trees not based on BrowserAccessibilityManager including all of Chrome OS, views, and others, we will need to bubble
the logic up to ui/accessibility (AXTreeManagerBase).
So, this should be an intermediate state until we can get AXTreeManager in as a base concrete class e.g. CL:3646162.
Follow-ups:
- Notify Apple of issue and workaround
- Consider not generating events when CanFireEvents() returns false.
See CL:3726240
- Look into bfcache/focus check failure
- Clarify child tree serialization rules (CL:3727378)
- Move logic to AXTreeManagerBase
- Do not use/allow null delegates in BrowserAccessibilityManager (currently a workaround for unit tests): bug 1340004
- Explore whether Mac a11y needs separate concept of ignored nodes:
bug 1339975
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 264 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #32 to this change.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #33 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. Child trees will begin firing events as soon as all of their parent documents are serialized.
This prevents a bug in AppKit that causes VoiceOver to get stuck repeating "not responding" messages. The bug goes away as long as
events are only fired on objects that are attached to the root.
We had the same bug in Views, and it was fixed in CL:3595084.
One of the bugs listed in the CL description is bug 1221262, which refers to getting stuck saying "busy busy busy", that was the previous message that VoiceOver would speak -- "[appname] is not responding" appears to be the same issue.
Also enforces tree relationship rules via additional CHECKs/DCHECKs.
This CL currently applies only to non-Chrome OS web contents. For it to generalize to other trees not based on BrowserAccessibilityManager including all of Chrome OS, views, and others, we will need to bubble
the logic up to ui/accessibility (AXTreeManagerBase).
So, this should be an intermediate state until we can get AXTreeManager in as a base concrete class e.g. CL:3646162.
Follow-ups:
- Notify Apple of issue and workaround
- Consider not generating events when CanFireEvents() returns false.
See CL:3726240
- Look into bfcache/focus check failure
- Clarify child tree serialization rules (CL:3727378)
- Move logic to AXTreeManagerBase
- Do not use/allow null delegates in BrowserAccessibilityManager (currently a workaround for unit tests): crbug.com/1340004
- Explore whether Mac a11y needs separate concept of ignored nodes:
crbug.com/1339975
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 264 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #34 to this change.
Do not fire events in detached child frames
Only fire events on nodes that in documents that are attached
to the root document of the a11y tree. Child trees will begin firing events as soon as all of their parent documents are serialized.
This prevents a bug in AppKit that causes VoiceOver to get stuck repeating "not responding" messages. The bug goes away as long as
events are only fired on objects that are attached to the root.
We had the same bug in Views, and it was fixed in CL:3595084.
One of the bugs listed in the CL description is bug 1221262, which refers to getting stuck saying "busy busy busy", that was the previous message that VoiceOver would speak -- "[appname] is not responding" appears to be the same issue.
Also enforces tree relationship rules via additional CHECKs/DCHECKs.
This CL currently applies only to non-Chrome OS web contents. For it to generalize to other trees not based on BrowserAccessibilityManager including all of Chrome OS, views, and others, we will need to bubble
the logic up to ui/accessibility (AXTreeManagerBase).
So, this should be an intermediate state until we can get AXTreeManager in as a base concrete class e.g. CL:3646162.
Follow-ups:
- Notify Apple of issue and workaround
- Consider not generating events when CanFireEvents() returns false.
See CL:3726240
- Look into bfcache/focus check failure. See TODO in
BrowserAccessibilityManager::GetLastFocusedNode()
- Do not use/allow null delegates in BrowserAccessibilityManager (currently a workaround for unit tests): crbug.com/1340004
- Explore whether Mac a11y still needs separate concept of ignored nodes: crbug.com/1339975
- Clarify child tree serialization rules (CL:3727378)
- Look into whether order of iframe serialization can be enforced to be parents first. See CL:3731609.
- Move logic to AXTreeManagerBase
Bug: 1311759
Test: https://github.com/password_reset
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
8 files changed, 265 insertions(+), 103 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal has uploaded this change for review.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, David Tseng, Nektarios Paisios.
Patch set 34:Commit-Queue +1
Aaron Leventhal (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, David Tseng, Nektarios Paisios.
Aaron Leventhal uploaded patch set #36 to this change.
AX-RelNotes: potential fix for issue where VoiceOver gets stuck saying "Chrome is not responding" repeatedly.
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: I170f656523b231e1bb53ae7bce3211707d5a8adc
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_manager_win_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
9 files changed, 269 insertions(+), 104 deletions(-)
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.
Nektarios Paisios (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, David Tseng, Nektarios Paisios.
Patch set 36:Commit-Queue +2
Nektarios Paisios (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, David Tseng.
Patch set 36:Code-Review +1
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3700452
Commit-Queue: Nektarios Paisios <nek...@chromium.org>
Reviewed-by: Nektarios Paisios <nek...@chromium.org>
Reviewed-by: Daniel Libby <dli...@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1021198}
---
M content/browser/accessibility/browser_accessibility_cocoa.mm
M content/browser/accessibility/browser_accessibility_fuchsia_unittest.cc
M content/browser/accessibility/browser_accessibility_manager.cc
M content/browser/accessibility/browser_accessibility_manager.h
M content/browser/accessibility/browser_accessibility_manager_unittest.cc
M content/browser/accessibility/browser_accessibility_manager_win_unittest.cc
M content/browser/accessibility/browser_accessibility_unittest.cc
M content/renderer/accessibility/render_accessibility_impl.cc
M ui/accessibility/platform/ax_platform_node_mac.mm
9 files changed, 274 insertions(+), 104 deletions(-)
Takumi Fujimoto (Gerrit)
Takumi Fujimoto has created a revert of this change.
To view, visit change 3700452. To unsubscribe, or for help writing mail filters, visit settings.