Summary: | Accessibility support for progress element | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Yael
2010-04-08 08:03:20 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 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. Created attachment 52890 [details]
Patch to addres review comments.
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 Committed r57494: <http://trac.webkit.org/changeset/57494> I missed the part of the last comment about RenderObject*. Will submit a separate patch for it. |