Bug 109023 – <meter> element not exposed to accessibility

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

[James Craig]

There are a number of ways to specify it. I think this should be an AXProgressIndicator, and its label/valuetext should be from the text contents.

<meter value=6 max=8>6 blocks used (out of 8 total)</meter>
<meter value=0.75><img alt="75%" src="graph75.png"></meter>
<meter min="0" max="100" value="75"></meter>
<meter min=0 max=20 value=12>12cm</meter>
<meter min=0 max=10 value=2>2cm</meter>
<meter min=0 max=20 value=12 title="centimeters">12cm</meter>
<meter min=0 max=10 value=2 title="centimeters">2cm</meter>

http://www.w3.org/html/wg/drafts/html/master/forms.html#the-meter-element

Please update the role-subrole-description.html layout test with any fixes for this bug.

[Radar WebKit Bug Importer]

<rdar://problem/13658964>

[chris fleizach]

Created attachment 199687 [details]
patch

[chris fleizach]

Adding Tim to help with review

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/meter-element-expected.txt', u'LayoutTests/accessibility/meter-element.html', u'LayoutTests/platform/mac/accessibility/role-subrole-roledescription-expected.txt', u'LayoutTests/platform/mac/accessibility/role-subrole-roledescription.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AXObjectCache.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp', u'Source/WebCore/accessibility/AccessibilityProgressIndicator.h', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp']" exit_code: 1
Source/WebCore/accessibility/AXObjectCache.cpp:364:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:101:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 11 files


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

[chris fleizach]

Created attachment 199688 [details]
patch

[Tim Horton]

Comment on attachment 199687 [details]
patch

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

> Source/WebCore/ChangeLog:5
> +

please stick the radar here

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:65
> +        return 0.0f;

I think we usually use 0 here. I see you have another 0.0f below, too.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:228
> +    // calculate some internal property (like it's description).

no apostrophe

[chris fleizach]

Created attachment 199691 [details]
patch

patch addressing Tim's comments.

I didn't change the 0.0f. It looks like there are a lot of places in the source, when a float is being returned, that 0.0f is used

[Tim Horton]

(In reply to comment #7)
> Created an attachment (id=199691) [details]
> patch
> 
> patch addressing Tim's comments.
> 
> I didn't change the 0.0f. It looks like there are a lot of places in the source, when a float is being returned, that 0.0f is used

WebKit style is actually pretty clear (http://www.webkit.org/coding/coding-style.html):

> Unless required in order to force floating point math, do not append .0, .f and .0f to floating point literals.

"return" is not a case in which you need to force floating point math, so should not be using any of those suffixes. Odd that this code got away with ignoring this part of the guide for so long.

But that's probably something to be fixed later, not in this patch :)

[chris fleizach]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=199691) [details] [details]
> > patch
> > 
> > patch addressing Tim's comments.
> > 
> > I didn't change the 0.0f. It looks like there are a lot of places in the source, when a float is being returned, that 0.0f is used
> 
> WebKit style is actually pretty clear (http://www.webkit.org/coding/coding-style.html):
> 
> > Unless required in order to force floating point math, do not append .0, .f and .0f to floating point literals.
> 
> "return" is not a case in which you need to force floating point math, so should not be using any of those suffixes. Odd that this code got away with ignoring this part of the guide for so long.
> 
> But that's probably something to be fixed later, not in this patch :)

Thanks. I'll update this patch

[chris fleizach]

Created attachment 199694 [details]
patch

[Tim Horton]

Comment on attachment 199694 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:233
> +    // If an object can't have children, then it is using this method to help
> +    // calculate some internal property (like its description).
> +    // In this case, it should check the Node level for children in case they're
> +    // not rendered (like a <meter> element).
> +    if (!firstChild && !canHaveChildren())
> +        return AccessibilityNodeObject::firstChild();
> +

Is any code anywhere ever going to even come close to assuming that AccessibilityRenderObject::firstChild() is an AccessibilityRenderObject? (and then just static_casting or something?) Can we make sure that it won't?

[chris fleizach]

Comment on attachment 199694 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:233
>> +
> 
> Is any code anywhere ever going to even come close to assuming that AccessibilityRenderObject::firstChild() is an AccessibilityRenderObject? (and then just static_casting or something?) Can we make sure that it won't?

I just went through all the firstChild() usage and all the static_cast<AXRenderObject> usage and did not see anything that casted without checking the type first. I believe the AX code is pretty good about that right now.

[Tim Horton]

(In reply to comment #12)
> (From update of attachment 199694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199694&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:233
> >> +
> > 
> > Is any code anywhere ever going to even come close to assuming that AccessibilityRenderObject::firstChild() is an AccessibilityRenderObject? (and then just static_casting or something?) Can we make sure that it won't?
> 
> I just went through all the firstChild() usage and all the static_cast<AXRenderObject> usage and did not see anything that casted without checking the type first. I believe the AX code is pretty good about that right now.

Just slightly worried that we're providing a new and unusual way for this to happen.

[chris fleizach]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 199694 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=199694&action=review
> > 
> > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:233
> > >> +
> > > 
> > > Is any code anywhere ever going to even come close to assuming that AccessibilityRenderObject::firstChild() is an AccessibilityRenderObject? (and then just static_casting or something?) Can we make sure that it won't?
> > 
> > I just went through all the firstChild() usage and all the static_cast<AXRenderObject> usage and did not see anything that casted without checking the type first. I believe the AX code is pretty good about that right now.
> 
> Just slightly worried that we're providing a new and unusual way for this to happen.

Understand your concern. I think the layout tests give us pretty good coverage over lots of bad cases, but there is some risk especially if future code makes incorrect assumptions

[chris fleizach]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 199694 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=199694&action=review
> > > 
> > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:233
> > > >> +
> > > > 
> > > > Is any code anywhere ever going to even come close to assuming that AccessibilityRenderObject::firstChild() is an AccessibilityRenderObject? (and then just static_casting or something?) Can we make sure that it won't?
> > > 
> > > I just went through all the firstChild() usage and all the static_cast<AXRenderObject> usage and did not see anything that casted without checking the type first. I believe the AX code is pretty good about that right now.
> > 
> > Just slightly worried that we're providing a new and unusual way for this to happen.
> 
> Understand your concern. I think the layout tests give us pretty good coverage over lots of bad cases, but there is some risk especially if future code makes incorrect assumptions

Thanks for the review

[Build Bot]

Comment on attachment 199694 [details]
patch

Attachment 199694 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/136592

[chris fleizach]

Created attachment 199730 [details]
patch for landing

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/meter-element-expected.txt', u'LayoutTests/accessibility/meter-element.html', u'LayoutTests/platform/mac/accessibility/role-subrole-roledescription-expected.txt', u'LayoutTests/platform/mac/accessibility/role-subrole-roledescription.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AXObjectCache.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp', u'Source/WebCore/accessibility/AccessibilityProgressIndicator.h', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp']" exit_code: 1
Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:94:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 11 files


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

[Build Bot]

Comment on attachment 199730 [details]
patch for landing

Attachment 199730 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/109221

[chris fleizach]

Created attachment 199747 [details]
patch for landing

[Build Bot]

Comment on attachment 199747 [details]
patch for landing

Attachment 199747 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/23345

[Tim Horton]

duplicate symbol __ZTVN7WebCore30AccessibilityProgressIndicatorE in:
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityProgressIndicator.o
duplicate symbol __ZNK7WebCore30AccessibilityProgressIndicator29computeAccessibilityIsIgnoredEv in:
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityProgressIndicator.o
duplicate symbol __ZNK7WebCore30AccessibilityProgressIndicator13valueForRangeEv in:
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityProgressIndicator.o
duplicate symbol __ZNK7WebCore30AccessibilityProgressIndicator16maxValueForRangeEv in:
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityProgressIndicator.o
duplicate symbol __ZNK7WebCore30AccessibilityProgressIndicator16minValueForRangeEv in:
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityAllInOne.o
    /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityProgressIndicator.o
ld: 13 duplicate symbols for architecture x86_64

[Build Bot]

Comment on attachment 199747 [details]
patch for landing

Attachment 199747 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/226015

[chris fleizach]

Created attachment 199752 [details]
patch for landing

I think this is it. Windows only builds AX files if they're in AccessibilityAllInOne.cpp
If something is in that file, it can't be part of the compile targets on mac

[chris fleizach]

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