You shouldn't be able to focus invisible Nodes

[jessegreenberg]

I was surprised in phetsims/sun#718 that we were able to focus an invisible Node. We should assert that this is not allowed.

[zepumph]

Perhaps an assertion in peer.focus() when applying to a peer that is invisible?

scenery/js/accessibility/pdom/PDOMPeer.js

Lines 827 to 835 in 4152e13

	focus() {
	assert && assert( this._primarySibling, 'must have a primary sibling to focus' );
	// We do not want to steal focus from any parent application. For example, if this element is in an iframe.
	// See https://github.com/phetsims/joist/issues/897.
	if ( FocusManager.windowHasFocusProperty.value ) {
	this._primarySibling.focus();
	}
	}

[zepumph]

Would this be enough? We would need to test to make sure we weren't already accidentally hitting it.

Subject: [PATCH] add support for timeouts to fix infinite-loops,
---
Index: js/accessibility/pdom/PDOMPeer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMPeer.js b/js/accessibility/pdom/PDOMPeer.js
--- a/js/accessibility/pdom/PDOMPeer.js	(revision 4f40df00b43dd1655951bcedc83717efb06d262b)
+++ b/js/accessibility/pdom/PDOMPeer.js	(date 1708718884326)
@@ -826,6 +826,7 @@
    */
   focus() {
     assert && assert( this._primarySibling, 'must have a primary sibling to focus' );
+    assert && assert( this.isVisible(), 'cannot focus an invisible Node' );
 
     // We do not want to steal focus from any parent application. For example, if this element is in an iframe.
     // See https://github.com/phetsims/joist/issues/897.

[zepumph]

I felt confident enough after running a couple sims and aqua fuzzing with fuzzBoard. Can you please review?

[zepumph]

Nevermind. CT picked up trouble.

[zepumph]

OK. I found one obvious bug, and I love that this assertion helped catch it. We were calling focus() in cases where we were toggling the visibility of this node to false already. That wasn't the code path that the focus() call was originally intended for, so I was able to clean up the screen-reader specific listener. I'll check back on CT to see if any other issues show up.

[zepumph]

I found another one, where popupable doesn't know if its focusable is visible. This is making me wonder what the best solution is for this kinda of thing. I don't like my commit, though it is relatively safe. @jessegreenberg can you weigh in on the above two fixes that have accompanied this assertion.

[zepumph]

Ok. I'm going to comment this out one more time. I just found a third case on this line here. The selection of the combo box is hiding the combo box (which is fine for this case).

https://github.com/phetsims/sun/blob/ee93abec9729f56a2deadb3aec7c604050bea11d/js/ComboBox.ts#L307-L306

@jessegreenberg, I think a bit of conversation would be nice here. Let me know what you'd like to do.

[jessegreenberg]

Thanks for giving this a try @zepumph. I think the case you found in #1290 (comment) proves that we need to be lenient for this.

So for cases like this:

1) Push button with two listeners, each doing one of the following:
  a) `focus` is manually called on the button.
  b) Button is made invisible

(a) then (b) is great, (b) then (a) would cause assertion - I believe that is undesireable.

I did revert 2795e2726c1ae94f95993e49a6d379da02e6f4c2, but the rest seem like good changes to keep. I removed the comments pointing to this issue. Closing.