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

Safari says "empty movable" in its hint for interaction with a grabbed balloon #525

Open
terracoda opened this issue Aug 10, 2021 · 14 comments

Comments

@terracoda
Copy link
Contributor

In #524 (comment)

it was noted that Voice over says "You are on an empty movable" when the Yellow Balloon is grabbed.

@jessegreenberg, can we check to see if this related to the issue that is investigating why VO is not finding the accessible name on the grab button?

If it is related, maybe we can force the accessible name when the balloon is in it's grabbed state?

I can test if Firefox and Chrome to see if the Accessible name is coming through in those browsers.

@jessegreenberg
Copy link
Contributor

Correct @terracoda, this seems very related to #519, and after that issue we confirmed that when VoiceOver lands on the "Grab"button it says

"Grab Yellow Balloon, button"

And when it lands on the "draggable" it says

"Yellow Balloon, movable"

The output

"You are currently on an empty movable"

only happens when "Speak instructions for using the item in the VoiceOver cursor" option is enabled.

@jessegreenberg
Copy link
Contributor

Here is the HTML for the "grabbed" object
image

It has an aria-label (which helped force the name in #519). It also has inner content. Maybe it is "empty" because there is no HTML under the element with the application role?

@jessegreenberg
Copy link
Contributor

I tried moving the role="application" to the parent div so that it doesn't say "empty", thinking that perhaps having role="application" on an element with no HTML children would be "empty" to VoiceOver. But it did not fix it.

@jessegreenberg
Copy link
Contributor

I was sort of correct. If I change the markup to this, "empty" goes away and it just says "movable".

<div aria-roledescription="movable" role="application" tabindex="0">
  <p>Yellow Balloon</p>
</div>

Here is the patch for GrabDragInteraction

Index: js/accessibility/GrabDragInteraction.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/accessibility/GrabDragInteraction.js	(revision 7a6b0480e7b81dc39ec48dc369bd951f59df9523)
+++ js/accessibility/GrabDragInteraction.js	(date 1628625954109)
@@ -225,7 +225,8 @@
 
     // @private
     this.draggableAccessibleName = options.objectToGrabString;
-    options.draggableOptions.innerContent = this.draggableAccessibleName;
+    // options.draggableOptions.innerContent = this.draggableAccessibleName;
+    options.draggableOptions.innerContent = null;
     options.draggableOptions.ariaLabel = this.draggableAccessibleName;
 
     assert && assert( !options.grabbableOptions.accessibleName, 'GrabDragInteraction sets its own accessible name, see objectToGrabString' );
@@ -304,6 +305,11 @@
       }
     } );
 
+    this.extraLabelNode = new Node( {
+      tagName: 'p',
+      innerContent: this.draggableAccessibleName
+    } );
+
     // @private - wrap the optional onRelease in logic that is needed for the core type.
     this.onRelease = () => {
       options.onRelease && options.onRelease();
@@ -544,8 +550,13 @@
       this.node.removeAriaDescribedbyAssociation( this.descriptionAssociationObject );
     }
 
+    if ( this.node.hasChild( this.extraLabelNode ) ) {
+      this.node.removeChild( this.extraLabelNode );
+    }
+
     this.baseInteractionUpdate( this.grabbableOptions, this.listenersForDragState, this.listenersForGrabState );
 
+
     // callback on completion
     this.onGrabbable();
   }
@@ -572,6 +583,8 @@
     // turn this into a draggable in the node
     this.baseInteractionUpdate( this.draggableOptions, this.listenersForGrabState, this.listenersForDragState );
 
+    this.node.addChild( this.extraLabelNode );
+
     // callback on completion
     this.onDraggable();
   }

This is not a VoiceOver bug, the element with role=application is in fact "empty" of children. Just silly VO decided to tell the user that in my opinion.

Next questions:

  • @terracoda is this an OK change to make, or would you recommend something else?
  • If we proceed with this, it caused a new issue where the VO highlight is huge and takes up most of the screen while in the "draggable" state. We would need to investigate this. This feels like a large enough change that we would want to test all supported platforms and make sure it doesn't have negative impacts elsewhere. The code in my patch is really gross, I would like to think of an alternative. I am guessing it will take 1-4 hours, depending on the first issue. Do you think it is worth spending this time to fix?

@jessegreenberg
Copy link
Contributor

I just tried putting the role and role-description on a parent of the focusable but it said "You are on an empty group" instead.
image

@terracoda
Copy link
Contributor Author

This is interesting to say the least. It must have something to do with flow content and screen readers. Paragraph elements are expected to have content, whereas div-elements may or may not.

@terracoda
Copy link
Contributor Author

Nope, I think this has to do with the difference between flow content and phrasing content
http://www.html-5.com/definitions/flow-content.html

@terracoda
Copy link
Contributor Author

terracoda commented Aug 11, 2021

Anyways, you are right this is not a bug, and I agree that Voice Over is just not understanding that there is content in the div. Using an HTML element that is meant for phrasing content seems like a good idea, but I am not sure it is critical for this release if it causes other serious problems.

I want to be clear on the what you are asking about the HTML. Are you saying we need to choose between:

Code A sample:

<div aria-roledescription="movable" role="application" tabindex="0">
  Yellow Balloon
</div>

or code B sample:

<div aria-roledescription="movable" role="application" tabindex="0">
  <p>Yellow Balloon</p>
</div>

And that code B sample causes some big highligting issues?

@terracoda
Copy link
Contributor Author

With Code A sample, Voice Over thinks the div is empty, and in the code B sample, Voice Over recognizes the content?

Does VO say, "You are currently on a Yellow Balloon movable" in code B sample?

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Aug 11, 2021

In #525 (comment) Code A sample is what we have now and code B sample is what makes "empty" go away. For the HTML I was asking if there is different HTML structure you want to try that might be better?

Does VO say, "You are currently on a Yellow Balloon movable" in code B sample?

With sample B VoiceOver says "You are currently on a movable."

@terracoda
Copy link
Contributor Author

Ok, thanks @jessegreenberg.
Nesting a p-element inside the div-element with application role is likely better code than not using a p-tag and worth investigating, but maybe this can wait because it causes other issues with highlights.

This is a Voice Over hint, not our design. The improvement from "empty movable" to "movable" alone, is definitely better, but not worth the impact risk.

I am going to mark this as deferred for now.
Maybe we can investigate this further in Friction's grab button, as well?

@terracoda
Copy link
Contributor Author

terracoda commented Aug 11, 2021

This issue is not related to #519. I think it has to do with some subtle differences between elements meant for flow content and elements meant for phrasing content. Both P-elements and Div-elements are meant for flow content, but of the two, I think only P-elements are meant for phrasing content.

@jessegreenberg
Copy link
Contributor

Discussed with @terracoda - we agreed with the above comments and we will not work on this now. When we work on friction which also has the grab drag interaction we can revisit this. This sim will likely get released again after interactive highlights are improved and we can republish BASE after that is done.

@jessegreenberg
Copy link
Contributor

Removing my assignment until we revisit this in the next use case.

@jessegreenberg jessegreenberg removed their assignment Aug 11, 2021
@jessegreenberg jessegreenberg changed the title Not hearing accessible name "Yellow Balloon" in grabbed state in safari Safari says "empty movable" in its hint for interaction with a grabbed balloon Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants