1837024 - Fuzzy hittesting breaks on LinkedIn post headers

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Morgan Reschenberg [:morgan]]

STR:

1. Visit LinkedIn.com
2. In the main feed, attempt to hittest the name of the post author with VO enabled and "VO cursor follows mouse" enabled

Expected:
The post author is read

Actual:
Nothing is read

I think this issue stems from the fact that our fuzzy hittesting algorithm only works on generic containers with one child. On LinkedIn, these generic containers can sometimes contain two children: one text leaf with whitespace, and another generic containing the text leaf we're actually targeting.

Comment 1

[Morgan Reschenberg [:morgan]]

I think this can be generalised to "we don't cope when the nested-generic chain we expect has occasional non-generic siblings".

I wonder if we can make this algorithm a bit more specific by relying on the clipped nature of these accs (the method linkedin is using to "hide" them). Right now we have a snippet that looks like this:

          if (lastMatch->Role() == roles::TEXT_CONTAINER) {
            // We've matched on a generic, we probably want its
            // inner text leaf (if one exists). Drill down through
            // subsequent generics, regardless of whether the point
            // we want is actually contained therein."
            while (lastMatch->ChildCount() == 1) {
              if (lastMatch->Role() == roles::TEXT_CONTAINER) {
                lastMatch = lastMatch->RemoteChildAt(0);
              } else {
                break;
              }
            }
            // If we failed to find a text leaf, fall back to the
            // original generic match.
            lastMatch = lastMatch->IsTextLeaf() ? lastMatch : acc;

I wonder if we'd be better off with:

           if (lastMatch->Role() == roles::TEXT_CONTAINER)  {
            // Check if this container has a clipped child.
            // This usually indicates invisible text, and we're
            // interested in returning the inner text content
            // even if it doesn't contain the point we're hittesting.
            RemoteAccessible* clippedContainer = nullptr;
            for (RemoteAccessible* child = lastMatch->RemoteFirstChild(); child; child = child->RemoteNextSibling()) {
              if (child->Role() == roles::TEXT_CONTAINER) {
                if (child->Bounds.IsEmpty()) {
                  clippedContainer = child;
                  break;
                }
              }
            }
            // If we found a clipped container, descend it in search of
            // meaningful text leaves. If we encounter multiple, or
            // if there are additional child accs, fall back to the closest
            // match.
            RemoteAccessible* maybeTextLeaf = clippedContainer;
            while (maybeTextLeaf && maybeTextLeaf->ChildCount() == 1) {
              RemoteAccessible* child = maybeTextLeaf->RemoteChildAt(0);
             // This feels like it'd be better written as a case statement, but I couldn't figure out how to do it
              if (child->Role() == roles::TEXT_CONTAINER) {
                maybeTextLeaf = child;
              } else if (child->Role() == roles::TEXT_LEAF) {
                maybeTextLeaf = child;
                break;
              } else {
                break;
              }
            }
            if (maybeTextLeaf && (maybeTextLeaf->IsTextLeaf() || maybeTextLeaf != clippedContainer)) {
             // If we found something, it's either a text leaf or a non-clipped container deeper than the
             // original match. Return it. This is allows for more fuzziness than our current code. 
              lastMatch = maybeTextLeaf;
            }
          }
          break;

This won't fix situations where subsequent TEXT_CONTAINERS have siblings, but perhaps it makes sense to add a similar sibling walk there, too?
My thinking is that if we can narrow the initial scope of this case by only doing the deep dive on containers we anticipate finding invisible text in, maybe we can be more liberal with how much walking of that subtree we do. I don't want to end up in a situation where we're doing a complete subtree walk every time we match on a generic :(

Comment 2

[James Teh [:Jamie]]

if (child->Bounds.IsEmpty()) {

Will this ever work as expected? It may not because we use ink overflow if the frame has a 0 area, so I generally see 1 x 1 or something like that in this case. But maybe this would cover the cases it needs to cover...

Comment 3

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1837024: Only do fuzzy hittesting on descendants of clipped accs r?Jamie

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2687a2f6d5d3
Only do fuzzy hittesting on descendants of clipped accs r=Jamie

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/2687a2f6d5d3

Comment 6

[James Teh [:Jamie]]

Comment on attachment 9339426 [details]
Bug 1837024: Only do fuzzy hittesting on descendants of clipped accs r?Jamie

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed to fix various accessibility regressions introduced by the Cache the World project which shipped in 115.
• User impact if declined: Needed to fix various accessibility regressions introduced by the Cache the World project which shipped in 115.
• Fix Landed on Version: 116
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Isolated to a specific case of accessibility hit testing. Covered by automated test. Baked on release since 116 without problems.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1825611

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4602d339a848

Comment 9

[Donal Meehan [:dmeehan]]

Comment on attachment 9339426 [details]
Bug 1837024: Only do fuzzy hittesting on descendants of clipped accs r?Jamie

Approved for 115.4esr.

Comment 10

[Bogdan Maris, Desktop QA]

(In reply to Morgan Reschenberg [:morgan] from comment #0)

STR:

1. Visit LinkedIn.com
2. In the main feed, attempt to hittest the name of the post author with VO enabled and "VO cursor follows mouse" enabled

Expected:
The post author is read

Actual:
Nothing is read

I used an old Nightly build from 2023-06-20 to try and reproduce the initial issue but I was only able to do so using the "Moves VoiceOver cursor" option.
I might not understand how those options work though, let me explain. I would expect that moving the mouse cursor (having "Follow VoiceOver cursor" option selected) to the post author in Linkedin will point where the VoiceOver narator should read but it does not (except using "Moves VoiceOver cursor" option). Using the tab key to navigate the VoiceOver rectangle over the autor will correctly read it in the old (affected) build.

Using the "Moves VoiceOver cursor" option I indeed can confirm that on affected build the author is not read and using latest ESR build (https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&revision=d9a7db1ad09fef191451549bacc7a2ccf9f092ec) the author is read.

Did I missed something on how I tested using the Follow VoiceOver cursor option or was I correct in using the Moves VoiceOver cursor when testing?

Comment 11

[Morgan Reschenberg [:morgan]]

(In reply to Bogdan Maris, Desktop QA from comment #10)

(In reply to Morgan Reschenberg [:morgan] from comment #0)

STR:

1. Visit LinkedIn.com
2. In the main feed, attempt to hittest the name of the post author with VO enabled and "VO cursor follows mouse" enabled

Expected:
The post author is read

Actual:
Nothing is read

I used an old Nightly build from 2023-06-20 to try and reproduce the initial issue but I was only able to do so using the "Moves VoiceOver cursor" option.
I might not understand how those options work though, let me explain. I would expect that moving the mouse cursor (having "Follow VoiceOver cursor" option selected) to the post author in Linkedin will point where the VoiceOver narator should read but it does not (except using "Moves VoiceOver cursor" option). Using the tab key to navigate the VoiceOver rectangle over the autor will correctly read it in the old (affected) build.

Using the "Moves VoiceOver cursor" option I indeed can confirm that on affected build the author is not read and using latest ESR build (https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&revision=d9a7db1ad09fef191451549bacc7a2ccf9f092ec) the author is read.

Did I missed something on how I tested using the Follow VoiceOver cursor option or was I correct in using the Moves VoiceOver cursor when testing?

Nope, that's the right way to do it :) Sorry, I got the string wrong on the name of the setting.

When you have "Follow VoiceOver cursor" selected, the mouse isn't controlling the position of the cursor -- VoiceOver is, and the mouse follows. Because this bug is a hittesting problem, the only way to reproduce it is to cause the hittesting algorithm to be used to select the node to focus.
Regular navigation with VoiceOver uses a tree traversal, not hittesting -- only the mouse does that :)

Comment 12

[Bogdan Maris, Desktop QA]

(In reply to Morgan Reschenberg [:morgan] from comment #11)

Nope, that's the right way to do it :) Sorry, I got the string wrong on the name of the setting.

When you have "Follow VoiceOver cursor" selected, the mouse isn't controlling the position of the cursor -- VoiceOver is, and the mouse follows. Because this bug is a hittesting problem, the only way to reproduce it is to cause the hittesting algorithm to be used to select the node to focus.
Regular navigation with VoiceOver uses a tree traversal, not hittesting -- only the mouse does that :)

Thanks so much for the explanation and clarification. I will go ahead and mark this as verified fixed based on the testing done in comment 10.