Closed Bug 1837024 Opened 1 year ago Closed 1 year ago

Fuzzy hittesting breaks on LinkedIn post headers

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox116 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

Details

Attachments

(1 file)

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.

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 :(

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...

Severity: -- → S3
Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2687a2f6d5d3 Only do fuzzy hittesting on descendants of clipped accs r=Jamie
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
Regressions: 1840343
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: