Bug 37275 - Accessibility support for progress element
Summary: Accessibility support for progress element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-08 08:03 PDT by Yael
Modified: 2010-04-12 19:44 PDT (History)
0 users

See Also:


Attachments
Patch (19.38 KB, patch)
2010-04-08 08:48 PDT, Yael
darin: review-
Details | Formatted Diff | Diff
Patch to addres review comments. (20.56 KB, patch)
2010-04-08 13:34 PDT, Yael
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.