[webkit-reviews] review granted: [Bug 70241] AX: buttons of number type <input> controls are not fully accessible : [Attachment 111305] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 70241: AX: buttons of number type <input> controls are not fully accessible
https://bugs.webkit.org/show_bug.cgi?id=70241

Attachment 111305: patch
https://bugs.webkit.org/attachment.cgi?id=111305&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review


> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:36
> +    return adoptRef(new AccessibilitySpinButton());

Extra unneeded () in there.

> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:40
> +    : AccessibilityMockObject()

You should be able to omit this initializer.

> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:77
> +	   m_spinButtonElement->renderer()->absoluteFocusRingQuads(quads);

Incorrect indentation.

> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:109
> +    : AccessibilityMockObject()

You should be able to omit this line.

> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:115
> +    return adoptRef(new AccessibilitySpinButtonPart());

Extra unneeded () in there.

> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:126
> +    LayoutRect parentRect = parentObject()->elementRect();
> +    if (m_isIncrementor)
> +	   parentRect.setHeight(parentRect.height()/2);
> +    else {
> +	   parentRect.setY(parentRect.y() + parentRect.height()/2);	   
> +	   parentRect.setHeight(parentRect.height()/2);        
> +    }

We normally put spaces around operators like "/".

This geometry code doesn’t belong here in the accessibility object. The
knowledge of what part of the button is what should be in the renderer, not
here.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1140
> +	   tempArray = [[NSMutableArray alloc] initWithArray:attributes];
> +	   [tempArray addObject:NSAccessibilityIncrementButtonAttribute];
> +	   [tempArray addObject:NSAccessibilityDecrementButtonAttribute];
> +	   incrementorAttrs = [[NSArray alloc] initWithArray:tempArray];
> +	   [tempArray release];

This could be done by using initWithObjects: in 1 line of code instead of 5.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1573
> +	   if (toAccessibilitySpinButtonPart(m_object)->isIncrementor())
> +	       return NSAccessibilityIncrementArrowSubrole;
> +	   else
> +	       return NSAccessibilityDecrementArrowSubrole;

In WebKit coding style we avoid else after return.

> Source/WebCore/accessibility/AccessibilityObject.h:517
> +    static LayoutRect boundingBoxForQuads(RenderObject*,
Vector<FloatQuad>&);

Should be const Vector<FloatQuad>&.

> Source/WebCore/accessibility/AccessibilityMockObject.h:40
> +    virtual bool isMockObject() const { return true; }

Should be private.

> Source/WebCore/accessibility/AccessibilityObject.cpp:411
> +    const size_t n = quads.size();

No need for the const here. Normally we’d use the name “size” or “count” for
this.

> Source/WebCore/accessibility/AccessibilitySpinButton.h:36
> +    

Normally we don’t have this blank line.

> Source/WebCore/accessibility/AccessibilitySpinButton.h:49
> +    AccessibilitySpinButton();

Can this be private?

> Source/WebCore/accessibility/AccessibilityMockObject.cpp:33
>  AccessibilityMockObject::AccessibilityMockObject()
> -    : m_parent(0)
> +    : AccessibilityObject()
> +    , m_parent(0)

This change should not be necessary.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:346
> +    

Extra whitespace added. Should get rid of that.