Closed zepumph closed 6 days ago
Using this patch, along with ?sceneryLog=Input
, I am able to confirm that when you tab from a grabbed block to another block, that new block doesn't get the focus event. I believe that is because the whole PDOM is redrawn. When I cancel out the swapping of tag names such that we don't need to redraw, it fixes the problem.
@jessegreenberg, I would like to discuss this with you to see if we should get creative in GrabDragInteraction, or try to better understand focus in PDOM peer here. This bug would apply to any spot in phet code when a tab triggers PDOM content to change and the PDOM to redraw. Have we encountered this before?
Great find on the re-rendering, I think that is correct. The focus event is being blocked by BrowserEvents.blockFocusCallbacks
:
@zepumph worked to track down what is going on. The problem is that this line prevents focus from being restored correctly after the PDOM update:
This change was added in https://github.com/phetsims/scenery/issues/1296 (though it now lives in FocusManager).
Basically the PDOMPointer is set up to null out the FocusManager.pdomFocus in between each blur/focus pair. So when tabbing from (a) to (b), the FocusManager acts like it went from (a) to null to (b).
The original issue was created to support PhET-iO playback. But it continues to describe cases of changing the PDOM/moving focus that should be supported. Id like to try removing the workaround and making sure that all these cases still work.
@zepumph and I paired on this again. We identified that it was buggy for PDOMTree to manipulate focus while scenery is actively responding to focus and blur events. We added a flag that is true while focus/blur events are being dispatched and prevents PDOMTree from changing focus when this is true.
I tested the following with this change
We are worried this problem will happen for other events too. For example, if a keydown event on a button re-renders it, will it still receive a click event? Ill test that next and report what I find here.
To test the keydown/click issue, I added this to ResetAllButton and I was happy to see that the click
event was coming through nicely. Tested in Chrome and FF.
this.addInputListener( {
keydown: () => {
this.innerContent = null; // to avoid assertion
this.tagName = 'input';
this.inputType = 'button';
this.accessibleName = Math.random() + '';
this.tagName = 'button';
}
} );
I added logging to make sure the PDOM is being re-rendered, and I verified that the element for the ResetAllButton was different before/after pdomContentChange
.
Full patch:
This is good news!
I am sure there are other cases to worry about but this helps me rest easier.
I tried adding a unit test for this but could not think of a way to simulate a keydown event that sends both a keydown and click event at the same time. The result was dispatching a click event after the re-render, which should certainly work.
@zepumph reassigning this back to you. What else do you think should be done/tested for this issue?
Excellent patch! I expanded upon it to get a notion of what I was wondering about. Basically that long and the short of it is that we need to be SOOOOO CAREFUL when updating PDOM content during input listeners, because it can totally change the way the browser dispatches the next event. I wrote notes directly in the patch, but let me know if you want to discuss more synchronously. Should we try to add this into documentation somehow? Not sure how to do so in a way that we will actually remember or care in the future, but it seems like a scary gotcha.
Doc added. Closing
While working on https://github.com/phetsims/scenery-phet/issues/869.
To reproduce: