Bug 37275

Summary: Accessibility support for progress element
Product: WebKit Reporter: Yael <yael>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
darin: review-
Patch to addres review comments. darin: review+

Description Yael 2010-04-08 08:03:20 PDT
The new progress element should be hooked up to the accessibility framework.
Comment 1 Yael 2010-04-08 08:48:47 PDT
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.
Comment 2 Darin Adler 2010-04-08 12:25:34 PDT
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.
Comment 3 Yael 2010-04-08 13:34:34 PDT
Created attachment 52890 [details]
Patch to addres review comments.
Comment 4 Darin Adler 2010-04-09 09:22:22 PDT
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
Comment 5 Yael 2010-04-12 19:15:25 PDT
Committed r57494: <http://trac.webkit.org/changeset/57494>
Comment 6 Yael 2010-04-12 19:44:11 PDT
I missed the part of the last comment about RenderObject*. Will submit a separate patch for it.