Bug 27993 - AXSliders are missing required attributes and actions
Summary: AXSliders are missing required attributes and actions
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
Depends on:
Reported: 2009-08-04 12:27 PDT by chris fleizach
Modified: 2009-08-04 16:05 PDT (History)
1 user (show)

See Also:

patch (14.40 KB, patch)
2009-08-04 12:33 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff
patch (18.69 KB, patch)
2009-08-04 14:29 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 2009-08-04 12:27:22 PDT
AXSliders are missing required attributes and actions.

following attribute:

and the following actions:
Comment 1 chris fleizach 2009-08-04 12:33:05 PDT
Created attachment 34079 [details]
Comment 2 Darin Adler 2009-08-04 12:45:35 PDT
Comment on attachment 34079 [details]

> +    // There's no support in ARIA to specify horizontal or vertical, so everything should be horizontal.
> +    if ((m_object->isSlider() || m_object->isProgressIndicator()) && [attributeName isEqualToString:NSAccessibilityOrientationAttribute])
> +        return NSAccessibilityHorizontalOrientationValue;

I'm not sure that's right. For <input type=range> we do know for sure whether it's horizontal or vertical, so I'm not sure why ARIA makes this less practical for that. And in the ARIA case is there a chance we could use the shape of the rendered object to guess about horizontal vs. vertical?

> +    JSStringRef action = NULL;

We use 0 for this in WebKit coding style, not NULL.

Comment 3 chris fleizach 2009-08-04 14:12:37 PDT
i'm going to re-submit the patch since the changes to calculate orientation or a little involved
Comment 4 chris fleizach 2009-08-04 14:26:37 PDT
Comment on attachment 34079 [details]

based on darin's feedback, i added a portion of new code to accurately return the orientation
Comment 5 chris fleizach 2009-08-04 14:29:29 PDT
Created attachment 34086 [details]
Comment 6 Darin Adler 2009-08-04 15:22:53 PDT
Comment on attachment 34086 [details]

> +    if (bounds.size().width() > bounds.size().height())
> +        return AccessibilityOrientationHorizontal;
> +    if (bounds.size().height() > bounds.size().width())
> +        return AccessibilityOrientationVertical;
> +    return AccessibilityOrientationUnknown;

I think it would be OK to break the tie here and use horizontal. I don't think we need a separate unknown concept since this is only a guess anyway. But if you think there's a strong-enough benefit to distinguishing unknown, then that's fine with me.

Not having unknown would also reduce the need to have a new type for orientation. We could share ScrollbarOrientation -- maybe we'd have to rename it though.

> +    ControlPart styleAppearance = style->appearance();
> +    if (styleAppearance == SliderHorizontalPart || styleAppearance == MediaSliderPart)
> +        return AccessibilityOrientationHorizontal;
> +    if (styleAppearance == SliderVerticalPart)
> +        return AccessibilityOrientationVertical;
> +    
> +    return AccessibilityOrientationUnknown;

I normally suggest using switch statements for something like this since if you don't include a default then you get a warning about any parts not listed. That way if someone adds a new part, they have to look at this code and it makes it more likely someone adding a new type of slider will look here.

> +    if ([attributeName isEqualToString:NSAccessibilityOrientationAttribute]) {
> +        AccessibilityOrientation elementOrientation = m_object->orientation();
> +        if (elementOrientation == AccessibilityOrientationVertical)
> +            return NSAccessibilityVerticalOrientationValue;
> +        if (elementOrientation == AccessibilityOrientationHorizontal)
> +            return NSAccessibilityHorizontalOrientationValue;
> +        return nil;
> +    }

Is it really OK to return nil?

> +enum AccessibilityOrientation {
> +    AccessibilityOrientationUnknown = 0,
> +    AccessibilityOrientationVertical,
> +    AccessibilityOrientationHorizontal,
> +};

The explicit "= 0" is not needed or helpful here.

Comment 7 chris fleizach 2009-08-04 15:25:18 PDT
Its ok to return nil for orientation on the mac, if not known

i'd like to keep the AccessibilityOrientation, because on the Mac they also have a Circular orientation. (for a circular slider). its possible that might come to webkit in some form

We can get rid of unknown and default to horizontal
Comment 8 chris fleizach 2009-08-04 16:05:42 PDT