Voice Over highlight doesn't move with the ruler

[zepumph]

@terracoda @jessegreenberg and I found that the VO focus highlight often doesn't "keep up" with the scenery Node. We should make sure that the css transforms of the PDOM are constantly staying in sync with the scenery Node (assigning @jessegreenberg to investigate this).

We then noticed that grabbing the ruler doesn't always work in mobile VO (gesture). This may have to do with the fact that scenery isn't keeping the VO highlight in the right place, but it also could be related to #189. I will solve that over there, and then we should come back here are retest to see if the out of sync VO highlight ever effects our ability to grab the ruler.

Note: The BASE Balloon VO highlight disappears when grabbing the balloon, and doesn't reappear again until you release the balloon, in which the VO highlight is in the right place around the balloon. The balloon can move more after being released; in this case the VO highlight stays invisible until the balloon is done moving, and then it appears in the right place on the balloon once done moving.

Marking on hold until #189 is solved. We don't want to assume that this is a separate issue.

[jessegreenberg]

No longer on hold because I can easily see the element bounds are not correct on chrome.

[jessegreenberg]

When moving the ruler wiht the keyboard the positioning update is correct. When moving the ruler with the mouse/pointer positioning is incorrect.

EDIT: This isn't always true, now I am having a hard time getting the bug to happen when using the mouse.

EDIT2: Ah I think it is happening when the ruler doesn't correctly get released by a mouse because an up event is caught over something else. To get this to happen consistently, grab the ruler, drag so that the mouse is over a checkbox, then release the ruler. The ruler will still be "grabbed" in the PDOM and the positioning doesn't update. The positioning should still update whether or not it is grabbed, but this may be related to other bugs here.

[jessegreenberg]

by a mouse because an up event is caught over something else

We worked on this part today and have a fix committed. The positioning is correct on grab/release now and so this should be mostly working for the user. But the positioning still isn't updating while dragging. We tried to see if this was happening in BASE and it is not.

[jessegreenberg]

The elements should update when the node transform changes, but I notice that we are never hitting the listener on the TransformTracker.

EDIT: We have the correct trail for the TransformTracker:

EDIT 2: No, actually the trail is missing the node that receives the updated transform:

950 is the RulerNode, which triggers the transform event, but node 949 is the ISLCRulerNode (parent Node of RulerNode) and doesn't get a transform update.

EDIT 3: Indeed, only the RulerNode is positioned when the rulerPositionProperty changes.

      model.rulerPositionProperty.link( value => {

        // Because the focus highlight is `focusHighlightLayerable`, the highlight for `this`
        // is a child of the ruler. As a result the "center" includes the focusHighlight as well
        // as some child added to it in GrabDragInteraction, and so it is easiest to set the center
        // disregarding the focusHighlight. See https://github.com/phetsims/gravity-force-lab/issues/140
        ruler.removeChild( focusHighlight );
        ruler.center = modelViewTransform.modelToViewPosition( value );
        ruler.addChild( focusHighlight );
      } );

Since the ISLCRulerNode has representation in the PDOM, it's AccessibleInstance has a trail with last node ISLCRulerNode, not including the child RulerNode. So it's TransformTracker doesn't include the RulerNode either.

One (verified) way to fix this is to make the RulerNode the target of the GrabDragInteraction. But maybe there are other consequences from this. @zepumph can we talk about how to handle this generally? It seems like we may need to move toward a limitation where any Node that is focusable/interactive must directly get representation in the PDOM. But maybe there is another way.

[jessegreenberg]

We discussed this today with help from @jonathanolson. We decided that we will add a setter to Accessibility.js to specify the source Node whose transforms should control updates to the PDOM siblings.

We brainstormed some potential names for this field

  - transformTarget
  - transformTargetNode
  - transformTargetListeningNode
  - transformListenerNode
  - pdomTransformNode
  - sourceNode
  - transformSourceNode
  - pdomTransformSourceNode
  - transformSource
  - transformListenee
  - visualTransformer
  - visualTransformNode
  - matchVisualNode
  - matchPDOMToVisualNode
  - observeVisualNodeTransform
  - observeTransformNode

We like pdomTransformSourceNode at this time. It is long but this is a pretty low level/complicated function so we are OK with that.

We also considered doing "sneaky" things like trying to extend the trail that we observe to a more leaf level node, or update the PDOM sibling positioning every animation frame. These sounded too complicated or performance intensive to investigate further.

[terracoda]

Today in 2.2.0-dev.18 I noticed that the VO highlight moves to the ruler sometimes when the pink focus highlight also shows up. Once the pink focus highlight shows up, the black VO highlight seems to jump into place around the ruler. This happens randomly as far as I can tell.

[jessegreenberg]

Thanks @terracoda! I think that is to be expected at this time, hopefully #194 (comment) will have this fixed.

[jessegreenberg]

pdomTransformSourceNode was added to Accessibility.js in the above commit. I also added QUnit tests for it.

[jessegreenberg]

Used in inverse-square-law-common in the above commit. I tried to do exactly what we discussed in #194 (comment) but @zepumph would you please review these changes?

[zepumph]

This looks really awesome! I'm excited for these changes, especially in GFL

• Since pdomTransformSourceNode is only ever Node|null, this:
  const transformSourceNode = this.node.pdomTransformSourceNode ? this.node.pdomTransformSourceNode : this.node;
  can be simplified to const transformSourceNode = this.node.pdomTransformSourceNode || this.node;
• At first glance I'm a bit confused as to the division of labor for this commit between AccessibleInstance and AccessiblePeer. Can you speak to this at all? For example. Why does one have setPDOMTransformSourceNode, but the other has updateTransformTracker? Note though that I do like the idiomatic way that Accessibility.setPDOMTransformSourceNode calls over to the same AccessiblePeer function, as this is the pattern used throughout the file.

[terracoda]

In 2.2.0-dev.20 the VO highlight now follows the position of the ruler and it is also displaying snug around the ruler now, too.

Edited to fix typos.

[terracoda]

@zepumph and @jessegreenberg, if no code review is need here, I think we can close.

[jessegreenberg]

#194 (comment)
Great, thanks @terracoda!

...can be simplified to...

Thanks!

I'm a bit confused as to the division of labor for this commit between AccessibleInstance and AccessiblePeer. Can you speak to this at all?

I struggle with the separation of these two in general. AccessibleInstance is the "model" and AccessiblePeer is the "view", and I think of TransformTracker and its Trail as "model" components to be observed by the AccessiblePeer. And so I think AccessibleInstance should be responsible for updating the TransformTracker while AccessiblePeer is responsible for updating listeners on the TransformTracker.

[zepumph]

Sounds good to me. Ready to close?