1825421 - [CTW] Hit testing reports Accessibles hidden by an element with no accessible

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[Nathan LaPré]

Attachment: visually_hidden_hit_testable_repro.html

STR:

1. Load the attached file with CTW enabled.
2. Hit test the area of the div <div style="height: 500px; background-color: blue;"></div>.

Expected: Hit testing won't report the accessible that's visually hidden by overflow: hidden, since it's not visible.
Actual: Hit testing reports the underlying, visually hidden accessible.

I think this problem exists because we don't have an accessible for that blue div. There are no bounds to reference in the parent process when hit testing, since there's no accessible to hang those bounds on.

Comment 1

[Morgan Reschenberg [:morgan]]

A gross but possible solution:

• When constructing the viewport cache in content, don't bail out early when we encounter a frame for which there is no corresponding accessible.
• Modify nsTArray<uint64_t> viewportCache to nsTArray<Maybe<uint64_t>> viewportCache, storing Nothing() at the indices where a frame exists in the traversal order without a corresponding accessible
• Maintain and cache nsTHashMap<size_t, nsRect> frameCache which stores the index of the "nothing" entry in viewportCache and the corresponding screen coordinates for that frame.
• Skip these entries when computing mOnScreenAccessibles in parent, because they won't change offscreen status
• When traversing the viewport cache in ChildAtPoint, lookup Nothing() entries in the frameCache and hittest those. If we get a "hit" on a non-acc, return the container (? does this seem like a sensible fallback?)

what do y'all think?

I'm not sure how we'd track mutations in non-acc frames, but ... maybe the way we're monitoring reflow right now is sufficient

Comment 2

[James Teh [:Jamie]]

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

• Modify nsTArray<uint64_t> viewportCache to nsTArray<Maybe<uint64_t>> viewportCache, storing Nothing() at the indices where a frame exists in the traversal order without a corresponding accessible

Minor optimisation: Leave it as uint64_t and use UINT_MAX for no Accessible. I think that will consume less memory.

• Maintain and cache nsTHashMap<size_t, nsRect> frameCache which stores the index of the "nothing" entry in viewportCache and the corresponding screen coordinates for that frame.

Absolute coordinates or parent relative? If the latter, relative to what parent? Absolute is probably easier, but will that get too nasty in terms of cache updates?

I worry there could be quite a lot of these frames, most of them not useful.

• When traversing the viewport cache in ChildAtPoint, lookup Nothing() entries in the frameCache and hittest those. If we get a "hit" on a non-acc, return the container (? does this seem like a sensible fallback?)

What container?

I'm not sure how we'd track mutations in non-acc frames, but ... maybe the way we're monitoring reflow right now is sufficient

I think we'd probably have to send the coords for the non-acc frames every time we update the viewport cache. Beyond that, I don't think we should have to track them?

Comment 3

[Morgan Reschenberg [:morgan]]

(In reply to James Teh [:Jamie] from comment #2)

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

• Maintain and cache nsTHashMap<size_t, nsRect> frameCache which stores the index of the "nothing" entry in viewportCache and the corresponding screen coordinates for that frame.

Absolute coordinates or parent relative? If the latter, relative to what parent? Absolute is probably easier, but will that get too nasty in terms of cache updates?

I was thinking absolute, but I don't have a guess as to how badly that'd impact performance. We could also cache them relative to the nearest acc ancestor and then pass them to BoundsWithOffset, but I dunno that that'd save us any computational effort/time.

I worry there could be quite a lot of these frames, most of them not useful.

• When traversing the viewport cache in ChildAtPoint, lookup Nothing() entries in the frameCache and hittest those. If we get a "hit" on a non-acc, return the container (? does this seem like a sensible fallback?)

What container?

the item that the hittesting call originated on

I'm not sure how we'd track mutations in non-acc frames, but ... maybe the way we're monitoring reflow right now is sufficient

I think we'd probably have to send the coords for the non-acc frames every time we update the viewport cache. Beyond that, I don't think we should have to track them?

makes sense to me

Comment 4

[James Teh [:Jamie]]

I think we update the viewport cache whenever the user scrolls anyway, so I guess absolute coordinates would be fine (performance aside).

However, I just realised that if the user doesn't scroll and only a no-acc frame moved, there would be nothing to trigger a viewport cache update, since we only trigger those for existing Accessibles.

Comment 5

[Morgan Reschenberg [:morgan]]

:nlapre did you reduce this test case from a website or did you come up with this on your own? interested to know if this exists in the wild :)

Comment 6

[Nathan LaPré]

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

:nlapre did you reduce this test case from a website or did you come up with this on your own? interested to know if this exists in the wild :)

My memory is a bit hazy, but after some archaeology I think that I noticed this issue while working on Bug 1819741, which came from the LinkedIn direct message page, namely the area below the edit box that obscures the scrollable message content. I think I found this while writing a test for that patch; something is telling me that it came from the depths of my mind. But then, that's my mind talking, so I can't say for sure.

Either way, I tried it again and can't reproduce the problem on LinkedIn. I now also can't reproduce the problem with the original test case in this bug. Seems like other changes have potentially solved the problem?

Comment 7

[James Teh [:Jamie]]

My guess is that the hit testing changes in bug 1837414 improved things here.

Closing worksforme. We can always reopen if the problem reappears.