phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

MR for windows aria-live support #16

Closed zepumph closed 3 years ago

zepumph commented 3 years ago

This is from https://github.com/phetsims/scenery-phet/issues/490, and will need to patch as many sims as needed with

https://github.com/phetsims/utterance-queue/commit/8104d517b9720d5111e9e2e453f0010c9fa188af

@jessegreenberg does that sound right to you?

ration and proportion 1.0 has already been patched.

jessegreenberg commented 3 years ago

Yes, that sounds right to me. There is currently a MR in progress for https://github.com/phetsims/QA/issues/608, when that is finished we can begin this one.

pixelzoom commented 3 years ago

@zepumph @jessegreenberg can you fill me in on why this is relevant for scenery-phet? The commit that you referenced above concernes AriaHerald, and I see no occurrences of AriaHerald in scenery-phet code.

jessegreenberg commented 3 years ago

I am guessing this was opened in scenery-phet because the parent issue phetsims/scenery-phet#490 is in scenery-phet. AriaHerald used to live in scenery-phet which is why phetsims/scenery-phet#490 was opened here in 2019. This MR involves code now in utterance-queue, I will move the issue there.

jessegreenberg commented 3 years ago

I notified dev-public that I was going to begin this. I reviewed sims listed in interactive-description and on the website and identified that these published sims will need the patch. Published branches were determined manually, I couldn't find a way to check branch status on a subset of sims.

Some of these were published some time ago, possibly before this repo existed so I expect we will need to do some manual patching.

zepumph commented 3 years ago

Some of these were published some time ago, possibly before this repo existed so I expect we will need to do some manual patching.

The auto MR process should allow you to detect this, likely only one or two patches will be needed (one for sims with utterance-queue/ and the other for that code in scenery-phet?). Are we worried at all about adding this code to a copy of AriaHerald that was before we need this patch? I don't have a good understanding of where the original commit in https://github.com/phetsims/scenery-phet/issues/490 come into play in terms of all of these sims. Is it possible that some, like maybe RIAW or BASE are older than this patch needs?

You are probably aware of this sentiment, but I wanted to ping you about just to be sure. I agree that manual testing will likely be important, but also, most of the important logic exists in a single function, AriaHerald.updateLiveElement, and so I would assume that it would be pretty straight forward to see cases where the "hidden" is not being set, and just timing and textContent is set.

Let me know if I can help, and good luck!

jessegreenberg commented 3 years ago

Thanks, that is a good point, for sims that predate use of hidden it is possible that no patch is needed. While going through the process I found that was true for the following sims

I tested these with NVDA in Chrome and verified that there were no issues with alerts.

That leaves the following sims

as needing the patch, some of which will have a patch in utterance-queue and others in scenery-phet.

jessegreenberg commented 3 years ago

Maintenance RCs deployed in https://github.com/phetsims/QA/issues/614. I did a spot check on all with JAWS in Chrome and things seem fixed, but we will see how the spot check goes.

jessegreenberg commented 3 years ago

QA spot check has cleared, proceeding with Maintenance.deployProduction().

jessegreenberg commented 3 years ago

After kicking it off, I received this error

maintenance> Maintenance.deployProduction()
Running production deploy for gravity-force-lab 2.2
Updating branch README
Production README is already up-to-date
Running "lint-all" task
Running "report-media" task
Running "clean" task
Running "build" task
Building runnable repository (gravity-force-lab, brands: phet, phet-io)
Building brand: phet
>> require.js optimization for brand: phet complete (6054364 bytes)
>> Minification for phet complete
>> Production scripts: 2996812 bytes
Building brand: phet-io
>> require.js optimization for brand: phet-io complete (6205998 bytes)
>> Minification for phet-io complete
>> Production scripts: 3021169 bytes
Done.
>> Detected failure during deploy, reverting to master
Maintenance task failed:
Error: Failure with production deploy for gravity-force-lab to 2.2: StatusCodeError: 401 - "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">\n<html><head>\n<title>401 Unauthorized</title>\n</head><body>\n<h1>Unauthorized</h1>\n<p>This server could not verify that you\nare authorized to access the document\nrequested.  Either you supplied the wrong\ncredentials (e.g., bad password), or your\nbrowser doesn't understand how to supply\nthe credentials required.</p>\n</body></html>\n"
    at Function.deployProduction (C:\Users\Jesse\Documents\Development\phetsims-secondary\perennial\js\common\Maintenance.js:818:17)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
Full Error details:
{}
zepumph commented 3 years ago

I made it so that GFL 2.2 will be able to production deploy over in https://github.com/phetsims/perennial/issues/209. Let me know if I should deploy it for any brands, or do anything more.

jessegreenberg commented 3 years ago

OK Thanks!

I just tried again and got "It appears that the last deployment was for production" error so I think I need to revert a commit to package.json and try again.

jessegreenberg commented 3 years ago

The production task finished. I went through each link reported by Maintenance.listLinks() and all are running. This has been completed, closing.