Bug 121977 – Regression: AX: <table><caption> no longer exposed as AXTitle.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

Created attachment 212741 [details]
test case 

Regression: AX: <table><caption> no longer exposed as AXTitle.

Table caption in attached file is exposed as AXTable->AXTitle in Safari 6, but not in the WebKit nightlies.

[Radar WebKit Bug Importer]

<rdar://problem/15091139>

[Samuel White]

Created attachment 213114 [details]
Patch.

[David Kilzer (:ddkilzer)]

Comment on attachment 213114 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=213114&action=review

r- to add the missing checks and to share more code between the two methods.

> Source/WebCore/accessibility/AccessibilityTable.cpp:555
> +void AccessibilityTable::titleElementText(Vector<AccessibilityText>& textOrder) const
> +{

Seems like there are some checks missing here that appear in other methods, namely:

if (!isAccessibilityTable())
    return;

if (!m_renderer)
    return

> Source/WebCore/accessibility/AccessibilityTable.cpp:561
> +    Node* node = this->node();
> +    if (node && isHTMLTableElement(node)) {
> +        HTMLTableCaptionElement* caption = toHTMLTableElement(node)->caption();
> +        if (caption)
> +            textOrder.append(AccessibilityText(caption->innerText(), LabelByElementText));
> +    }

Instead of duplicating this code from AccessibilityTable::title(), try to share the code in AccessibilityTable::title() with this method.

Also, shouldn't you use m_renderer->node() instead of this->node() as in AccessibilityTable::title() below?

[Samuel White]

(In reply to comment #3)
> (From update of attachment 213114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213114&action=review
> 
> r- to add the missing checks and to share more code between the two methods.
> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:555
> > +void AccessibilityTable::titleElementText(Vector<AccessibilityText>& textOrder) const
> > +{
> 
> Seems like there are some checks missing here that appear in other methods, namely:
> 
> if (!isAccessibilityTable())
>     return;
This check was omitted because isAccessibilityTable always returns true if we have a caption. In hindsight, I shouldn't depend on that heuristic to remain constant so I'll add an explicit isAccessibilityTable check as requested.
> 
> if (!m_renderer)
>     return
node() is just a convenience method on AccessibilityObject that contains the above renderer check.
> 
> > Source/WebCore/accessibility/AccessibilityTable.cpp:561
> > +    Node* node = this->node();
> > +    if (node && isHTMLTableElement(node)) {
> > +        HTMLTableCaptionElement* caption = toHTMLTableElement(node)->caption();
> > +        if (caption)
> > +            textOrder.append(AccessibilityText(caption->innerText(), LabelByElementText));
> > +    }
> 
> Instead of duplicating this code from AccessibilityTable::title(), try to share the code in AccessibilityTable::title() with this method.
Line 602 of AccessibilityObject.h notes that title() will be deprecated:

// Accessibility Text - (To be deprecated).
    virtual String accessibilityDescription() const { return String(); }
    virtual String title() const { return String(); }
    virtual String helpText() const { return String(); }

In fact I believe this regression is due to us moving away from title() and over to the accessibilityText stack. Therefore, I chose to avoid it to make the refactor easier moving forward. I can leverage it for now if you think this is premature.

> 
> Also, shouldn't you use m_renderer->node() instead of this->node() as in AccessibilityTable::title() below?

Thanks for the feedback David!

[Samuel White]

Created attachment 213217 [details]
Updated patch.

Removed redundant code per review.

[Darin Adler]

Comment on attachment 213217 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=213217&action=review

Looks generally good, but with a lot of “style changes” we probably don’t want.

> Source/WebCore/accessibility/AccessibilityImageMapLink.h:64
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Why does this need to be public? It is good to have overriding functions be private, even if the function they are overriding is public. It helps us detect people calling functions directly on derived classes but paying virtual dispatch overhead. In general, most virtual function overrides should be private.

> Source/WebCore/accessibility/AccessibilityMediaControls.h:50
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Same comment.

> Source/WebCore/accessibility/AccessibilityNodeObject.h:125
> +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;

Why does this function need to be public now? It was private before.

Strange that this function is non-const, but all the other nearly identical ones are const. Not new to this patch, though.

Also, as an aside, WebKit coding style would name this function getAccessibilityText. We use the prefix "get" to mean "returns a value through an out argument". Since you didn’t add this function, that doesn’t need to be fixed in this patch.

> Source/WebCore/accessibility/AccessibilityNodeObject.h:196
> +    virtual void titleElementText(Vector<AccessibilityText>&) const;
> +    virtual void alternativeText(Vector<AccessibilityText>&) const;
> +    virtual void visibleText(Vector<AccessibilityText>&) const;
> +    virtual void helpText(Vector<AccessibilityText>&) const;

Why do these functions need to be public now? They were private before.

Why do all of these functions need to be virtual now? Only titleElementText is now being overridden, so I would suggest making only it be virtual.

> Source/WebCore/accessibility/AccessibilityTable.h:96
> +    virtual void titleElementText(Vector<AccessibilityText>&) const OVERRIDE;

Should be private, not protected.

[Samuel White]

Created attachment 213308 [details]
Updated patch.

[Samuel White]

(In reply to comment #6)
> (From update of attachment 213217 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213217&action=review
> 
> Looks generally good, but with a lot of “style changes” we probably don’t want.
> 
> > Source/WebCore/accessibility/AccessibilityImageMapLink.h:64
> > +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;
> 
> Why does this need to be public? It is good to have overriding functions be private, even if the function they are overriding is public. It helps us detect people calling functions directly on derived classes but paying virtual dispatch overhead. In general, most virtual function overrides should be private.

Fixed, thanks.

> 
> > Source/WebCore/accessibility/AccessibilityMediaControls.h:50
> > +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;
> 
> Same comment.

Same.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.h:125
> > +    virtual void accessibilityText(Vector<AccessibilityText>&) OVERRIDE;
> 
> Why does this function need to be public now? It was private before.

I suspect that subclasses will augment the results returned by accessibilityText rather than doing a complete override. However, this was just a suspicion so we shouldn't make this change until that need becomes more clear. Thanks for pointing that out.

> 
> Strange that this function is non-const, but all the other nearly identical ones are const. Not new to this patch, though.

Agreed. I made titleElementText const for this reason.

> 
> Also, as an aside, WebKit coding style would name this function getAccessibilityText. We use the prefix "get" to mean "returns a value through an out argument". Since you didn’t add this function, that doesn’t need to be fixed in this patch.

Noted. Thanks for the tip.

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.h:196
> > +    virtual void titleElementText(Vector<AccessibilityText>&) const;
> > +    virtual void alternativeText(Vector<AccessibilityText>&) const;
> > +    virtual void visibleText(Vector<AccessibilityText>&) const;
> > +    virtual void helpText(Vector<AccessibilityText>&) const;
> 
> Why do these functions need to be public now? They were private before.

Same reasoning as above. I suspect augmentation will be necessary. Reverted until needs become more clear.

> 
> Why do all of these functions need to be virtual now? Only titleElementText is now being overridden, so I would suggest making only it be virtual.

Fixed.

> 
> > Source/WebCore/accessibility/AccessibilityTable.h:96
> > +    virtual void titleElementText(Vector<AccessibilityText>&) const OVERRIDE;
> 
> Should be private, not protected.

Fixed.


Thanks for taking a look Darin.

[WebKit Commit Bot]

Comment on attachment 213308 [details]
Updated patch.

Clearing flags on attachment: 213308

Committed r156867: <http://trac.webkit.org/changeset/156867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.