phetsims / greenhouse-effect

"Greenhouse Effect" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Description repeated when layers are added/removed #386

Closed Nancy-Salpepi closed 7 months ago

Nancy-Salpepi commented 8 months ago

Test device MacBook Air M1 chip

Operating System 14.1.2

Browser Safari 17.1.2

Problem description For https://github.com/phetsims/qa/issues/1033, On the Layer Model Screen when using VO + keyboard nav OR a11y view + keyboard nav: When the number of absorbing layers is changed, description is repeated --ex. Layer 1 added above surface. Layer 1 added above surface

Steps to reproduce

  1. In a11y View on the Layer Model Screen (or in the sim with VO on), Tab to Absorbing Layers vertical number spinner and press the Right Arrow key once
  2. Press the Right Arrow Key again
  3. Press the Left Arrow Key

Visuals

Screenshot 2024-01-31 at 2 06 36 PM Screenshot 2024-01-31 at 2 07 26 PM Screenshot 2024-01-31 at 2 19 37 PM
Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Greenhouse Effect‬ URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.3.0-dev.3/phet/greenhouse-effect_all_phet.html Version: 1.3.0-dev.3 2024-01-23 21:58:15 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.2.1 Safari/605.1.15 Language: en-US Window: 1356x710 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
Nancy-Salpepi commented 8 months ago

The same thing happens when the % of Infrared Absorbance is changed. The "All layers now absorbing X%....." statement gets repeated.

Screenshot 2024-01-31 at 2 27 04 PM
arouinfar commented 8 months ago

Good catch @Nancy-Salpepi! I was able to reproduce in the a11y view and VO.

@jbphet the double alert occurs for keyboard interaction, but not for mouse interaction.

jbphet commented 7 months ago

I've done some investigation, and I can see that the alert is actually being performed twice in the keyboard interaction case, once by LayerModelModelAlerter and once by the a11yCreateContextResponseAlert option for the NumberPicker in AbsorbingLayersControl. The latter is not triggered via mouse interaction but is triggered via keyboard interaction. The former occurs regardless of the type of interaction.

Removing the a11yCreateContextResponseAlert option to the NumberPicker seems like the obvious solution, but I'd like to run this by @jessegreenberg before doing that, since he has much more experience with description than I.

jessegreenberg commented 7 months ago

Yes, I think that is right! I was curious why didn't notice this before and found the commit message for https://github.com/phetsims/greenhouse-effect/commit/68982a7569e8f10a09b65e0c9f88b71a7581539e

move context responses from a11yCreateContextResponseAlert options to the stepped alerter for better sequencing, see https://github.com/phetsims/greenhouse-effect/issues/374

The a11yCreateContextResponseAlerts were removed from the SolarIntensityControl and the SurfaceAlbedoControl and I think we just forgot to remove from the AbsorbingLayersControl.

jbphet commented 7 months ago

Cool. I'll go ahead and remove the one from the number picker (a11yCreateContextResponseAlert).

jbphet commented 7 months ago

Removed. By the way, when @jessegreenberg looked at the history of this, we saw that message accompanying the commit where the alert was moved in with the periodic ones says "...for better sequencing". When I tested the change I just made I noticed that the sequencing is indeed quite good in that it talks about a layer being added, then immediately says things like "more infrared photons emitting from the surface". So removing the one on the number picker and leaving the one that is grouped with the periodic checks definitely seems like the right move. Closing.

jbphet commented 7 months ago

I'm going to reopen this so that QA can test in the next round of testing. QA can close once verified.

KatieWoe commented 7 months ago

This looks ok in the a11y view of the sim.

Nancy-Salpepi commented 7 months ago

Also fixed with VO.

Nancy-Salpepi commented 7 months ago

Going to reopen because I see the same issue with changes to IR absorbance:

Screenshot 2024-02-16 at 12 49 46 PM
jbphet commented 7 months ago

Good catch @Nancy-Salpepi. The repeated alerts about "All layers now absorbing..." had the same root cause as the "Layer added" ones, namely that there was a a11yCreateContextResponseAlert option passed in to a UI component (a slider in this instance), and there was also code to create this alert based on periodic updates. I've removed the a11yCreateContextResponseAlert option and the code that went with it. I've verified that there is now only a single alert generated for both keyboard and mouse interaction.

I also searched the greenhouse-effect code for any other usages of a11yCreateContextResponseAlert and didn't find any, so there shouldn't be any other double-alerts that are caused by this.

arouinfar commented 7 months ago

I verified that this has been resolved in dev.6 (to be used for interviews) and on main. Closing.