search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
http://scenerystack.org/
MIT License
8 stars 6 forks source link

PhET-iO Design meeting for joist + a11y #445

Closed zepumph closed 6 years ago

zepumph commented 7 years ago

Today we hosted a design meeting for joist/a11y components and phet-io instrumentation. The focus was to look into the desirable features and also to prune down what is already instrumented in instance proxies. Attendees @emily-phet, @kathy-phet, @ariel-phet, @arouinfar, @jessegreenberg, @samreid, @zepumph, @terracoda.

The discussion was loosely based on faradays-law because we will implement this customization as the next step of https://github.com/phetsims/faradays-law/issues/79

Meeting Notes:



~~ EDIT: PLEASE SEE https://github.com/phetsims/joist/issues/445#issuecomment-341248255 FOR AN UPDATED LIST ~~



Joist Features

This may be done with new methods on TScreen. Is this bullet that we want to support having different names, or that this independence is problematic? @zepumph sorta thinks that it is a problem that they can be updated independently.

?noHomeScreen ?homeScreenAndHomeScreenIconInNavbar=false ?allowHomeScreen=false ?hideHomeScreen our favorite: > ?homeScreen=false

Maybe do ?bypassHomeScreen instead of showHomeScreen=false See https://github.com/phetsims/joist/issues/451 for outcome and impelmentation.

Let's uninstrument frameEndedEmitter, it is for phet internal use only

@samreid says it is unclear why we would want to remove TSim. Is the getScreenshotDataURL not satisfactory? @zepumph scratches his head while scratching that bullet out. I think the current way of doing things is nice.

Thanks to all who joined us, it was a long one!! Please feel free to annotate or provide feedback. From here @samreid and I will implement the above list.

zepumph commented 7 years ago

For now I am not instrumenting keyboard help dialog content since it seems like we are just going to want to toggle the nav bar button until we work out dynamic features.

zepumph commented 7 years ago

Marking as high priority as it is blocking FL redesign and data stream design meeting schedule.

zepumph commented 7 years ago

I will try to get to this asap

zepumph commented 7 years ago

Functionality lost but not mentioned in the list above:

Notes:

zepumph commented 7 years ago

I made a first pass at this issue today, and @samreid and I discussed a few good points. For the most part everything has been promoted to it's own issue. As a high priority it seems like we should fix everything that hits Faradays Law so that we can move forward with the redesign process. Depending on the outcome of https://github.com/phetsims/joist/issues/454, that may not touch any a11y stuff (although unlikely).

zepumph commented 7 years ago

TFocus should identify the conceptual node being highlighted, not an array of numbers indicating the trail. Note like in BASE some of these nodes are “grabber” nodes that allow you to grab the node. If we are using tandem names for this it means all tabbable nodes need to be instrumented with tandems. Michael says we may need a new tandem flag for this (to indicate tabbable).

In the above commit I added the "focusedPhetioID" into the state object. This works pretty well to mark the focused item. I found that about 90% or more of tabbable entities in the sim have a tandem. From here I think it would be easy enough to fill in the other tandems so that all tab navigation has phet-io support. I left the indices in there too because that is important for the state wrapper, although that sorta leaks an implementation detail that clients shouldn't look at.

zepumph commented 7 years ago

@kathy-phet I have a question about the 4th bullet from above:

title on homescreen varies independently from title on navbar

Is this bullet that we want to support having different names, or instead that this independence is problematic? I sorta thought that independent updating was a problem. I don't remember exactly what the consensus was. What do you recommend for that feature? It seems like this is in the same boat as the screen icons. For this I thought we decided that you shouldn't be able to make the homescreen icon different than the nav bar icon for the same screen.

zepumph commented 7 years ago

In regards to https://github.com/phetsims/joist/issues/445#issuecomment-340616526,

title on homescreen varies independently from title on navbar

In the above comment I thought that the 'title' we were talking about was for each screen. @samreid helped clear it up. It is for the actual sim title. Unassigning @kathy-phet, sorry for confusion.

zepumph commented 7 years ago

@samreid please read through https://github.com/phetsims/joist/issues/445#issuecomment-340614487, then go to http://localhost/phet-io-wrappers/instance-proxies/instance-proxies.html?sim=faradays-law&ea&accessibility and use the keyboard nav of the sim (tab key) while looking at the value of the FocusProperty. Notice that when tabbing on the Magnet, we don't have a "focusedPhetioID" key, only indices. What do you recommend?

