Screen reader not reading out adjusted force if mass moved twice #146

etwa4650 · 2019-06-10T17:23:57Z

For phetsims/qa#333

Test Device

Jordan

Operating System

Mac OS 10.14.5

Browser

Safari 12.1.1

Problem Description

If either mass if moved more than once in either direction, the screen reader will read the new distance of the two masses, but it will not read out the new adjusted force.

Steps to Reproduce

1: Go to the sim, turn on Voice Over
2: Using keyboard nav, select a mass
3: Move the mass with the arrow keys at least twice in one direction

Visuals

https://drive.google.com/open?id=16wYOZK75nVWuJ9aPZKKshZ6KvugHFhld

terracoda · 2019-06-10T17:57:40Z

Nice find @etwa4650.

@zepumph, I can reproduce this, and after 1 step VoiceOver recovers, and also three quick steps does not seem to cause the alerts to stop. I have no idea why the alerts would stop only after two quick steps.

Any ideas?

terracoda · 2019-06-10T18:01:50Z

This does not happen on the Mass number-spinner-sliders.

terracoda · 2019-06-10T22:10:31Z

@jessegreenberg and @zepumph, just an FYI that this might not be a new bug. VoiceOver recovers quite easily, so it is not something that really sticks out. That said, I can reproduce it easily.

jessegreenberg · 2019-06-11T15:36:04Z

Thanks for reporting. I am not able to reproduce with NVDA. @zepumph I will have access to a mac on Friday, I can take a look then.

It sounds like we are missing the alert content. I can see the alerts in the a11y-view. My initial guess would be to increase the alertStableDelay of ValueChangeUtterance (unfortunate because it is already quite long). Although, that does't explain why it isn't happening on the mass spinners.

terracoda · 2019-06-13T08:22:37Z

@jessegreenberg, I agree, I am not sure we want to increase the delay, but we can see if that has something to do with it.

zepumph · 2019-06-14T23:25:41Z

@jessegreenberg thanks, let me know if there is anything I can do to help.

jessegreenberg · 2019-06-17T16:01:03Z

This doesn't happen every time you press arrow keys multiple times in a row, but it happens frequently. I still haven't been able to get it to happen for the mass spinners.

EDITS to avoid spamming with new comments:

I would also expect this to be a problem in ohms-law, but I just verified that it isn't.
JSFiddle for testing: https://jsfiddle.net/uoqxr67g/6/
I was wondering if it was because of the sliders changing position, I tested to see if moving the slider as the value changes impacted alerts. I saw no impact.

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>title</title>
  </head>
  <body>

    <input type="range" id="range-1" aria-orientation="horizontal" style="position: absolute; top: 100px;">
    <input type="range" id="range-2">

    <p id='live-element' aria-live="polite"></p>
  
  </body>

  <script type="text/javascript">
    const range1 = document.getElementById( 'range-1');
    const range2 = document.getElementById( 'range-2');
    const liveElement = document.getElementById( 'live-element' );

    let counter = 0;
    let timeoutId = null;
    const alertLater = ( alert ) => {
      if ( timeoutId !== null )
      window.clearTimeout( timeoutId );

      timeoutId = window.setTimeout( () => {
        liveElement.textContent = alert + ' ' + (++counter);
      }, 1000 );
    }

    let topCounter = 0;
    range1.addEventListener( 'change', ( event ) => {
      range1.setAttribute( 'aria-valuetext', 'value: ' + range1.value );
      alertLater( 'First Slider Changed' );

      range1.style.top = 10 * topCounter++ + 'px';
    } );

    range2.addEventListener( 'change', ( event ) => {
      range2.setAttribute( 'aria-valuetext', 'value: ' + range2.value );
      
      alertLater( 'Second Slider Changed' );  
  } );
  </script>
</html>

I also tried disabling the css positioning of PDOM elements altogether and still saw the bug.

