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

Description repeated when layers are added/removed #386

Closed
Tracked by #1043
Nancy-Salpepi opened this issue Jan 31, 2024 · 12 comments
Closed
Tracked by #1043

Description repeated when layers are added/removed #386

Nancy-Salpepi opened this issue Jan 31, 2024 · 12 comments

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
14.1.2

Browser
Safari 17.1.2

Problem description
For phetsims/qa#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
Copy link
Author

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
Copy link
Contributor

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 added a commit that referenced this issue Feb 6, 2024
@jbphet
Copy link
Contributor

jbphet commented Feb 6, 2024

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
Copy link
Contributor

Yes, I think that is right! I was curious why didn't notice this before and found the commit message for 68982a7

move context responses from a11yCreateContextResponseAlert options to the stepped alerter for better sequencing, see #374

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

@jbphet
Copy link
Contributor

jbphet commented Feb 7, 2024

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

jbphet added a commit that referenced this issue Feb 7, 2024
@jbphet
Copy link
Contributor

jbphet commented Feb 7, 2024

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 jbphet closed this as completed Feb 7, 2024
@jbphet
Copy link
Contributor

jbphet commented Feb 14, 2024

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

@KatieWoe
Copy link
Contributor

This looks ok in the a11y view of the sim.

@Nancy-Salpepi
Copy link
Author

Also fixed with VO.

@Nancy-Salpepi
Copy link
Author

Going to reopen because I see the same issue with changes to IR absorbance:
Screenshot 2024-02-16 at 12 49 46 PM

@jbphet
Copy link
Contributor

jbphet commented Feb 20, 2024

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
Copy link
Contributor

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

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

5 participants