Bug 37275 – Accessibility support for progress element

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

[Yael]

The new progress element should be hooked up to the accessibility framework.

[Yael]

Created attachment 52872 [details]
Patch

This patch implements AccessibilityProgressindicator. It was tested only on Mac OS X with Safari.
The implementation and test are heavily based on the way AccessibilitySlider was implemented.

[Darin Adler]

Comment on attachment 52872 [details]
Patch

> +bool AccessibilityProgressIndicator::accessibilityIsIgnored() const
> +{
> +    AccessibilityObjectInclusion decision = accessibilityIsIgnoredBase();
> +    if (decision == IncludeObject)
> +        return false;
> +    if (decision == IgnoreObject)
> +        return true;
> +    
> +    return false;
> +}

How about writing it like this instead?

    return accessibilityIsIgnoredBase() == IgnoreObject;

> +    // Indneterminate progress bar should return 0.

Typo: "Indneterminate"

> +HTMLProgressElement* AccessibilityProgressIndicator::element() const
> +{
> +    return static_cast<HTMLProgressElement*>(m_renderer->node());
> +}

I don't like having a risky typecast here so far away from the code that guarantees that the type is an HTMLProgressElement. Ideally the renderer itself would have a function that returns an HTMLProgressElement* or we would put both an assertion and a runtime check here.

> +    virtual ~AccessibilityProgressIndicator() { }

Should just drop this. It adds nothing.

> +    virtual AccessibilityRole roleValue() const { return ProgressIndicatorRole; }
> +
> +    virtual bool isProgressIndicator() const { return true; }
> +
> +    virtual float valueForRange() const;
> +    virtual float maxValueForRange() const;
> +    virtual float minValueForRange() const;

These member functions should all be private.

> +protected:
> +    AccessibilityProgressIndicator(RenderObject*);

This too.

[Yael]

Created attachment 52890 [details]
Patch to addres review comments.

[Darin Adler]

Comment on attachment 52890 [details]
Patch to addres review comments.

> +class AccessibilityProgressIndicator : public AccessibilityRenderObject {
> +    
> +public:
> +    static PassRefPtr<AccessibilityProgressIndicator> create(RenderObject*);
> +
> +private:
> +    virtual AccessibilityRole roleValue() const { return ProgressIndicatorRole; }
> +
> +    virtual bool isProgressIndicator() const { return true; }
> +
> +    virtual float valueForRange() const;
> +    virtual float maxValueForRange() const;
> +    virtual float minValueForRange() const;
> +
> +    AccessibilityProgressIndicator(RenderObject*);
> +
> +private:
> +    HTMLProgressElement* element() const;
> +    virtual bool accessibilityIsIgnored() const;
> +};

Extra blank line before public here. The argument type should be RenderProgress* rather than RenderObject* to make it clear it's illegal to call this with another kind of RenderObject. No need to say "private" twice. r=me

[Yael]

Committed r57494: <http://trac.webkit.org/changeset/57494>

[Yael]

I missed the part of the last comment about RenderObject*. Will submit a separate patch for it.