Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image does not announce "disabled" #30935

Closed
amarlette opened this issue Feb 10, 2021 · 11 comments
Closed

Image does not announce "disabled" #30935

amarlette opened this issue Feb 10, 2021 · 11 comments
Labels
Accessibility Component: Image Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot.

Comments

@amarlette
Copy link

amarlette commented Feb 10, 2021

Description

When using a screen reader this component does not announce that it is disabled.

This issue is covered in-depth by parent issue #30840, please check there for more details.

React Native version:

v0.63

@huzaifaaak
Copy link

huzaifaaak commented Feb 18, 2021

@kacieb @lunaleaps Image component doesn't have a disabled prop. Does adding accessibilityState to it make's sense or Talkback should just announce the accessibilityLabel ?

Edit: I don't think accessibilityState would make sense on image. accessible={true} will make it focusable on android.

@huzaifaaak
Copy link

huzaifaaak commented Feb 23, 2021

@lunaleaps @blavalla Can you please confirm this ☝️ . My PR is almost ready for this

@blavalla
Copy link
Contributor

@huzaifaaak, just to clarify, the "disabled" field in accessibilityState isn't used to block something from becoming focusable. As you called out, setting accessible={true} (or false) is used for that purpose, and should properly work already. What "disabled" does is let the user know that this element is currently in a disabled state and can't be clicked or interacted with. They can still focus on it however and hear a description of that element. It's most commonly used for form fields that aren't enabled until some requirement has been met, such as filling out another field.

While it may be strange to commonly want to make an image "disabled", there are at least some use cases I can think of. For example, those captchas where you select "all the images that contain X". Once you select one, maybe it becomes disabled to make it clear it can no longer be selected.

The main issue here is that can currently take the accessibilityState prop, and set disabled: true on it, but this has no effect at all. In the end, if the API allows us to set this property, it should at least work as intended, even if its real-world uses are edge-cases.

@huzaifaaak
Copy link

huzaifaaak commented Feb 24, 2021

I have broken this down into two cases that announces the disabled accessibilityState:

  1. Add the ViewProps.ACCESSIBILITY_STATE in ReactImageManager.java.
  2. Make View focusable in setViewState function in BaseViewManager.java.

I think it would not be a good idea to add it in the BaseViewManager.java (Assumption). I am pausing this. If anyone want's to work on this

@amarlette amarlette added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. and removed Needs: Triage 🔍 labels Mar 5, 2021
@fabOnReact
Copy link
Contributor

I can not move the focus on the Image

CLICK TO OPEN TESTS RESULTS -

2021-03-19.13-02-58.mp4

@blavalla
Copy link
Contributor

@fabriziobertoglio1987, do you have a link to the code that rendered that image? It's possible that it had no accessibilityLabel, or was not set as "accessible", so would not get focused by default.

@fabOnReact
Copy link
Contributor

fabOnReact commented Mar 25, 2021

Thanks a lot @blavalla, I'm trying to troubleshoot this issue and also better understand both BaseViewManager and ReactAccessibilityDelegate

I'm taking the new Button functionality as a reference to understand how it should work and debug the native code.

CLICK TO OPEN TESTS RESULTS - BUTTON - ACCESSIBILITY DISABLED ANNOUNCED

TEST SCENARIO

  • The Button with accessibilityState={{disabled: true}} will announce disabled after reading the accessibilityLabel

RESULT - SUCCESSFUL

  • The screen reader will read the Accessibility Label and add the disabled at the end of this label
 <Button
  accessibilityState={{disabled: true}}
  onPress={() => onButtonPress('submitted')}
  testID="accessibilityState_button"
  color={theme.LinkColor}
  title="Submit Application"
  accessibilityLabel="Press to submit your application!"
/>
2021-03-25.12-26-20.mp4

CLICK TO OPEN TESTS RESULTS - IMAGE - ACCESSIBILITY DISABLED NOT ANNOUNCED

TEST SCENARIO

  • The image has accessibilityState={{disabled: true}} and accessibilityLabel="testing". The screen reader should announce testing disabled.

RESULT - FAILURE

  • The screen reader announces testing
<Image
  accessibilityState={{disabled: true}}
  source={fullImage}
  style={{height: 300}}
  accessibilityLabel="testing"
  accessible={true}
/>
2021-03-25.12-26-44.mp4

Currently I am investigating and debugging how ReactAndroid generates the string Press to submit your application button disabled instead of Press to submit your application.

@fabOnReact
Copy link
Contributor

