Bug 70241 - AX: buttons of number type <input> controls are not fully accessible
Summary: AX: buttons of number type <input> controls are not fully accessible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on: 70243 70305 70308
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 08:19 PDT by chris fleizach
Modified: 2011-10-19 07:01 PDT (History)
5 users (show)

See Also:


Attachments
patch (41.00 KB, patch)
2011-10-17 13:01 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2011-10-17 08:19:26 PDT
The arrow buttons on the number-type input control are not visible to Accessibility.
Comment 1 chris fleizach 2011-10-17 13:01:50 PDT
Created attachment 111305 [details]
patch
Comment 2 WebKit Review Bot 2011-10-17 13:03:41 PDT
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﷒1﷓; 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﷒2﷓; 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﷒3﷓; 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.
Comment 3 chris fleizach 2011-10-17 13:04:05 PDT
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
Comment 4 Darin Adler 2011-10-17 13:16:30 PDT
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.
Comment 5 chris fleizach 2011-10-17 16:05:10 PDT
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
Comment 6 chris fleizach 2011-10-17 16:38:11 PDT
http://trac.webkit.org/changeset/97669
Comment 7 chris fleizach 2011-10-17 16:56:56 PDT
(In reply to comment #6)
> http://trac.webkit.org/changeset/97669

Fixed chromium failure in

http://trac.webkit.org/changeset/97677
Comment 9 Ryosuke Niwa 2011-10-17 22:44:55 PDT
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.
Comment 10 Ryosuke Niwa 2011-10-17 22:50:27 PDT
Reopen the bug because the patch was rolled out in http://trac.webkit.org/changeset/97721.
Comment 11 chris fleizach 2011-10-17 23:30:54 PDT
(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
Comment 12 chris fleizach 2011-10-18 08:22:49 PDT
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
Comment 13 chris fleizach 2011-10-18 10:02:32 PDT
(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
Comment 14 chris fleizach 2011-10-18 12:43:01 PDT
(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...
Comment 15 Arko Saha 2011-10-19 02:18:19 PDT
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.
Comment 16 chris fleizach 2011-10-19 07:01:56 PDT
(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