Bug 119093 – AX: overflow bounding boxes are way too big; prevents VoiceOver taps from activating the right elements

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

If an element is contained within a RenderLayer that has a transform on it, the bounding box for accessibility is too big.

It looks like that is happening because in absoluteFocusRingQuads, the owner layer is not being used

[chris fleizach]

Created attachment 207464 [details]
patch

[chris fleizach]

Adding Simon to help with review

[chris fleizach]

rdar://13654423

[Simon Fraser (smfr)]

Comment on attachment 207464 [details]
patch

This doesn'ts seem right. absoluteFocusRingQuads() is just that: focus ring quads in absolute coordinates, so there should be no need to pass in a repaint container.

I suspect the real issue might be suggested by the comment:
    // FIXME: addFocusRingRects() needs to be passed this transform-unaware
    // localToAbsolute() offset here because RenderInline::addFocusRingRects()
    // implicitly assumes that. This doesn't work correctly with transformed
    // descendants.

[chris fleizach]

Any ideas how to do that?

[Simon Fraser (smfr)]

The coordinate mapping seems very confused here.

RenderObject::absoluteFocusRingQuads() computes a transform-unaware absolute offset for the renderer, and passes that to addFocusRingRects(). RenderInline::addFocusRingRects() uses that offset inside AbsoluteRectsGeneratorContext to push "absolute" rects onto the list, then recurs on children, but for those uses either a localToContainerPoint(, 0) offset or a relative box offset.

Finally, back out in RenderObject::absoluteFocusRingQuads() we subtract the absolute offset from each rect then map the quads back into absolute coords (respecting transforms). Very confusing and mixed up!

[Simon Fraser (smfr)]

I think we should:
1. Hack in a way to visualize these rects on screen for development/debugging.
2. Understand and clean up all the coordinate mapping in the addFocusRingRects functions.

[Lucas Forschler]

<rdar://problem/13654423>

[chris fleizach]

(In reply to comment #7)
> I think we should:
> 1. Hack in a way to visualize these rects on screen for development/debugging.
> 2. Understand and clean up all the coordinate mapping in the addFocusRingRects functions.

Does there look like a safe fix for Accessibility that can get us through this period?

[chris fleizach]

(In reply to comment #9)
> (In reply to comment #7)
> > I think we should:
> > 1. Hack in a way to visualize these rects on screen for development/debugging.
> > 2. Understand and clean up all the coordinate mapping in the addFocusRingRects functions.
> 
> Does there look like a safe fix for Accessibility that can get us through this period?

(In reply to comment #6)
> The coordinate mapping seems very confused here.
> 
> RenderObject::absoluteFocusRingQuads() computes a transform-unaware absolute offset for the renderer, and passes that to addFocusRingRects(). RenderInline::addFocusRingRects() uses that offset inside AbsoluteRectsGeneratorContext to push "absolute" rects onto the list, then recurs on children, but for those uses either a localToContainerPoint(, 0) offset or a relative box offset.
> 
> Finally, back out in RenderObject::absoluteFocusRingQuads() we subtract the absolute offset from each rect then map the quads back into absolute coords (respecting transforms). Very confusing and mixed up!

Some debugging info I learned along the way

addFocusRingRects(rects, flooredLayoutPoint(absolutePoint));

returns 4 rects in the attached case. the last one is offset again by the transformed owner layer. So it has the absolute offset built in + the absolute offset of the transformed owner layer.

in drawFocusRings (which is the other client of addFocusRingRects), the call is pretty similar except that it passes in a paintContainer (the owner layer).

[Simon Fraser (smfr)]

It's correct for drawing to use the repaint container. It does seem like the bug is double-accounting of the offset in one code path.

[Simon Fraser (smfr)]

The issue is that RenderBlock::addFocusRingRects() uses the transform-aware localToContainerPoint():

                if (box->layer()) 
                    pos = curr->localToContainerPoint(FloatPoint(), paintContainer);

but then RenderObject::absoluteFocusRingQuads() does some offsetting with a transform-unaware offset:

    FloatPoint absolutePoint = localToAbsolute();

    for (size_t i = 0; i < count; ++i) {
        IntRect rect = rects[i];
        rect.move(-absolutePoint.x(), -absolutePoint.y());
        quads.append(localToAbsoluteQuad(FloatQuad(rect)));
    }

[Beth Dakin]

This regression was caused by http://trac.webkit.org/changeset/144350

And looking at that change actually makes me think that Chris's patch might be right after all. That patch added the paintContainer parameter to addFocusRingRects(), and it made many callers send in a paintContainer, but it failed to patch this particular call site, not sure why.

[chris fleizach]

Fixed by rolling out
http://trac.webkit.org/changeset/144350