The Accessibility info are initially set with the setViewState ReactProp on the View

@Override
@ReactProp(name = ViewProps.ACCESSIBILITY_STATE)
public void setViewState(@NonNull T view, @Nullable ReadableMap accessibilityState) {

The onAfterUpdateTransaction callback is used to set the ReactAccessibilityDelegate

/**
* Callback that will be triggered after all properties are updated in current update transaction
* (all @ReactProp handlers for properties updated in current transaction have been called). If
* you want to override this method you should call super.onAfterUpdateTransaction from it as the
* parent class of the ViewManager may rely on callback being executed.
*/
protected void onAfterUpdateTransaction(@NonNull T view) {}

@Override
protected void onAfterUpdateTransaction(@NonNull T view) {
super.onAfterUpdateTransaction(view);
updateViewAccessibility(view);
}

private void updateViewAccessibility(@NonNull T view) {
ReactAccessibilityDelegate.setDelegate(view);
}

ReactImageManager enhances this methdo by calling view.maybeUpdateView()

@Override
protected void onAfterUpdateTransaction(ReactImageView view) {
super.onAfterUpdateTransaction(view);
view.maybeUpdateView();
}

after this all the Accessibility functionalities are delegated to ReactAccessibilityDelegate

@Override
protected void onAfterUpdateTransaction(ReactImageView view) {
super.onAfterUpdateTransaction(view);
view.maybeUpdateView();
}

@blavalla
Copy link
Contributor

@fabriziobertoglio1987 , From an Android standpoint, the thing that matters is whether the "enabled" property of the AccessibilityNodeInfo is set to false. If it is, it will announce "disabled", if it's not, it won't. This property is set in one of two ways:

1.) Setting it on the View directly, which when the view generates the AccessibilityNodeInfo will pull its own value for enabled and populate it to be the same.
2.) Setting it on the AccessibilityNodeInfo associated with the view via something like an AccessibilityDelegate.

In React Native, we seem to be doing both of these things, and they seem to potentially conflict. I'm not certain that this conflict is the cause of the issue, but it definitely can't be helping.

In BaseViewManager, we are always setting enabled to be "true" here:

And in ReactAccessibilityDelegate we are setting it to be the value of the accessibilityState's "disabled" key here:

What that means is that if you set accessibilityState={{disabled: true}} you will end up with a View that has enabled="true" and an AccessibilityNodeInfo that has enabled="false". In theory this shouldn't cause a problem, as the AccessibilityNodeInfo should take precedence, but when these are specifically set may make a difference. You could try removing the call in BaseViewManager to see if it has any impact.

I think the more likely culprit here though is the same one that was on the <Button> component, which is that the accessibilityState prop is simply being ignored. Check out this pull request to see how it was solved for Button, as that same approach may work here as well.

#31001

@fabOnReact
Copy link
Contributor

fabOnReact commented Mar 26, 2021

Thanks @blavalla, if I comment the setEnabled line, the Button will not announce disabled.

The line is executed both with the Image and Button (I debug both).
In both cases the method is called with accessibilityState={{disabled: true}}, but the Image does not announce disabled.

Is it an issue with the ReactImageView?

public class ReactImageView extends GenericDraweeView {

The DraweeView inherits from ImageView and View

https://github.com/facebook/fresco/blob/master/drawee/src/main/java/com/facebook/drawee/view/GenericDraweeView.java

setEnabled is defined in View and overriden in each subclass

https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/view/View.java

OPEN TO SEE SOURCECODE View#setEnabled

