Bug 70241 – AX: buttons of number type <input> controls are not fully accessible

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

[chris fleizach]

The arrow buttons on the number-type input control are not visible to Accessibility.

[chris fleizach]

Created attachment 111305 [details]
patch

[WebKit Review Bot]

Attachment 111305 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
	M	Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h
	M	Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp
	M	Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h
	M	Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp
	A	Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h
	M	Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp
	M	Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp
	M	Source/WebKit2/WebProcess/WebPage/WebPage.cpp
	A	Source/WebKit2/WebProcess/WebPage/LayerTreeHost.messages.in
	M	Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp
	M	Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h
	M	Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.h
	M	Source/WebKit2/WebProcess/WebPage/DrawingArea.h
	A	Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp
	M	Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h
	A	Source/WebKit2/UIProcess/LayerTreeHostProxy.h
	M	Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
	M	Source/WebKit2/UIProcess/WebPageProxy.cpp
	M	Source/WebKit2/UIProcess/DrawingAreaProxy.cpp
	A	Source/WebKit2/UIProcess/LayerTreeHostProxy.messages.in
	M	Source/WebKit2/UIProcess/DrawingAreaProxy.h
	M	Source/WebKit2/Platform/CoreIPC/MessageID.h
	M	Source/WebKit2/DerivedSources.pro
	M	Source/WebKit2/Scripts/webkit2/messages.py
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/Shared/qt/LayerTreeContextQt.cpp
	M	Source/WebKit2/Shared/WebLayerTreeInfo.cpp
	M	Source/WebKit2/Shared/LayerTreeContext.h
	M	Source/WebKit2/Shared/WebLayerTreeInfo.h
	M	Source/WebKit2/WebKit2.pro
r97639 = 6a21199564eb7173886ffb3ae167adbf7188a1d2 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 111305 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:897
>          actions = actionElementActions;

all buttons should expose the press action, not only if they have an actionElement

[Darin Adler]

Comment on attachment 111305 [details]
patch

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.

[chris fleizach]

Comment on attachment 111305 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review

>> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:126
>> +    }
> 
> 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.

Unfortunately there's no distinguishable renderer for these inner control objects. In the hit testing for this it checks if the point is below or above the midpoint. I can't find any another reference to these specific controls.
I'll add a FIXME to take care of this when it's more clear how to accomplish this

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1140
>> +        [tempArray release];
> 
> This could be done by using initWithObjects: in 1 line of code instead of 5.

We have to start with the "attributes" array, so we can't directly call initWithObjects unfortunately and get the behavior we want

[chris fleizach]

http://trac.webkit.org/changeset/97669

[chris fleizach]

(In reply to comment #6)
> http://trac.webkit.org/changeset/97669

Fixed chromium failure in

http://trac.webkit.org/changeset/97677

[Ryosuke Niwa]

It appears that this patch caused 20+ tests to crash on Windows bots:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r97675%20(17199)/results.html
http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/33254

[Ryosuke Niwa]

I'm sorry but I'm going to roll out the change for now since we're losing a significant test due to nrwt exiting early of these crashes.

[Ryosuke Niwa]

Reopen the bug because the patch was rolled out in http://trac.webkit.org/changeset/97721.

[chris fleizach]

(In reply to comment #10)
> Reopen the bug because the patch was rolled out in http://trac.webkit.org/changeset/97721.

Very strange. Not clear why this change is crashing every AX test on windows

[chris fleizach]

I will roll in this piece by piece since I don't see why the original patch would only cause crashes on windows

Part 1
http://trac.webkit.org/changeset/97753

[chris fleizach]

(In reply to comment #12)
> I will roll in this piece by piece since I don't see why the original patch would only cause crashes on windows
> 
> Part 1
> http://trac.webkit.org/changeset/97753

Part 2
http://trac.webkit.org/changeset/97755

part 3
http://trac.webkit.org/changeset/97756

Part 4
http://trac.webkit.org/changeset/97757

part 5
http://trac.webkit.org/changeset/97759

So far so good it appears

[chris fleizach]

(In reply to comment #13)
> (In reply to comment #12)

Part 6

http://trac.webkit.org/changeset/97769

I do not see any Windows related crashes this time...

[Arko Saha]

Modifying/Adding new files in WebCore.vcproj and running webkit check style on it results in 
"Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]" while adding new files in WebCore.vcporj.

Here is my observation :

In http://trac.webkit.org/changeset/97756

Index: Source/WebCore/WebCore.vcproj/WebCore.vcproj
===================================================================
--- Source/WebCore/WebCore.vcproj/WebCore.vcproj	(revision 97638)
+++ Source/WebCore/WebCore.vcproj/WebCore.vcproj	(working copy)
@@ -25221,6 +25221,13 @@
 				RelativePath="..\accessibility\AccessibilitySlider.h"
 				>
 			</File>
+                        <File
+                                RelativePath="..\accessibility\AccessibilitySpinButton.cpp"
+                                >
+                        </File>
+                        <File
+                                RelativePath="..\accessibility\AccessibilitySpinButton.h"
+                                >
 			<File
 				RelativePath="..\accessibility\AccessibilityTable.cpp"
 				>

Here closing </File> tag is missing for AccessibilitySpinButton.h.
Hence the style checker complains about everything that we add/modify in WebCore.vcproj.

Adding the required closing </File> tag resolves the issue.

[chris fleizach]

(In reply to comment #15)
> Modifying/Adding new files in WebCore.vcproj and running webkit check style on it results in 
> "Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583:  mismatched tag  [xml/syntax] [5]" while adding new files in WebCore.vcporj.
> 
> Here is my observation :

> Here closing </File> tag is missing for AccessibilitySpinButton.h.
> Hence the style checker complains about everything that we add/modify in WebCore.vcproj.
> 
> Adding the required closing </File> tag resolves the issue.

Thanks for catching that. Fixed with

http://trac.webkit.org/changeset/97857