[webkit-reviews] review granted: [Bug 45793] AX: when text is auto-truncated, accessibility bounds are wrong : [Attachment 67627] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 45793: AX: when text is auto-truncated, accessibility bounds are wrong
https://bugs.webkit.org/show_bug.cgi?id=45793

Attachment 67627: patch
https://bugs.webkit.org/attachment.cgi?id=67627&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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.