  /**
   * Set the enabled state of this view. The interpretation of the enabled state varies by subclass.
   *
   * @param enabled True if this view is enabled, false otherwise.
   */
  @RemotableViewMethod
  public void setEnabled(boolean enabled) {
    if (enabled == isEnabled()) return;

    setFlags(enabled ? ENABLED : DISABLED, ENABLED_MASK);

    /*
     * The View most likely has to change its appearance, so refresh
     * the drawable state.
     */
    refreshDrawableState();

    // Invalidate too, since the default behavior for views is to be
    // be drawn at 50% alpha rather than to change the drawable.
    invalidate(true);

    if (!enabled) {
      cancelPendingInputEvents();
    }
  }

Is this an issue with the ShadowNode (ReactImageManager)?

The definition of ReactShadowNodeImpl

/**
* Base node class for representing virtual tree of React nodes. Shadow nodes are used primarily for
* layouting therefore it extends {@link YogaNode} to allow that. They also help with handling
* Common base subclass of {@link YogaNode} for all layout nodes for react-based view. It extends
* {@link YogaNode} by adding additional capabilities.
*
* <p>Instances of this class receive property updates from JS via @{link UIManagerModule}.
* Subclasses may use {@link #updateShadowNode} to persist some of the updated fields in the node
* instance that corresponds to a particular view type.
*
* <p>Subclasses of {@link ReactShadowNodeImpl} should be created only from {@link ViewManager} that
* corresponds to a certain type of native view. They will be updated and accessed only from JS
* thread. Subclasses of {@link ViewManager} may choose to use base class {@link
* ReactShadowNodeImpl} or custom subclass of it if necessary.
*
* <p>The primary use-case for {@link ReactShadowNodeImpl} nodes is to calculate layouting. Although
* this might be extended. For some examples please refer to ARTGroupYogaNode or ReactTextYogaNode.
*
* <p>This class allows for the native view hierarchy to not be an exact copy of the hierarchy
* received from JS by keeping track of both JS children (e.g. {@link #getChildCount()} and
* separately native children (e.g. {@link #getNativeChildCount()}). See {@link
* NativeViewHierarchyOptimizer} for more information.
*/

For example ReactPickerManager over-rides the setEnabled method.

@ReactProp(name = ViewProps.ENABLED, defaultBoolean = true)
public void setEnabled(ReactPicker view, boolean enabled) {
view.setEnabled(enabled);
}

Unluckily I don't have a solution for this problem right now, but I plan to work on other related issues and gain a better understanding of the ReactAndroid and Android relevant sourcecode. Thanks :🙏 ☮

@fabOnReact
Copy link
Contributor

fabOnReact commented Mar 26, 2021

I have broken this down into two cases that announces the disabled accessibilityState:

1. Add the `ViewProps.ACCESSIBILITY_STATE` in ReactImageManager.java.

2. Make `View` focusable in `setViewState` function in BaseViewManager.java.

I think it would not be a good idea to add it in the BaseViewManager.java (Assumption). I am pausing this. If anyone want's to work on this

CLICK TO OPEN TEST CASE

2021-03-26.18-27-36.mp4

/** View manager for AndroidViews (plain React Views). */
@ReactModule(name = ReactViewManager.REACT_CLASS)
public class ReactViewManager extends ReactClippingViewManager<ReactViewGroup> {

@ReactProp(name = "accessible")
public void setAccessible(ReactViewGroup view, boolean accessible) {
view.setFocusable(accessible);
}

adding setFocusable to ReactImageManager and passing the accessible prop to Image component fixes the issue.

definition of ShadowNodes in React Native

/**
* Base node class for representing virtual tree of React nodes. Shadow nodes are used primarily for
* layouting therefore it extends {@link YogaNode} to allow that. They also help with handling
* Common base subclass of {@link YogaNode} for all layout nodes for react-based view. It extends
* {@link YogaNode} by adding additional capabilities.
*
* <p>Instances of this class receive property updates from JS via @{link UIManagerModule}.
* Subclasses may use {@link #updateShadowNode} to persist some of the updated fields in the node
* instance that corresponds to a particular view type.
*
* <p>Subclasses of {@link ReactShadowNodeImpl} should be created only from {@link ViewManager} that
* corresponds to a certain type of native view. They will be updated and accessed only from JS
* thread. Subclasses of {@link ViewManager} may choose to use base class {@link
* ReactShadowNodeImpl} or custom subclass of it if necessary.
*
* <p>The primary use-case for {@link ReactShadowNodeImpl} nodes is to calculate layouting. Although
* this might be extended. For some examples please refer to ARTGroupYogaNode or ReactTextYogaNode.
*
* <p>This class allows for the native view hierarchy to not be an exact copy of the hierarchy
* received from JS by keeping track of both JS children (e.g. {@link #getChildCount()} and
* separately native children (e.g. {@link #getNativeChildCount()}). See {@link
* NativeViewHierarchyOptimizer} for more information.
*/

Information available in Android Open Source View Class on Focus

https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/view/View.java

