VoiceOver reads the wrong accessible name

[jessegreenberg]

From phetsims/greenhouse-effect#385

In some cases, MacOS VoiceOver just reads the wrong accessible name. Moving to scenery because it is a general problem with description.

From phetsims/greenhouse-effect#385, we know that

1. It was introduced in a recent version of macOS. It does not happen with 15.5 Safari but it does happen for 17.3 Safari.
2. So far, it is only observed with inputType: checkbox
3. It seems to happen when components are dynamically added/removed from the screen.
4. We tried to produce in a simple test harness but have not been able to yet. See phetsims/greenhouse-effect#385 (comment).

[terracoda]

Has a bug been reported to Apple?

[jessegreenberg]

Not yet - we still need to determine if it is an Apple bug or a Scenery bug. If we can produce the problem in a minimal test case we should submit a report to Apple.

[jessegreenberg]

I was able to see this on a Mac with remote access. Some notes:

• This only happens when photons are moving in greenhouse effect. That indicates this is a performance problem.
• In order to see this, things need to be added/removed WHILE the photons are moving. Labels are correct if you set up checkboxes and then start photons.

EDIT:

• I tried in a version with the photon sprites removed and it did not fix the problem. It seems to happen a little less often, but still happens.
• I tried removing the number displays from the AtmosphereLayerNode in greenhouse effect and the problem seems gone.

Thoughts:

• It is behaving as if the Accessibility Tree doesn't update fully when the system is stressed.
• We could look for a workaround that forces an update to the Accessibility Tree so that accessible names are more correct.
• I don't see any drop in visual performance, so this behavior is strange.
• We could try to find a way to reproduce this outside of PhET and report something to Apple. Ideally we will make a test demo that fails in recent macOS and works OK in older versions to demonstrate when the regression ocurrred.

[jessegreenberg]

@Nancy-Salpepi or @arouinfar would you mind testing to confirm that it is fixed in this version? (photons and temperature number displays removed to improve performance).

https://bayes.colorado.edu/dev/html/jg-tests/greenhouse-effect_en_phet.html

[Nancy-Salpepi]

sorry still happening for me @jessegreenberg:

[jessegreenberg]

Over slack, @Nancy-Salpepi confirmed that this does not happen for her until the sunlight has started.

But it DOES consistently happen for her while the sim is paused.

It also DOES happen in the version in with photons and number displays removed.

This makes it seem less likely to be a performance issue, which is good. But now we are back to the drawing board.

[jessegreenberg]

Testing with @terracoda on MacOS 14.1.2 Safari 17.1.2. 16 GB RAM Apple M1 chip. We did not see this at all. We tested:

Adding/removing layers before sunlight started.
Adding/removing layers after sunlight started.

This is a slightly older version so we thought we might not see it. But it is also a high end Mac so we might not be seeing it because of good performance.

[jessegreenberg]

@arouinfar saw this on an M1 Pro with 32 GB of RAM, macOS 14.3. So I think that indicates this is NOT a performance problem.

So I will try again to remote access into a macOS device with this in mind.

[jessegreenberg]

@KatieWoe helped me test on a MacBook Pro with Sonoma 14.3.1. 8GB 2133 MHz LPDDR3.

I found a hacky workaround that forces a reflow of the element that becomes displayed

Subject: [PATCH] Indicate that items have been sorted, see https://github.com/phetsims/scenery-phet/issues/815
---
Index: js/accessibility/pdom/PDOMPeer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMPeer.js b/js/accessibility/pdom/PDOMPeer.js
--- a/js/accessibility/pdom/PDOMPeer.js	(revision bf5650694b5d0ea4e262b4f438f7b5b648f6837e)
+++ b/js/accessibility/pdom/PDOMPeer.js	(date 1708030516141)
@@ -787,6 +787,18 @@
         const element = this.topLevelElements[ i ];
         if ( visible ) {
           this.removeAttributeFromElement( 'hidden', { element: element } );
+
+          // do something to the element that will force it to reflow to see if it fixes a VoiceOver problem
+          console.log( 'forcing reflow' );
+          element.style.display = 'none';
+          element.offsetHeight;
+
+          // @ts-expect-error
+          // eslint-disable-next-line bad-sim-text
+          window.setTimeout( () => {
+            element.offsetHeight;
+            element.style.display = '';
+          }, 10 );
         }
         else {
           this.setAttributeToElement( 'hidden', '', { element: element } );

I also spent a long time trying to isolate the problem but was not successful. I could not get this to occur outside of a PhET simulation. I took notes while testing, Ill just put them here in details.

Testing notes -

Testing on a MacBook Pro with Sonoma 14.3.1
8GB memory
2133 MHz LPDDR3

Re-testing an old build, I AM able to get the problem to happen even when photons/number displays are removed.

I tried in a test case with rapid VoiceOver alerts and changing aria-valuetext. I have a theory now that
this is happening because of aria-live alerts. That would explain why we don't see it until sun starts, and it is
intermittent.

it seems harder to produce if the sim is paused.

In an isolated context, Lets try adding lots of other paragraphs which changing content to see if other accessible name changes cuase the problem.

Tried copying entire PDOM, adding JavaScript to add/remove the specific checkboxes.
0
Tried turning off context respones in VO settings.

I tried a build with aria-live totally disabled

I tried a build with aria-valuetext totally disabled

I noticed that the accessible name returns once you enable the energy balance.

I am trying a version with screen summary content and observation window content removed.
    - This seems to make the problem go away.

I am trying a version with screen summary/observation window content removed with aria-live and aria-valuetext added back in.
- The problem happened in the a11y view but did not happen in the regular view.

This makes me believe that this is a performance problem.
     - It is most consistent when photons are running and there are tons of animated changes happening in the DOM.
     - As we remove content, the problem happens less frequently.

I tried to add a canvas that is expensive to draw to see if that would cause the problem to happen more often.
    - It did not.

Now trying to remove custom scenery required styling in case that is causing it.
    - Removing a lot of the styling, still see it.

I tried with just the observation window removed
    - It still happened

I tried with the observation window and screen summary content removed
    - This actually seems to make the problem go away for normal sim
    - But it still happens in the a11y view

I tried inspecting the values of the accessibility tree and they look correct, even when VoiceOver is reading the wrong name.

In the sim we use an article attribute, related?
    - Probably not.
    - In my test page, I tried putting contents in an article (we have seen that element cause problems in the past).

I am trying to force a reflow when the hidden attribute is removed like this:
          element.style.display = 'none';
          element.offsetHeight;
          element.style.display = '';

But the problem still happened

This seemed to work better though:

          // do something to the element that will force it to reflow to see if it fixes a VoiceOver problem
          console.log( 'forcing reflow' );
          element.style.display = 'none';
          element.offsetHeight;

          // @ts-expect-error
          // eslint-disable-next-line bad-sim-text
          window.setTimeout( () => {
            element.offsetHeight;
            element.style.display = '';
          }, 10 );

EDIT: Also, here is my test context where I was trying to mimic parts of the sim that I thought may be causing this. It also tries to bog down the device with lots of drawing and rapidly changing DOM content. I saw the fps go down, but the problem never happened here yet.

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>VO Checkbox Test</title>
</head>
<body>

<div>
  <p id='alerter' aria-live='polite'></p>
</div>

<div>
  <label for='range-input'>Range Input</label>
  <input type='range' min='0' max='5' value='50' id='range-input'>
</div>

<article>
  <section id='readout-section'></section>
  <section>


    <h3
        id="display1-label-48-62-1111-1118-1113-1321">Infrared Controls</h3>
    <div id='1'>
      <label
          id="display1-label-48-62-1111-1118-1113-1036"
          for="display1-primary-48-62-1111-1118-1113-1036">Surface Thermometer</label><input
        id="display1-primary-48-62-1111-1118-1113-1036" type="checkbox">
      <p>A description for thermometer 1</p>
    </div>

    <div id='2'>
      <label
          id="display1-label-48-62-1111-1118-1113-991-990-980"
          for="display1-primary-48-62-1111-1118-1113-991-990-980">Layer 1 Thermometer</label><input
        id="display1-primary-48-62-1111-1118-1113-991-990-980" type="checkbox">
      <p>A description for thermometer 2</p>
    </div>

    <div id='3'>
      <label
          id="display1-label-48-62-1111-1118-1113-1010-1009-999"
          for="display1-primary-48-62-1111-1118-1113-1010-1009-999">Layer 2 Thermometer</label><input
        id="display1-primary-48-62-1111-1118-1113-1010-1009-999" type="checkbox">
      <p>A description for thermometer 2</p>

    </div>

    <div id='4'>
      <label
          id="display1-label-48-62-1111-1118-1113-1029-1028-1018"
          for="display1-primary-48-62-1111-1118-1113-1029-1028-1018">Layer 3 Thermometer</label><input
        id="display1-primary-48-62-1111-1118-1113-1029-1028-1018" type="checkbox">
      <p>A description for thermometer 3</p>

    </div>

    <div id='5'>
      <label
          id="display1-label-48-62-1111-1118-1113-916-915-903"
          for="display1-primary-48-62-1111-1118-1113-916-915-903">Energy Balance</label><input
        id="display1-primary-48-62-1111-1118-1113-916-915-903" type="checkbox">
      <p>Measure the energy balance</p>
    </div>

    <div id='6'>
      <label
          id="display1-label-48-62-1111-1118-1113-916-915-909"
          for="display1-primary-48-62-1111-1118-1113-916-915-909">Flux
        Meter</label><input
        id="display1-primary-48-62-1111-1118-1113-916-915-909" type="checkbox">
      <p>Measure the flux</p>
    </div>

  </section>
</article>


<button id='add'>Add Checkbox</button>
<button id='remove'>Remove Checkbox</button>

<canvas id='canvas'></canvas>

</body>

<script>

  // get all the containers
  const checkboxContainerIds = [ '1', '2', '3', '4', '5', '6' ];
  const checkboxContainers = checkboxContainerIds.map( id => document.getElementById( id ) );

  const setContainerVisible = ( container, visible ) => {

    // instead of setting on the container, set on the children individually (as we do in scenery)
    const children = container.children;
    for ( let i = children.length - 1; i > -1; i-- ) {
      children[ i ].hidden = !visible;
    }
  };

  // initially hide them all
  checkboxContainers.forEach( container => setContainerVisible( container, false ) );

  // Add alerts that will interrupt the screen reader, in case that is related
  const alerter = document.getElementById( 'alerter' );
  let alertCount = 0;
  window.setInterval( () => {
    alerter.textContent = `Visible count: ${alertCount++}`;
  }, 5000 );

  const rangeInput = document.getElementById( 'range-input' );
  rangeInput.addEventListener( 'input', () => {

    // test aria-valuetext
    rangeInput.setAttribute( 'aria-valuetext', `The current value is ${rangeInput.value}` );

    // The range triggers an alert, in case that kind of thing causes the problem
    alerter.textContent = `The range input changed and the new value is ${rangeInput.value}`;

    // Our alert system does something like this. Just trying it out, but it didnt seem to have an impact
    // window.setTimeout( () => {
      // alerter.textContent = '';
    // }, 10 );

    // hide all checkboxes
    checkboxContainers.forEach( container => setContainerVisible( container, false ) );

    // show the numnber of checkboxes based on the range input
    for ( let i = 0; i < rangeInput.value; i++ ) {

      // instead of adding hidden to the container, add it to children individually
      setContainerVisible( checkboxContainers[ i ], true );
    }
  } );

  // To mimic a PDOM with lots of changing elements, in case this creates a performance burden on the device
  // and causes it not to update frequently.
  const readoutElements = [];
  for ( let i = 1; i <= 200; i++ ) {
    const newElement = document.createElement( 'p' );
    newElement.id = `readout-${i}`;
    readoutElements.push( newElement );

    const readoutParent = document.getElementById( 'readout-section' );
    readoutParent.appendChild( newElement );
  }
  readoutElements.forEach( ( readout, index ) => {
    window.setInterval( () => {
      readout.textContent = `Readout ${index + 1} - ${Math.random()}`;
    }, 100 );
  } );

  // A canvas element where we will try to draw lots of things (like the sim)
  const canvasElement = document.getElementById( 'canvas' );

  // set the width and height of the canvas
  canvasElement.width = 500;
  canvasElement.height = 500;

  // get the context
  const context = canvasElement.getContext( '2d' );

  const particles = [];
  for ( let i = 0; i < 1000; i++ ) {
    particles.push( {
      x: Math.random() * canvasElement.width,
      y: Math.random() * canvasElement.height,
      color: `rgba(${Math.random() * 255}, ${Math.random() * 255}, ${Math.random() * 255}, ${Math.random()})`
    } );
  }

  window.setInterval( () => {

    // clear the canvasq
    context.clearRect( 0, 0, canvasElement.width, canvasElement.height );

    // animate the particles in a smooth way
    particles.forEach( particle => {
      particle.x += ( Math.random() - 0.5 ) * 5;
      particle.y += ( Math.random() - 0.5 ) * 5;
      context.fillStyle = particle.color;
      context.fillRect( particle.x, particle.y, 5, 5 );
    } );
  }, 16 );

</script>
</html>

@Nancy-Salpepi would you mind testing with https://bayes.colorado.edu/dev/html/jg-tests/greenhouse-effect_en_phet.html? If the problem is gone for you as well we can look into adding this workaround permanently.

To verify that the right build is being tested you should be able to see "forcing reflow" in the dev tools console.

[Nancy-Salpepi]

I was no longer able to reproduce the problem with the above link 🎉!

[jessegreenberg]

Excellent, thanks @Nancy-Salpepi! Lets find a better way to add this workaround then. Here is a patch that doesn't use a timeout but has the reflow done in the next scenery.Display update. There are a couple of TODOs still in the patch and will need better documentation for the workaround. Also want to test a lot more.

Subject: [PATCH] Indicate that items have been sorted, see https://github.com/phetsims/scenery-phet/issues/815
---
Index: js/accessibility/pdom/PDOMPeer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMPeer.js b/js/accessibility/pdom/PDOMPeer.js
--- a/js/accessibility/pdom/PDOMPeer.js	(revision bf5650694b5d0ea4e262b4f438f7b5b648f6837e)
+++ b/js/accessibility/pdom/PDOMPeer.js	(date 1708530634017)
@@ -107,6 +107,12 @@
     // the siblings need to be repositioned in the next Display.updateDisplay()
     this.positionDirty = false;
 
+    // @private {boolean} - flag that indicates that the peer requires a force reflow next animation. At this time,
+    // this is needed as a workaround for a VoiceOver bug where the accessible name is incorrect unless this is done.
+    // The usual workaround to force a reflow (set the display to none, query the offset, set it back) does not work
+    // unless the restoration is done in the next animation frame.
+    this.forceReflowWorkaround = false;
+
     // @private {boolean} - indicates that this peer's pdomInstance has a descendant that is dirty. Used to
     // quickly find peers with positionDirty when we traverse the tree of PDOMInstances
     this.childPositionDirty = false;
@@ -136,10 +142,10 @@
     // TODO: Should we be watching "model" changes from ParallelDOM.js instead of using MutationObserver? https://github.com/phetsims/scenery/issues/1581
     // See https://github.com/phetsims/scenery/issues/852. This would be less fragile, and also less
     // memory intensive because we don't need an instance of MutationObserver on every PDOMInstance.
-    this.mutationObserver = this.mutationObserver || new MutationObserver( this.invalidateCSSPositioning.bind( this ) );
+    this.mutationObserver = this.mutationObserver || new MutationObserver( this.invalidateCSSPositioning.bind( this, false ) );
 
     // @private {function} - must be removed on disposal
-    this.transformListener = this.transformListener || this.invalidateCSSPositioning.bind( this );
+    this.transformListener = this.transformListener || this.invalidateCSSPositioning.bind( this, false );
     this.pdomInstance.transformTracker.addListener( this.transformListener );
 
     // @private {*} - To support setting the Display.interactive=false (which sets disabled on all primarySiblings,
@@ -794,7 +800,7 @@
       }
 
       // invalidate CSS transforms because when 'hidden' the content will have no dimensions in the viewport
