Bug 37079 – Links around blocks (e.g. divs) results in too many VoiceOver call outs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Maciej Stachowiak]

Links around blocks (e.g. divs) results in too many VoiceOver call outs

[Maciej Stachowiak]

Created attachment 52520 [details]
Patch

[Maciej Stachowiak]

<rdar://problem/7234118>

Comment on attachment 52520 [details]
Patch

Beth and I looked at the patch, and we suspect the logic for Case 3 below may be wrong:

> @@ -158,8 +272,31 @@ AccessibilityObject* AccessibilityRender
>  {
>      if (!m_renderer)
>          return 0;
> +
> +    RenderObject* nextSibling = 0;
> +
> +    // Case 1: node is a block and has an inline continuation. Next sibling is the inline continuation's
> +    // first child.
> +    RenderInline* inlineContinuation;
> +    if (m_renderer->isRenderBlock() && (inlineContinuation = toRenderBlock(m_renderer)->inlineContinuation()))
> +        nextSibling = firstChildConsideringContinuation(inlineContinuation);
> +
> +    // Case 2: node has an actual next sibling
> +    else if (RenderObject* ns = m_renderer->nextSibling())
> +        nextSibling = ns;
> +
> +    // Case 3: node has no next sibling, and its parent is an inline with a continuation.
> +    else if (isInlineWithContinuation(m_renderer->parent())) {
> +        RenderObject* continuation = toRenderInline(m_renderer->parent())->continuation();
> +        
> +        // Case 3a: continuation is a block - in this case the block itself is the next sibling.
> +        if (continuation->isRenderBlock())
> +            nextSibling = continuation;
> +        // Case 3b: continuation is an inline - in this case the inline's first child is the next sibling
> +        else
> +            nextSibling = firstChildConsideringContinuation(continuation);
> +    }

Considering a case like <span id="span1">a<div>b</div></span>c<span id="span2">d</span>, if asked for the next sibling of the first RenderInline for span1, the above code will hit case 3 (because that inline doesn’t have a next sibling in its parent anonymous block), but then it will fail the condition for that case, because its parent is not an inline. Instead, shouldn’t we return the RenderInline for span2? Perhaps the way to do this is to follow the inlineContinuation chain and return the next sibling of the last renderer on the chain?

[Maciej Stachowiak]

Comment on attachment 52520 [details]
Patch

Marking this r- because upon discussion, Mitz and I concluded that there was indeed a bug.

[Maciej Stachowiak]

Created attachment 54047 [details]
Patch

[Maciej Stachowiak]

(In reply to comment #5)
> Created an attachment (id=54047) [details]
> Patch

I believe the new patch addresses Mitz's comments above.

Comment on attachment 54047 [details]
Patch

> +static inline RenderObject* lastChildConsideringContinuation(RenderObject* renderer)
> +{
> +    RenderObject* lastChild = renderer->lastChild();
> +    RenderObject* prev = renderer;

prev is unused (you just assign to it).

> +    RenderObject* cur = renderer;
> +
> +    while (cur) {
> +        prev = cur;
> +
> +        if (RenderObject* lc = cur->lastChild())
> +            lastChild = lc;
> +
> +        if (cur->isRenderInline()) {
> +            cur = toRenderInline(cur)->continuation();
> +            if (cur && cur->isRenderBlock()) 
> +                cur = toRenderBlock(cur)->inlineContinuation();
> +        } else
> +            cur = toRenderBlock(cur)->inlineContinuation();

I can’t think of a case where you couldn’t just do cur = cur->inlineContinuation() (assuming cur is a block or an inline). In other words, the first branch seems needlessly complicated. What am I missing?

>  AccessibilityObject* AccessibilityRenderObject::firstChild() const
>  {
>      if (!m_renderer)
>          return 0;
>      
>      RenderObject* firstChild = m_renderer->firstChild();
> +
> +    if (!firstChild && isInlineWithContinuation(m_renderer))
> +        firstChild = firstChildConsideringContinuation(m_renderer);

You can just use firstChildConsideringContinuation(), because it includes the above (getting firstChild() and checking if it’s 0 etc.).

> +static inline RenderObject* endOfContinuations(RenderObject* renderer)
> +{
> +    RenderObject* prev = renderer;
> +    RenderObject* cur = renderer;
> +
> +    while (cur) {
> +        prev = cur;
> +        if (cur->isRenderInline()) {
> +            cur = toRenderInline(cur)->continuation();
> +            if (cur && cur->isRenderBlock()) 
> +                cur = toRenderBlock(cur)->inlineContinuation();
> +        } else
> +            cur = toRenderBlock(cur)->inlineContinuation();

Like in lastChildConsideringContinuations, I don’t understand why the inline case can’t just use inlineContinuation().

r=beth and me

[Maciej Stachowiak]

Committed r58150: <http://trac.webkit.org/changeset/58150>