Bug 109023 - <meter> element not exposed to accessibility
Summary: <meter> element not exposed to accessibility
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: InRadar
Depends on:
Blocks: 117650 117651
  Show dependency treegraph
 
Reported: 2013-02-06 00:37 PST by James Craig
Modified: 2013-06-14 12:49 PDT (History)
11 users (show)

See Also:


Attachments
patch (14.35 KB, patch)
2013-04-25 10:55 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (14.33 KB, patch)
2013-04-25 11:02 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (14.35 KB, patch)
2013-04-25 11:09 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (14.38 KB, patch)
2013-04-25 11:13 PDT, chris fleizach
thorton: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for landing (15.78 KB, patch)
2013-04-25 12:05 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for landing (16.33 KB, patch)
2013-04-25 14:02 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for landing (24.60 KB, patch)
2013-04-25 15:08 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2013-02-06 00:37:16 PST
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.
Comment 1 Radar WebKit Bug Importer 2013-04-15 17:07:15 PDT
<rdar://problem/13658964>
Comment 2 chris fleizach 2013-04-25 10:55:46 PDT
Created attachment 199687 [details]
patch
Comment 3 chris fleizach 2013-04-25 10:56:01 PDT
Adding Tim to help with review
Comment 4 WebKit Commit Bot 2013-04-25 10:58:32 PDT
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.
Comment 5 chris fleizach 2013-04-25 11:02:17 PDT
Created attachment 199688 [details]
patch
Comment 6 Tim Horton 2013-04-25 11:04:05 PDT
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
Comment 7 chris fleizach 2013-04-25 11:09:34 PDT
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
Comment 8 Tim Horton 2013-04-25 11:12:08 PDT
(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 :)
Comment 9 chris fleizach 2013-04-25 11:12:51 PDT
(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
Comment 10 chris fleizach 2013-04-25 11:13:39 PDT
Created attachment 199694 [details]
patch
Comment 11 Tim Horton 2013-04-25 11:18:03 PDT
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?
Comment 12 chris fleizach 2013-04-25 11:21:37 PDT
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.
Comment 13 Tim Horton 2013-04-25 11:25:32 PDT
(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.
Comment 14 chris fleizach 2013-04-25 11:30:24 PDT
(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
Comment 15 chris fleizach 2013-04-25 11:30:34 PDT
(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
Comment 16 Build Bot 2013-04-25 11:50:58 PDT
Comment on attachment 199694 [details]
patch

Attachment 199694 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/136592
Comment 17 chris fleizach 2013-04-25 12:05:20 PDT
Created attachment 199730 [details]
patch for landing
Comment 18 WebKit Commit Bot 2013-04-25 12:08:04 PDT
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.
Comment 19 Build Bot 2013-04-25 12:30:58 PDT
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
Comment 20 chris fleizach 2013-04-25 14:02:31 PDT
Created attachment 199747 [details]
patch for landing
Comment 21 Build Bot 2013-04-25 14:28:18 PDT
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
Comment 22 Tim Horton 2013-04-25 14:29:35 PDT
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
Comment 23 Build Bot 2013-04-25 14:42:03 PDT
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
Comment 24 chris fleizach 2013-04-25 15:08:02 PDT
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
Comment 25 chris fleizach 2013-04-25 16:32:46 PDT
http://trac.webkit.org/changeset/149155