zepumph commented 7 years ago

I want to give an updated checklist of what is left to be done for this issue, note that there are a few types of priorities here:

  1. Needed for Faradays Law
  2. Needed for other sims (like multiscreen sims)
  3. Needed for when a11y is fully integrated into phet-io, potentially on a sim by sim instrumentation basis.

Below is a checklist split into the above priorities

1.

2.

3.

Other questions that perhaps should be answered before proceeding even with Faradays Law:

samreid commented 7 years ago

Notice that when tabbing on the Magnet, we don't have a "focusedPhetioID" key, only indices. What do you recommend?

It seems that everything you can tab to should probably have a phetioID.

Below is a checklist split into the above priorities

Let's remove the home screen item from (1) because Faraday's Law has only 1 screen and hence no homescreen.

EDIT from MK: updated above.

zepumph commented 7 years ago

It seems that everything you can tab to should probably have a phetioID.

see phetsims/faradays-law#83 for implementation for Faradays Law.

That should be a step in the instrumentation process, to check and make sure the focusProperty looks good when tabbing through everything. Otherwise that focusProperty value won't mean anything.

Also, those indices are bound to NEVER be stable, not sure how to handle this @samreid.

samreid commented 7 years ago

Also, those indices are bound to NEVER be stable, not sure how to handle this @samreid.

I don't recall what the indices are for, but if we have a phetioID for each tabbable item, perhaps we can use that instead, and eliminate the indices?

zepumph commented 7 years ago

See TFocus.fromStateObject for details., I'm not sure. It may be nice to keep them around when we start to have dynamic particles that are focusable. A11y hasn't gotten there yet, but I don't want to paint us into a corner.

zepumph commented 7 years ago

After thinking a bit more, it does seem like an index[] and a phetioID are two independent ways to get at the node. The only thing to fill in is how to get a Trail from a phetioID. Maybe the phetioID is already tied to a unique trail rather than the Node, but if not I'm not sure we can do that when the scene graph is DAGgy.

samreid commented 7 years ago

I see, we have been instrumenting Nodes, but we should have been instrumenting scenery.display.Instances?

zepumph commented 7 years ago

I see, we have been instrumenting Nodes, but we should have been instrumenting scenery.display.Instances?

Hmmm, well we say tandem.createTandem( 'magnet');, so that basically is a Trail since two magnet instances could look like screen1.magnet and screen2.magnet. Maybe we should investigate solving this just with phet-io, no indices allowed.

samreid commented 7 years ago

The DAG issue is more like this:

var myNode = new Node({tandem:t});
x.addChild(myNode);
y.addChild(myNode);

The node appears at two places (two Instances) in the scene graph and is controlled through a central Node. But is focus Instance-based or Node-based? Can the myNode in x be focused without the myNode in y being focused?

samreid commented 7 years ago

We decided we are at a sweet spot of using the phetioID of the focused Node (not instance) and the indices for playback reconstruction. @zepumph also recommended adding another element in the TFocus.toStateObject which will be like the indices of the trail, but with indices swapped out for phetioIDs for nodes that were instrumented. This could help researchers disambiguate different trails.

zepumph commented 7 years ago

This feature is pretty nice in our sims that are using "instrument everything" approach, but may not be that handy for our stable api. Here is an example of a good one in MAL, but Faraday's law has very few nested tandems. Overall I still think this is useful feature for understanding the phet-io context of a focus, although at the end of the day, it may be a bit redundant, for the most part isn't it just showing the trace that you can see in the tandem id? What do you think @samreid committed below.

image

Update: Sorry that the text is so small in the above picture, I was trying to get it on one screen.

samreid commented 7 years ago

Let's raise it as a discussion item for the follow-up Joist phet-io design meeting.

UPDATE from MK: Marked in https://github.com/phetsims/joist/issues/465 (round two design meeting)

zepumph commented 7 years ago

Updated list on the progress of this issue:

Any questions for the next design meeting: #465

List of issues that we are not working on for faradays law, and this issue can be closed before they are completed:

samreid commented 7 years ago

Great summary @zepumph, I'll unassign from this master issue and continue in the sub-issues.

jessegreenberg commented 6 years ago

https://github.com/phetsims/scenery/commit/3bc6c6c074f9c72ff1b6b6b72c9cdafe5077f0e0 assumed that every node up the focused node's trail was instrumented. I believe https://github.com/phetsims/scenery/commit/6f5d2273d5176390454c2856fe2769ed974bfa98 fixes this, and a problem I was running into where phetioID was undefined. I also replaced a reference to .tandem with .phetioObjectTandem. @samreid could you please review?

samreid commented 6 years ago

For https://github.com/phetsims/scenery/commit/6f5d2273d5176390454c2856fe2769ed974bfa98

https://github.com/phetsims/scenery/commit/6c8d9feaff540f3c508b4dce200be8459f529d0d looks great.

Leaving self-assigned to look into the two bullet points above.

samreid commented 6 years ago

Regarding the failure, I tried running http://localhost/phet-io-wrappers/instance-proxies/instance-proxies.html?sim=balloons-and-static-electricity&ea&accessibility with this modification:

            if ( node.phetioObjectTandem ) {
              assert && assert( node.phetioObjectTandem.phetioID, 'phetioID should be defined' );
              phetioIDIndices.push( node.phetioObjectTandem.phetioID );
            }

and saw this:

image

This seems it is working as expected. Reassigning to @jessegreenberg to get more information about the problem where phetioID was undefined.

jessegreenberg commented 6 years ago

Sorry I misunderstood, phetioObjectTandem was undefined. The problematic line was 

phetioIDIndices.push( node.phetioObjectTandem.phetioID || focus.trail.indices[ i ] );

because not all nodes in the trail had a phetioObjectTandem. We can remove the check for phetioID that I added.

jessegreenberg commented 6 years ago

Removed in the above commit. Removing my assignment. Thanks for reviewing @samreid.

zepumph commented 6 years ago

All that is left are the two checkboxes in https://github.com/phetsims/joist/issues/445#issuecomment-346216200, @samreid let's talk about this soon, as It is blocking stable api of FL.

samreid commented 6 years ago

@zepumph can you elaborate on this text?

"PhetMenu should not be instrumented so that you can have the barrier rectangle visible, but not the menu" workaround in master, phetsims/scenery#711 for real solution

Is it saying PhetMenu should not be instrumented? Is it saying the reason PhetMenu should not be instrumented so that you can show the barrier rectangle but not the menu? Why would we want to show the barrier rectangle without showing the menu? What is the nature of the workaround? What is the problem it is working around?

zepumph commented 6 years ago

Wow that was a seriously confusing trail of breadcrumbs I left us. Took me a few minutes to figure it out. Here is what I came up with.

First know that if PhetMenu is instrumented as a NodeIO, then you can make it visible:false such that when you click the PhetMenuButton, all you see is the BarrierRectangle (not what we want).

I was thinking that ideally we wouldn't have a Node (PhetMenu) instrumented as not a NodeIO or subtype. I feel this way because in general, keeping the PhET-iO type hierarchy as similar to scenery's is good to do (I know we break it sometimes).

So the workaround that I put in place is to instrument a new type, that isn't a subtype of NodeIO, called PhetMenuIO, in which you can't toggle visibility: image

This feels like a workaround because if ever we instrumented a feature of Node that we wanted all nodes to get (like softening corners or something totally different to visibility), then PhetMenu would not get this change. This is a slippery slope to me.

phetsims/scenery#711 is the issue to come up with a robust solution. In my mind I'm leaning towards a way to say "hey, your a NodeIO, but you can't be made invisible".

@samreid does this clear up the confusion?

samreid commented 6 years ago

My proposal in https://github.com/phetsims/scenery/issues/711#issuecomment-372192021 is more of an "opt-in" instead of "opt-out" strategy, but it would be good to discuss. On hold until then.

zepumph commented 6 years ago

I think we are ready to close this issue, there are a few loose ends that will be finished up in separate issues. . .

phetsims/scenery#711 will handle the last feature needed for Faraday's Law.

https://github.com/phetsims/phet-io/issues/1205#issuecomment-346450068 logs the last few tasks for the intersections of a11y and phet-io.

https://github.com/phetsims/phet-io/issues/1320 covers a question about updating phet-io sims.