 * <p>The framework will handle routine focus movement in response to user input. This includes
 * changing the focus as views are removed or hidden, or as new views become available. Views
 * indicate their willingness to take focus through the {@link #isFocusable} method. To change
 * whether a view can take focus, call {@link #setFocusable(boolean)}. When in touch mode (see notes
 * below) views indicate whether they still would like focus via {@link #isFocusableInTouchMode} and
 * can change this via {@link #setFocusableInTouchMode(boolean)}.
 *
 * <p>Focus movement is based on an algorithm which finds the nearest neighbor in a given direction.
 * In rare cases, the default algorithm may not match the intended behavior of the developer. In
 * these situations, you can provide explicit overrides by using these XML attributes in the layout
 * file:
 *
 * <pre>
 * nextFocusDown
 * nextFocusLeft
 * nextFocusRight
 * nextFocusUp
 * </pre>
 /**
  * Initializes an {@link AccessibilityNodeInfo} with information about this view. The base
  * implementation sets:
  *
  * <ul>
  *   <li>{@link AccessibilityNodeInfo#setParent(View)},
  *   <li>{@link AccessibilityNodeInfo#setBoundsInParent(Rect)},
  *   <li>{@link AccessibilityNodeInfo#setBoundsInScreen(Rect)},
  *   <li>{@link AccessibilityNodeInfo#setPackageName(CharSequence)},
  *   <li>{@link AccessibilityNodeInfo#setClassName(CharSequence)},
  *   <li>{@link AccessibilityNodeInfo#setContentDescription(CharSequence)},
  *   <li>{@link AccessibilityNodeInfo#setEnabled(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setClickable(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setFocusable(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setFocused(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setLongClickable(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setSelected(boolean)},
  *   <li>{@link AccessibilityNodeInfo#setContextClickable(boolean)}
  * </ul>
  *
  * <p>Subclasses should override this method, call the super implementation, and set additional
  * attributes.
  *
  * <p>If an {@link AccessibilityDelegate} has been specified via calling {@link
  * #setAccessibilityDelegate(AccessibilityDelegate)} its {@link
  * AccessibilityDelegate#onInitializeAccessibilityNodeInfo(View, AccessibilityNodeInfo)} is
  * responsible for handling this call.
  *
  * @param info The instance to initialize.
  */
 @CallSuper
 public void onInitializeAccessibilityNodeInfo(AccessibilityNodeInfo info) {
   if (mAccessibilityDelegate != null) {
     mAccessibilityDelegate.onInitializeAccessibilityNodeInfo(this, info);
   } else {
     onInitializeAccessibilityNodeInfoInternal(info);
   }
 }

setFocusable in Android View

  /**
   * Set whether this view can receive the focus.
   *
   * <p>Setting this to false will also ensure that this view is not focusable in touch mode.
   *
   * @param focusable If true, this view can receive the focus.
   * @see #setFocusableInTouchMode(boolean)
   * @see #setFocusable(int)
   * @attr ref android.R.styleable#View_focusable
   */
  public void setFocusable(boolean focusable) {
    setFocusable(focusable ? FOCUSABLE : NOT_FOCUSABLE);
  }

  /**
   * Sets whether this view can receive focus.
   *
   * <p>Setting this to {@link #FOCUSABLE_AUTO} tells the framework to determine focusability
   * automatically based on the view's interactivity. This is the default.
   *
   * <p>Setting this to NOT_FOCUSABLE will ensure that this view is also not focusable in touch
   * mode.
   *
   * @param focusable One of {@link #NOT_FOCUSABLE}, {@link #FOCUSABLE}, or {@link #FOCUSABLE_AUTO}.
   * @see #setFocusableInTouchMode(boolean)
   * @attr ref android.R.styleable#View_focusable
   */
  public void setFocusable(@Focusable int focusable) {
    if ((focusable & (FOCUSABLE_AUTO | FOCUSABLE)) == 0) {
      setFlags(0, FOCUSABLE_IN_TOUCH_MODE);
    }
    setFlags(focusable, FOCUSABLE_MASK);
  }

I attach a DRAFT pr to keep track of my progress. Next week I will investigate:

  • The difference in the inheritance rules of all the ShadowNodes ViewManagers on Android

Thanks a lot. :🙏 ☮

fabOnReact added a commit to fabOnReact/react-native that referenced this issue Feb 9, 2022
Adding the prop `accessible` to `ReactTextAnchorViewManager` fixes the problem for this component.
The same solution from my previous pr facebook#30935 (comment).

See test case at fabOnReact/react-native-notes#1 (comment)
fabOnReact added a commit to fabOnReact/react-native that referenced this issue Feb 9, 2022
Adding the prop `accessible` to `ReactTextAnchorViewManager` fixes the problem for this component.
The same solution from my previous pr facebook#30935 (comment).

See test case at fabOnReact/react-native-notes#1 (comment)
666647e
@facebook facebook locked as resolved and limited conversation to collaborators May 4, 2022
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility Component: Image Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants