Bug 163610 – AX: [Mac] Meter element should use AXValueDescription to descrbe the status of the value

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

Use AXValueDescription to describe the status of the value using “Optimal”, “Suboptimal” and “Critical” by aligning with UIA and MSAA see http://www.w3.org/TR/html51/semantics.html#the-meter-element

<rdar://problem/26638677>

[Nan Wang]

Created attachment 291976 [details]
patch

[chris fleizach]

Comment on attachment 291976 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:176
> +    // Only expose this when the author has explicitly specified following attributes.

specified "the" following

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:184
> +            description = "optimum";

I think we can return directly from this switch statement instead of setting a value

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:190
> +            description = "less good";

I think either this method should return the enum or it should return the localized string itself

right now it returns an intermediary form which still needs to be processed further

[Nan Wang]

Created attachment 291982 [details]
patch

updated from review and should've fixed the build failure.

[chris fleizach]

Comment on attachment 291982 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:175
> +    if (!m_renderer || !m_renderer->isMeter())

why do we only want this on COCOA? (does COCOA also mean iOS?)

[Nan Wang]

Comment on attachment 291982 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:175
>> +    if (!m_renderer || !m_renderer->isMeter())
> 
> why do we only want this on COCOA? (does COCOA also mean iOS?)

Because the localized string is in cocoa, I followed other mac only descriptions. I think it's both mac and iOS. But Meter is not currently supported on iOS. Should we wrap with platform(mac)?

[chris fleizach]

Comment on attachment 291982 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:175
>>> +    if (!m_renderer || !m_renderer->isMeter())
>> 
>> why do we only want this on COCOA? (does COCOA also mean iOS?)
> 
> Because the localized string is in cocoa, I followed other mac only descriptions. I think it's both mac and iOS. But Meter is not currently supported on iOS. Should we wrap with platform(mac)?

I think the #if FEATURE(METER) flag would handle that. so maybe we use COCOA and METER

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2620
> +        if (builder.length())

is there no better way to do this? does stringBuilder have methods for this kind of scenario?

> Source/WebCore/platform/LocalizedStrings.cpp:786
> +    if (value == "optimum")

I don't think we need to use strings here for the value. It seems like using an enum would be better. less copying required

[Nan Wang]

Comment on attachment 291982 [details]
patch

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

Will address other issues.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2620
>> +        if (builder.length())
> 
> is there no better way to do this? does stringBuilder have methods for this kind of scenario?

I don't think so. I've searched the code, we have static function shouldAddSpaceBeforeAppendingNextElement in AccessibilityNodeObject doing the length check. I think it's easier here than try to expose that function.

[Nan Wang]

Comment on attachment 291982 [details]
patch

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

>>>> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:175
>>>> +    if (!m_renderer || !m_renderer->isMeter())
>>> 
>>> why do we only want this on COCOA? (does COCOA also mean iOS?)
>> 
>> Because the localized string is in cocoa, I followed other mac only descriptions. I think it's both mac and iOS. But Meter is not currently supported on iOS. Should we wrap with platform(mac)?
> 
> I think the #if FEATURE(METER) flag would handle that. so maybe we use COCOA and METER

This #if PLATFORM(COCOA) is already in the #if ENABLE(METER_ELEMENT) wrap

[Nan Wang]

Created attachment 292028 [details]
patch

updated the function to take enum instead

[chris fleizach]

Comment on attachment 292028 [details]
patch

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

> Source/WebCore/platform/LocalizedStrings.cpp:788
> +String AXMeterGaugeRegionValueText(int value)

shouldn't int -> GaugeRegion

[Nan Wang]

Comment on attachment 292028 [details]
patch

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

>> Source/WebCore/platform/LocalizedStrings.cpp:788
>> +String AXMeterGaugeRegionValueText(int value)
> 
> shouldn't int -> GaugeRegion

I tried to include HTMLMeterElement.h in LocalizedStrings.h but it always gave me file not found error.

[Nan Wang]

Created attachment 292072 [details]
patch

Split the function so that we don't need to deal with enum and includes in LocalizedString.h

[WebKit Commit Bot]

Comment on attachment 292072 [details]
patch

Clearing flags on attachment: 292072

Committed r207540: <http://trac.webkit.org/changeset/207540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.