-      this.invalidateCSSPositioning();
+      this.invalidateCSSPositioning( true );
     }
   }
 
@@ -984,12 +990,22 @@
    * Mark that the siblings of this PDOMPeer need to be updated in the next Display update. Possibly from a
    * change of accessible content or node transformation. Does nothing if already marked dirty.
    *
+   * @param [forceReflowWorkaround] - In addition to repositioning, force a reflow next animation frame?
    * @private
    */
-  invalidateCSSPositioning() {
+  invalidateCSSPositioning( forceReflowWorkaround = false ) {
     if ( !this.positionDirty ) {
       this.positionDirty = true;
 
+      if ( forceReflowWorkaround ) {
+        this.forceReflowWorkaround = true;
+
+        // make all top level elements display: none so that we can restore
+        this.topLevelElements.forEach( element => {
+          element.style.display = 'none';
+        } );
+      }
+
       // mark all ancestors of this peer so that we can quickly find this dirty peer when we traverse
       // the PDOMInstance tree
       let parent = this.pdomInstance.parent;
@@ -1091,7 +1107,19 @@
       }
     }
 
+    // TODO: Is this the right place for it? Do we need to be more selective about when we do this?
+    // TODO: Should this function be renamed now that it is doing more in Display.update() than just positioning?
+    if ( this.forceReflowWorkaround ) {
+
+      // force a reflow (recalculation of DOM layout) to fix the accessible name
+      this.topLevelElements.forEach( element => {
+        element.style.display = ''; // force reflow request added display: none
+        element.style.offsetHeight; // query the offsetHeight after restoring display to force reflow
+      } );
+    }
+
     this.positionDirty = false;
+    this.forceReflowWorkaround = false;
   }
 
   /**

[jessegreenberg]

The above commit cleans up the patch. I also decided to only apply the workaround on Safari. I was able to remote access into the same Mac I used in #1606 (comment) to confirm that this commit fixes the problem. I was able to easily observe the problem in the build for phetsims/qa#1043 and can no longer see it at main.

I opened phetsims/a11y-research#193 to submit a bug report to Apple, and hopefully someday this change can be removed.

This issue can be closed.