I also noticed that the difference between these sliders and others that don't show the issue is aria-orientation. I tested different values and it seemed to have no impact on this issue occurrence.
I am seeing this bug also in the vertical "Solute Amount" slider in Molarity, but not the "Solution Volume" slider.
I see a pattern where this issue shows up right after VoiceOver gives interaction hints "You are currently on a slider inside of web content, blah blah....", which is also the case in the original issue ticket (https://drive.google.com/open?id=16wYOZK75nVWuJ9aPZKKshZ6KvugHFhld). However, I tried to turn these off by unchecking "Speak instructions for using item in the VoiceOver cursor" and the problem was still there.
Commenting out this.updateAriaValueText( oldValue ); in AccessibleValueHandler makes the problem go away.
I was able to make the issue happen on the mass spinners by making the aria-valuetext string longer {{massValue}} kilograms and some other content to read

jessegreenberg · 2019-06-17T20:21:05Z

OK, so I think this is related to the length of the text used in aria-valuetext. I was able to produce the issue in this example:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>title</title>
  </head>
  <body>

    <input type="range" id="range-1" aria-orientation="horizontal">
    <input type="range" id="range-2">

    <p id='live-element' aria-live="polite"></p>
  
  </body>

  <script type="text/javascript">
    const range1 = document.getElementById( 'range-1');
    const range2 = document.getElementById( 'range-2');
    const liveElement = document.getElementById( 'live-element' );

    let counter = 0;
    let timeoutId = null;
    const alertLater = ( alert ) => {
      if ( timeoutId !== null )
      window.clearTimeout( timeoutId );

      timeoutId = window.setTimeout( () => {
        liveElement.textContent = alert + ' ' + (++counter);
      }, 1000 );
    }

    let topCounter = 0;
    range1.addEventListener( 'change', ( event ) => {
      range1.setAttribute( 'aria-valuetext', 'a very long aria-valuetext string that will take time to read: ' + range1.value );
      alertLater( 'First Slider Changed' );

    } );

    range2.addEventListener( 'change', ( event ) => {
      range2.setAttribute( 'aria-valuetext', 'a very long aria-valuetext string that will take time to read: ' + range2.value );
      
      alertLater( 'Second Slider Changed' );  
  } );
  </script>
</html>

ValueChangeUtterance was added to work around this exact issue but assumed that the value-text would only be spoken once per interaction. In this case, the aria-valuetext is spoken twice or more to notify the user of each value change so we need to wait about the amount of time the screen reader reads all of the aria-valuetext before speaking an alert so VoiceOver doesn't ignore it.

We should submit a bug report to Apple about this, alerts should be working in this case.

We could probably work around this by creating a method that is like "make alertStableDelay dependent on the speed at which the user changed an input value so that VoiceOver has time to read queued aria-valuetext updates". We would probably add this workaround in AccessibleValueHandler.

jessegreenberg · 2019-06-17T21:55:03Z

This diff is sort of what I was thinking, but I haven't tested it with GFLB yet.

diff --git a/js/accessibility/AccessibleValueHandler.js b/js/accessibility/AccessibleValueHandler.js
index 46e6db4..d43a327 100644
--- a/js/accessibility/AccessibleValueHandler.js
+++ b/js/accessibility/AccessibleValueHandler.js
@@ -31,6 +31,8 @@ define( require => {
   const SunConstants = require( 'SUN/SunConstants' );
   const timer = require( 'AXON/timer' );
   const Util = require( 'DOT/Util' );
+  const utteranceQueue = require( 'SCENERY_PHET/accessibility/utteranceQueue' );
+  const ValueChangeUtterance = require( 'SCENERY_PHET/accessibility/ValueChangeUtterance' );
 
   const AccessibleValueHandler = {
 
@@ -127,6 +129,10 @@ define( require => {
              */
             a11yCreateValueChangeAriaValueText: _.identity,
 
+            a11yCreateValueChangeAlert: () => {
+              return 'Alert Generated';
+            },
+
             /**
              * List the dependencies this Node's PDOM descriptions have. This should not include the valueProperty, but
              * should list any Properties who's change should trigger description update for this Node.
@@ -247,6 +253,19 @@ define( require => {
           // @private - {null|function} see options for doc
           this.a11yCreateOnFocusAriaValueText = options.a11yCreateOnFocusAriaValueText;
 
+          // @private {null|function} see options for doc
+          this.a11yCreateValueChangeAlert = options.a11yCreateValueChangeAlert;
+
+          // @private {number} - number of times the input has changed in value before the valueChangeUtterance made
+          // was able to be spoken, only applicable if using a11yCreateValueChangeAlert
+          this.timesChangedBeforeAlerting = 0;
+
+          // @private - The utterance sent to the utteranceQueue when the value changes, alert content generated by
+          // optional a11yCreateValueChangeAlert. The alertStableDelay on this utterance will increase if the input
+          // receives many interactions before the utterance can be announced so that VoiceOver has time to read the
+          // aria-valuetext before the alert.
+          this.valueChangeUtterance = new ValueChangeUtterance();
+
           this.setA11yDependencies( options.a11yDependencies );
 
           // listeners, must be unlinked in dispose
@@ -265,7 +284,7 @@ define( require => {
 
           // when the property changes, be sure to update the accessible input value and aria-valuetext which is read
           // by assistive technology when the value changes
-          const valuePropertyListener = () => {
+          const valuePropertyListener = ( value, oldValue ) => {
 
             const formattedValue = this.getA11yFormattedValue();
 
@@ -276,6 +295,23 @@ define( require => {
 
             // update the PDOM input value on Property change
             this.inputValue = formattedValue;
+
+            if ( this.a11yCreateValueChangeAlert ) {
+              this.valueChangeUtterance.reset();
+              
+              this.valueChangeUtterance.alert = this.a11yCreateValueChangeAlert( formattedValue, value, oldValue );
+
+              if ( utteranceQueue.hasUtterance( this.valueChangeUtterance ) ) {
+                this.timesChangedBeforeAlerting++;
+              }
+              else {
+                this.timesChangedBeforeAlerting = 1;
+              }
+              const delayTime = this.timesChangedBeforeAlerting * ValueChangeUtterance.DEFAULT_STABLE_DELAY;
+              this.valueChangeUtterance.alertStableDelay = delayTime;
+
+              utteranceQueue.addToBack( this.valueChangeUtterance );
+            }
           };
           this._valueProperty.link( valuePropertyListener );

I think VoiceOver does collapse some of the back to back aria-valuetext so maybe we only have to increase alertStableDelay by a factor of 2 or 3 in this case, but I don't know for certain yet.

@zepumph what do you think about this? It adds complexity to AccessibleValueHandler. It is brittle because it only works with certain user settings. That is true for ValueChangeUtterance, but this increases the investment into this workaround. It is an issue with VoiceOver and I don't know if we should spend time changing AccessibleValueHandler and slider usages for this. I am not certain that this will actually work.

On the other hand, is it generally beneficial to have a valueChangeUtterance come with AccessibleValueHandler since often we want aria-valuetext followed immediately by an aria-live alert?

zepumph · 2019-06-18T00:29:31Z

@jessegreenberg let's talk about this tomorrow! @twant and I noticed the following thoughts:

multiplying by 1000 seems like a large coefficient, perhaps there is a better number we can find.
I don't yet see how timesChangedBeforeAlerting correlates to the length of the alert itself. Isn't the length of the alert the factor that changes how VO ignores some alerts?

jessegreenberg · 2019-06-18T17:22:38Z

multiplying by 1000 seems like a large coefficient, perhaps there is a better number we can find.

There definitely could be, it just seemed like a reasonable starting place. 1000 was determined to be a decent workaround value for this issue when there is only one aria-valuetext to read. So the thought was to multiply this by the number of times we expect aria-valuetext to be read when keys are pressed multiple times.

Isn't the length of the alert the factor that changes how VO ignores some alerts?

From what I have seen, it is the length of the aria-valuetext, not the length of the alert. So this variable represents the number of times we expect VO to read an aria-valuetext before reading an alert.

jessegreenberg · 2019-06-18T17:27:45Z

I was preparing a bug report for Apple when I realized that the example HTML where this is happening is still pretty complicated. And when I removed the window.setTimeout in #146 (comment) the problem went away.

So this isn't as straight forward as I thought, its more like there is a window of time where sending an alert to VoiceOver is bad. If set immediately on change, all is well. If set ~200 ms after, things are bad if the value is changed only once. If set ~1000 ms after, things are bad if the user changes the value more than once. Ill try #146 (comment) in the example HTML before we decide to go further with it.

jessegreenberg · 2019-06-18T17:47:52Z

This HTML does resolve the issue.

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>title</title>
  </head>
  <body>

    <input type="range" id="range-1">
    <input type="range" id="range-2">

    <p id='live-element' aria-live="polite"></p>
  
  </body>

  <script type="text/javascript">
    const range1 = document.getElementById( 'range-1');
    const range2 = document.getElementById( 'range-2');
    const liveElement = document.getElementById( 'live-element' );

    let counter = 0;
    let timeoutId = null;
    let changeCountBeforeAlerting = 0;
    let alertContent = null;

    const alertLater = ( alert ) => {
      'use strict';

      window.clearTimeout( timeoutId );

      if ( alertContent !== null ) {
        changeCountBeforeAlerting++
      }
      else {
        changeCountBeforeAlerting = 0;
      }

      alertContent = alert + ' ' + (++counter);

      timeoutId = window.setTimeout( () => {
        liveElement.textContent = alertContent;

        alertContent = null;
        changeCountBeforeAlerting = 0;
      }, 1000 * ( changeCountBeforeAlerting + 1 ) );
    }

    let topCounter = 0;
    range1.addEventListener( 'change', ( event ) => {
      range1.setAttribute( 'aria-valuetext', 'a very long aria-valuetext string that will take time to read: ' + range1.value );
      alertLater( 'First Slider Changed' );

    } );

    range2.addEventListener( 'change', ( event ) => {
      range2.setAttribute( 'aria-valuetext', 'a very long aria-valuetext string that will take time to read: ' + range2.value );
      
      alertLater( 'Second Slider Changed' );  
  } );
  </script>
</html>

As @zepumph and @twant mentioned, this delay is probably much longer than it needs to be. VO reads at most 2 aria-valuetext updates when keys are pressed multiple times, so maybe we just need to delay by 1000 * 2 ms in this case.

… change its alertStableDelay when there are multiple interactions before an alert can be spoken, see phetsims/gravity-force-lab-basics#146

zepumph · 2019-06-18T21:01:26Z

@jessegreenberg and I worked on this, basically committing the diff that @jessegreenberg created in #146 (comment). We found that it worked very well, and couldn't reproduce the bug with the changes, but there are a lot of factors at play here:

Once we reverted changes, and tested phettest, it was particularly hard to reproduce the bug (though it was still possible)
We only tested one rate of talking for VO
Things sounded good on NVDA firefox/chrome

We didn't commit our hacky changes to GFLB, so currently there is no way to test this. I will work on a better solution this afternoon, and then ask @terracoda to review.

zepumph · 2019-06-18T21:02:30Z

@jessegreenberg refreshed phetdev (dev.39) and now the bug is much more reproduce-able. How strange!

…sics#146

…ravity-force-lab-basics#146

zepumph · 2019-06-18T23:09:32Z

@terracoda I added some temporary code to GFLB so that we could test this out. If it seems like the direction we should go, then I can clean it up. Let me know what you think.

terracoda · 2019-06-19T16:01:06Z

@zepumph, I think you nailed it. The alerts are no longer stopping. Assigning back to you.

KatieWoe · 2019-06-24T17:47:48Z

May be related to this known issue:

[Safari, VO] Sometimes VoiceOver will stop reading “value” information when a slider is moved with the keyboard. See phetsims/sun#508 and phetsims/ohms-law#141

If it is and the above solution is effective, it may be worth expanding to other sims.

terracoda · 2019-06-24T21:04:34Z

Thanks @KatieWoe for tagging those issues. I'll let @zepumph comment, but I don't think either of those issues are related. The Ohm's Law issue was about information in aria-valuetext not being read out whereas this one is about information in an aria-live region not being read out.

I'll let @zepumph comment further about whether this issue is completed and ready to close.

jessegreenberg · 2019-06-28T19:45:37Z

I think this is ready to be closed, but I did happen on one last todo from #156:

      this.positionUtterance = new ValueChangeUtterance(); // TODO: delete me depending on https://github.com/phetsims/gravity-force-lab-basics/issues/146

Usages are already commented out so I think this can be removed, but I will let @zepumph comment for certain.

jessegreenberg · 2019-06-28T19:47:17Z

Ah I see some other TODO code related to this so it may not be quite ready

refactor this better if we decide to do it, alertPositionUnchanged too, #146

…de, #146

…de, phetsims/gravity-force-lab-basics#146

zepumph · 2019-06-28T21:54:43Z

I did the needed refactoring, and now the only things pointing to this issue are the warnings @jessegreenberg and I added purposefully. I think this is ready to close. @terracoda can you just give a once over to make sure the descriptions still sound as expected?

terracoda · 2019-06-29T10:03:37Z

@zepumph, all still sounds good on MacOS 10.13.6 with VoiceOver and Safari 12.1.1.

…sics#146

etwa4650 added the type:bug label Jun 10, 2019

etwa4650 assigned jessegreenberg and zepumph Jun 10, 2019

zepumph removed their assignment Jun 14, 2019

jessegreenberg assigned zepumph and unassigned jessegreenberg Jun 17, 2019

jessegreenberg mentioned this issue Jun 18, 2019

VoiceOver doesn't read alerts values change quickly with long strings of aria-valuetext phetsims/a11y-research#140

Open

zepumph added the priority:2-high label Jun 18, 2019

jessegreenberg added a commit to phetsims/scenery-phet that referenced this issue Jun 18, 2019

functiosn to reset timing variables for phetsims/gravity-force-lab-ba…

137f3d2

…sics#146

zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Jun 18, 2019

ISLCObjectNode to use AccessibleValueHandler for alerting, phetsims/g…

f80ea0d

…ravity-force-lab-basics#146

zepumph added a commit to phetsims/sun that referenced this issue Jun 18, 2019

ISLCObjectNode to use AccessibleValueHandler for alerting, phetsims/g…

1e362be

…ravity-force-lab-basics#146

zepumph added a commit to phetsims/scenery-phet that referenced this issue Jun 18, 2019

validate alert setter, phetsims/gravity-force-lab-basics#146

3dd8e0e

zepumph removed their assignment Jun 18, 2019

zepumph assigned terracoda Jun 18, 2019

zepumph added dev:code-review status:ready-for-review and removed dev:code-review labels Jun 18, 2019

terracoda mentioned this issue Jun 19, 2019

Progress on description implementaion for this sim #84

Closed

68 tasks

terracoda assigned zepumph and unassigned terracoda Jun 19, 2019

terracoda removed status:ready-for-review type:bug labels Jun 24, 2019

zepumph added a commit that referenced this issue Jun 28, 2019

cleanup after prototype by passing in force describer to ISLCObjectNo…

8a7385b

…de, #146

zepumph added a commit to phetsims/gravity-force-lab that referenced this issue Jun 28, 2019

cleanup after prototype by passing in force describer to ISLCObjectNo…

3567cc8

…de, phetsims/gravity-force-lab-basics#146

zepumph added a commit to phetsims/coulombs-law that referenced this issue Jun 28, 2019

cleanup after prototype by passing in force describer to ISLCObjectNo…

3970d4e

…de, phetsims/gravity-force-lab-basics#146

zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Jun 28, 2019

cleanup after prototype by passing in force describer to ISLCObjectNo…

e31f707

…de, phetsims/gravity-force-lab-basics#146

zepumph assigned terracoda and unassigned zepumph Jun 28, 2019

zepumph added the status:ready-for-review label Jun 28, 2019

terracoda closed this as completed Jun 29, 2019

zepumph pushed a commit to phetsims/utterance-queue that referenced this issue Oct 22, 2019

functiosn to reset timing variables for phetsims/gravity-force-lab-ba…

9e0a6ab

…sics#146

zepumph pushed a commit to phetsims/utterance-queue that referenced this issue Oct 22, 2019

functiosn to reset timing variables for phetsims/gravity-force-lab-ba…

ada2d41

…sics#146

zepumph added a commit to phetsims/utterance-queue that referenced this issue Oct 22, 2019

validate alert setter, phetsims/gravity-force-lab-basics#146

0e27b93

Screen reader not reading out adjusted force if mass moved twice #146

Screen reader not reading out adjusted force if mass moved twice #146

Comments

etwa4650 commented Jun 10, 2019

terracoda commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terracoda commented Jun 10, 2019

Uh oh!

terracoda commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessegreenberg commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terracoda commented Jun 13, 2019

Uh oh!

zepumph commented Jun 14, 2019

Uh oh!

jessegreenberg commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessegreenberg commented Jun 17, 2019

Uh oh!

jessegreenberg commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zepumph commented Jun 18, 2019

Uh oh!

jessegreenberg commented Jun 18, 2019

Uh oh!

jessegreenberg commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessegreenberg commented Jun 18, 2019

Uh oh!

zepumph commented Jun 18, 2019

Uh oh!

zepumph commented Jun 18, 2019

Uh oh!

zepumph commented Jun 18, 2019

Uh oh!

terracoda commented Jun 19, 2019

Uh oh!

KatieWoe commented Jun 24, 2019

Uh oh!

terracoda commented Jun 24, 2019

Uh oh!

jessegreenberg commented Jun 28, 2019

Uh oh!

jessegreenberg commented Jun 28, 2019

Uh oh!

zepumph commented Jun 28, 2019

Uh oh!

terracoda commented Jun 29, 2019

Uh oh!

terracoda commented Jun 10, 2019 •

edited

Loading

terracoda commented Jun 10, 2019 •

edited

Loading

jessegreenberg commented Jun 11, 2019 •

edited

Loading

jessegreenberg commented Jun 17, 2019 •

edited

Loading

jessegreenberg commented Jun 17, 2019 •

edited

Loading

jessegreenberg commented Jun 18, 2019 •

edited

Loading