AX: when text is auto-truncated, accessibility bounds are wrong

[chris fleizach]

When text-ellipsis is used, the AX bounds of the static text are wrong

[chris fleizach]

Created attachment 67627 [details]
patch

[chris fleizach]

To accomplish this, I added a new absoluteQuads method to RenderText that supports clippingToEllipsis. That's used in accessibility, but not elsewhere.

Also pulled out the common code to find the box of an ellipsis so it could be re-used by the new method

[chris fleizach]

<rdar://problem/8359426> AX: when text is auto-truncated, accessibility bounds are wrong

[Simon Fraser (smfr)]

Comment on attachment 67627 [details]
patch

> Index: WebCore/accessibility/AccessibilityRenderObject.cpp
> ===================================================================
> --- WebCore/accessibility/AccessibilityRenderObject.cpp	(revision 67422)
> +++ WebCore/accessibility/AccessibilityRenderObject.cpp	(working copy)
> @@ -1392,7 +1392,9 @@
>      // absoluteFocusRingQuads will query the hierarchy below this element, which for large webpages can be very slow.
>      // For a web area, which will have the most elements of any element, absoluteQuads should be used.
>      Vector<FloatQuad> quads;
> -    if (obj->isText() || isWebArea())
> +    if (obj->isText())
> +        toRenderText(obj)->absoluteQuads(quads, true);

This illustrates why we try to avoid boolean parameters; it's hard to know what 'true' means without looking at the method declaration.

> Index: WebCore/rendering/RenderText.cpp
> ===================================================================
> --- WebCore/rendering/RenderText.cpp	(revision 67421)
> +++ WebCore/rendering/RenderText.cpp	(working copy)
> @@ -310,10 +310,46 @@
>      }
>  }
>  
> +static IntRect ellipsisRectForBox(InlineTextBox* box, unsigned startPos, unsigned endPos)
> +{
> +    if (!box)
> +        return IntRect();
> +    
> +    unsigned short truncation = box->truncation();
> +    if (truncation == cNoTruncation)
> +        return IntRect();
> +    
> +    IntRect rect;
> +    if (EllipsisBox* ellipsis = box->root()->ellipsisBox()) {
> +        int ePos = min<int>(endPos - box->start(), box->len());
> +        int sPos = max<int>(startPos - box->start(), 0);

I find the ePos/sPos abbreviations a little obtuse, and I think it would make logical sense to compute start before end.

> +        // The ellipsis should be considered to be selected if the end of
> +        // the selection is past the beginning of the truncation and the
> +        // beginning of the selection is before or at the beginning of the
> +        // truncation.
> +        if (ePos >= truncation && sPos <= truncation)
> +            return ellipsis->selectionRect(0, 0);

> +void RenderText::absoluteQuads(Vector<FloatQuad>& quads, bool clipToEllipsis)

Maybe use an enum rather than bool for the param to make the call sites clearer.

> Index: LayoutTests/accessibility/ellipsis-text.html
> ===================================================================


> +            // The width of the ellipsis text should be short.
> +            document.getElementById("text-ellipsis").focus();
> +            var textContainer = accessibilityController.focusedElement;
> +            var textNode = textContainer.childAtIndex(0);
> +            shouldBe("textNode.width", "42");
> +
> +            // The width of non-ellipsis'd text should be longer.
> +            document.getElementById("text-noellipsis").focus();
> +            textContainer = accessibilityController.focusedElement;
> +            textNode = textContainer.childAtIndex(0);
> +            shouldBe("textNode.width", "375");

Hardcoding pixel widths is sensitive to font rendering differences on platforms. Is there a more robust way to do this (like comparing two strings, one truncated, and one not (but with an ellipsis in the source)?

r=me with those issues addressed.

[chris fleizach]

(In reply to comment #4)

Thanks will address all of those issues

[chris fleizach]

http://trac.webkit.org/changeset/67561

[chris fleizach]

emailed xan why this test didn't work on GTK

[WebKit Review Bot]

http://trac.webkit.org/changeset/67561 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67560
http://trac.webkit.org/changeset/67561

[Mihai Parparita]

(In reply to comment #8)
> http://trac.webkit.org/changeset/67561 might have broken GTK Linux 32-bit Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/67560
> http://trac.webkit.org/changeset/67561

Since accessibility/ellipsis-text.html failed, I assume it's this patch's fault.

[chris fleizach]

(In reply to comment #9)
> (In reply to comment #8)
> > http://trac.webkit.org/changeset/67561 might have broken GTK Linux 32-bit Release
> > The following changes are on the blame list:
> > http://trac.webkit.org/changeset/67560
> > http://trac.webkit.org/changeset/67561
> 
> Since accessibility/ellipsis-text.html failed, I assume it's this patch's fault.

I will add this test to the GTK skipped list until we figure out why it doesn't work

[chris fleizach]

http://trac.webkit.org/changeset/67563

[WebKit Review Bot]

http://trac.webkit.org/changeset/67563 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67562
http://trac.webkit.org/changeset/67563