Bug 243768 – AX: role="spinbutton" elements are not adjustable on iOS

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

[Tyler Wilcock]

role="spinbutton" components are not required to have accompanying buttons to adjust the value, so they should be adjustable on their own.

Example component can be found on:

https://www.w3.org/WAI/ARIA/apg/patterns/spinbutton/

[Radar WebKit Bug Importer]

<rdar://problem/98426197>

[Tyler Wilcock]

Created attachment 461519 [details]
Patch

[Tyler Wilcock]

Created attachment 461520 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 461520 [details]
> Patch

Looks good.

--- a/LayoutTests/accessibility/spinbutton-increment-decrement.html
+++ a/LayoutTests/accessibility/spinbutton-increment-decrement.html

Should we also test that the spin button exposes the right value after increment/decrement?

[Tyler Wilcock]

Created attachment 461531 [details]
Patch

[James Craig]

Comment on attachment 461531 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1205
> +void AccessibilityNodeObject::alterStepperValue(StepAction stepAction)

How about "range" which the superclass of slider, scrollbar, and spinbutton. Note: ARIA spinbutton = Mac "stepper"

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1207
> +    if (roleValue() != AccessibilityRole::Slider && roleValue() != AccessibilityRole::SpinButton)

scrollbar too, right?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1217
>      if (!getAttribute(stepAttr).isEmpty())
> -        changeValueByStep(increase);
> +        changeValueByStep(stepAction);
>      else
> -        changeValueByPercent(increase ? 5 : -5);
> +        changeValueByPercent(stepAction == StepAction::Increment ? 5 : -5);

I think direct value changes should only happen on native form elements, never on custom ARIA fields. Does this cover all the HTML5 stepper types like <input type="number">? There are about 2 dozen input types, and several are steppers. The layout test should probably cover these too... I don't recall if the stepAttr conditional will catch all. Does this also catch the default step value when the attribute is missing like `input[type=slider]:not([step])`?

[James Craig]

> several are steppers.

Looks likeWebKit, Chrome, and Firefox implement type=number as the only stepper, too. I recalled there were more. 🤷

[Tyler Wilcock]

Created attachment 461539 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #8)
> Created attachment 461539 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

 void AccessibilityNodeObject::changeValueByPercent(float percentChange)
@@ -1321,7 +1325,7 @@ void AccessibilityNodeObject::changeValueByPercent(float percentChange)
         step = std::abs(percentChange) * (1 / percentChange);

     value += step;
-    setNodeValue(percentChange > 0, value);
+    setNodeValue(percentChange > 0 ? StepAction::Increment : StepAction::Decrement, value);
 }

This function is missing some early returns and sanity checks like percentCahnge == 0, and value between min and max... Since you're touching this, could you please add them?

--- a/LayoutTests/platform/ios/accessibility/spinbutton-increment-decrement-expected.txt
+++ a/LayoutTests/platform/ios/accessibility/spinbutton-increment-decrement-expected.txt
Interesting that the value for MacOS and iOS differ in type? int vs float?

[Tyler Wilcock]

Created attachment 461555 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #8)
> > Created attachment 461539 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> 
>  void AccessibilityNodeObject::changeValueByPercent(float percentChange)
> @@ -1321,7 +1325,7 @@ void
> AccessibilityNodeObject::changeValueByPercent(float percentChange)
>          step = std::abs(percentChange) * (1 / percentChange);
> 
>      value += step;
> -    setNodeValue(percentChange > 0, value);
> +    setNodeValue(percentChange > 0 ? StepAction::Increment :
> StepAction::Decrement, value);
>  }
> 
> This function is missing some early returns and sanity checks like
> percentCahnge == 0, and value between min and max... Since you're touching
> this, could you please add them?
I added the check for !percentChange. Investigated the prevention of setting values outside the min and max, but that will be a little more involved (because we can't just do it in this one function, or need to centralize it somewhere else) and will require it's own layout test, so I think it will be best to save that one for a separate patch.

> ---
> a/LayoutTests/platform/ios/accessibility/spinbutton-increment-decrement-
> expected.txt
> +++
> a/LayoutTests/platform/ios/accessibility/spinbutton-increment-decrement-
> expected.txt
> Interesting that the value for MacOS and iOS differ in type? int vs float?
Yeah, it's unfortunate...we should make these the same in a later patch.

[EWS]

Committed 253403@main (9a87db7810f8): <https://commits.webkit.org/253403@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461555 [details].