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.
MIT License
8 stars 6 forks source link

ScreenView focus() call can steal focus from the parent page #897

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

@emily-phet just reported that this logic is causing trouble with a partner when trying to have phetsims load as one of many resources on a page. It will steal focus away from other parts of the parent frame. Here is the report:

Not sure where to ask this question, or to whom…it’s from colleague, Steven Clontz, in the POSE community who is attempting to support PhET sim embedding in an open source textbook creation platform called PreTeXt: “We’ve implemented embedding PhET interactives into PreTeXt, but they seem to have a strange behavior that hijacks the browser, causing the viewport to jump to the interactive when it loads, even if it’s just one element within a page of many other features, see https://pretextbook.org/examples/sample-article/annotated/section-interactive-server.html and wait a few seconds.This makes using PhET interactives non-ideal unless they live on a page dedicated to a single interactive”. Does this behavior seem like a known issue or something else? It seems surprising and non-ideal, as Steven notes. (I’m not requesting a solution if it’s an issue on our end, I just want to be able to respond to Steven).

It is easy to reproduce this with this script. iframes can steal their parent frame's focus:

parent:

<button>one</button>
<button>two</button>

<iframe src="iframe.html"></iframe>
<button>three</button>
<script>setInterval( () => {console.log( document.activeElement );}, 1000 ) </script>

iframe

<button id="hi">hi</button>
<script>setInterval( () => {
  console.log( 'focusing iframe button' );
  document.getElementById( 'hi' ).focus();
}, 3000 )</script>
zepumph commented 1 year ago

@emily-phet can you please state how you think we should proceed with this? A partner reported it, should we work out a patch that can be maintenance released? Should we first work towards fixing this bug on master? Now that we see that it is in result of a feature, and not a bug, will you comment on how you are thinking about this at this time?

samreid commented 1 year ago

@jessegreenberg said:

I haven't tested but I am guessing this is happening because we focus the top of the ScreenView when the sim loads

Can you please comment on why we trigger focus on load? Is that conventional for websites to do? Is it a workaround for something? What would go wrong if we stopped triggering focus on load?

UPDATE: @jessegreenberg already said:

We started doing this so that the screen reader starts at the top of the screen every time you switch screens. Maybe we could make it so it doesn't start doing this until after the sim loads so focus doesn't move until there is user input.

I guess I still don't fully understand. Why do we have to trigger focus for the screen reader to start at the top? Could we do that only when you change screens, and not on startup?

jessegreenberg commented 1 year ago

Could we do that only when you change screens, and not on startup?

Yes! I think that would fix this, that is what I was trying to get at in my second sentence there. @zepumph also suggested

perhaps improve the logic to detect when we (the phet sim iframe) has focus.

and that also seems like a good way to do this. This could be done fundamentally in scenery, like maybe our ParallelDOM.focus() could noop if the document is not active. Just brainstorming.

Removing my assignment for thoughts from @emily-phet, but this does seem like something we can/should fix.

zepumph commented 1 year ago

perhaps improve the logic to detect when we (the phet sim iframe) has focus.

Not sure what logic we are talking about. If our only tool is document.activeElement, the most information we have is that when somewhere else outside the iframe is focused, our iframe's document.activeElement is the document.body. It is the body in a lot of other cases too.

jessegreenberg commented 1 year ago

I started https://github.com/phetsims/scenery/issues/1516 to brainstorm how to detect when the window has focus and think there may be overlap with https://github.com/phetsims/quadrilateral/issues/311.

StevenClontz commented 1 year ago

Thanks @emily-phet for connecting me to this issue during our conversation today - I'm the member of the @PreTeXtBook community who reported this issue.

I did just find https://stackoverflow.com/a/11969151#comment128118788_11969151 as a possible workaround on our end, but it looks like your team may be on track for a more elegant solution.

Of course, if there's a more elegant way to embed these applets into a larger webpage/app besides an iframe, we might look into that as well. Thanks!

zepumph commented 1 year ago

@jessegreenberg are these failing PDOM unit tests from this change?

scenery : top-level-unit-tests : unbuilt
http://127.0.0.1/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-tests.html
395 out of 396 tests passed. 1 failed.
ParallelDOMTests: Next/Previous focusable failed:
a in focus (next)
at Object.<anonymous> (http://127.0.0.1/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:936:10)

ParallelDOMTests: Next/Previous focusable failed:
b in focus (next)
at Object.<anonymous> (http://127.0.0.1/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:938:10)

ParallelDOMTests: Next/Previous focusable failed:
c in focus (next)
at Object.<anonymous> (http://127.0.0.1/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:940:10)

ParallelDOMTests: Next/Previous focusable failed:
d in focus (next)
at Object.<anonymous> (http://127.0.0.1/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:942:10)

id: Sparky Puppeteer
Snapshot from 5/18/2023, 8:19:46 AM

----------------------------------

scenery : top-level-unit-tests : unbuilt?brand=phet-io
https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-tests.html?brand=phet-io
398 out of 399 tests passed. 1 failed.
ParallelDOMTests: Next/Previous focusable failed:
a in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:936:10)

ParallelDOMTests: Next/Previous focusable failed:
b in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:938:10)

ParallelDOMTests: Next/Previous focusable failed:
c in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:940:10)

ParallelDOMTests: Next/Previous focusable failed:
d in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:942:10)

[URL] https://sparky.colorado.edu/continuous-testing/aqua/html/qunit-test.html?url=..%2F..%2Fct-snapshots%2F1684419586833%2Fscenery%2Fscenery-tests.html%3Fbrand%3Dphet-io&testInfo=%7B%22test%22%3A%5B%22scenery%22%2C%22top-level-unit-tests%22%2C%22unbuilt%3Fbrand%3Dphet-io%22%5D%2C%22snapshotName%22%3A%22snapshot-1684419586833%22%2C%22timestamp%22%3A1684421260962%7D
[NAVIGATED] https://sparky.colorado.edu/continuous-testing/aqua/html/qunit-test.html?url=..%2F..%2Fct-snapshots%2F1684419586833%2Fscenery%2Fscenery-tests.html%3Fbrand%3Dphet-io&testInfo=%7B%22test%22%3A%5B%22scenery%22%2C%22top-level-unit-tests%22%2C%22unbuilt%3Fbrand%3Dphet-io%22%5D%2C%22snapshotName%22%3A%22snapshot-1684419586833%22%2C%22timestamp%22%3A1684421260962%7D
[NAVIGATED] about:blank
[NAVIGATED] https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-tests.html?brand=phet-io
[CONSOLE] Failed to load resource: the server responded with a status of 404 (): https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-strings_en.json
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently

id: "Bayes Node Puppeteer"
Snapshot from 5/18/2023, 8:19:46 AM

----------------------------------

scenery : top-level-unit-tests : unbuilt?ea
https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-tests.html?ea
395 out of 396 tests passed. 1 failed.
ParallelDOMTests: Next/Previous focusable failed:
a in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:936:10)

ParallelDOMTests: Next/Previous focusable failed:
b in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:938:10)

ParallelDOMTests: Next/Previous focusable failed:
c in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:940:10)

ParallelDOMTests: Next/Previous focusable failed:
d in focus (next)
at Object.<anonymous> (https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/chipper/dist/js/scenery/js/accessibility/pdom/ParallelDOMTests.js:942:10)

[URL] https://sparky.colorado.edu/continuous-testing/aqua/html/qunit-test.html?url=..%2F..%2Fct-snapshots%2F1684419586833%2Fscenery%2Fscenery-tests.html%3Fea&testInfo=%7B%22test%22%3A%5B%22scenery%22%2C%22top-level-unit-tests%22%2C%22unbuilt%3Fea%22%5D%2C%22snapshotName%22%3A%22snapshot-1684419586833%22%2C%22timestamp%22%3A1684420941349%7D
[NAVIGATED] https://sparky.colorado.edu/continuous-testing/aqua/html/qunit-test.html?url=..%2F..%2Fct-snapshots%2F1684419586833%2Fscenery%2Fscenery-tests.html%3Fea&testInfo=%7B%22test%22%3A%5B%22scenery%22%2C%22top-level-unit-tests%22%2C%22unbuilt%3Fea%22%5D%2C%22snapshotName%22%3A%22snapshot-1684419586833%22%2C%22timestamp%22%3A1684420941349%7D
[NAVIGATED] about:blank
[NAVIGATED] https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-tests.html?ea
[CONSOLE] Failed to load resource: the server responded with a status of 404 (): https://sparky.colorado.edu/continuous-testing/ct-snapshots/1684419586833/scenery/scenery-strings_en.json
[CONSOLE] enabling assert
[CONSOLE] Assertion failed: tagName: input does not support inner content
[CONSOLE] Assertion failed: tagName: input does not support inner content
[CONSOLE] Assertion failed: setPDOMAttribute does not support association attributes
[CONSOLE] Assertion failed: setPDOMAttribute does not support association attributes
[CONSOLE] Assertion failed: setPDOMAttribute does not support association attributes
[CONSOLE] Assertion failed: inputType does not support checked: range
[CONSOLE] Assertion failed: If both visible and visibleProperty are provided, then values should match
[CONSOLE] Assertion failed: If both pickable and pickableProperty are provided, then values should match
[CONSOLE] Assertion failed: If both enabled and enabledProperty are provided, then values should match
[CONSOLE] Assertion failed: If both inputEnabled and inputEnabledProperty are provided, then values should match
[CONSOLE] Assertion failed: targetProperty must be settable
[CONSOLE] Assertion failed: targetProperty must be settable
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently
[CONSOLE] Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently
[CONSOLE] Assertion failed: getUniqueTrail found a Node with 2 parents.
[CONSOLE] Assertion failed: getUniqueTrail found 0 matching trails for the predicate
[CONSOLE] Assertion failed: getUniqueTrail found 0 matching trails for the predicate
[CONSOLE] Assertion failed: getUniqueLeafTrail found 3 matching trails for the predicate
[CONSOLE] Assertion failed: getUniqueLeafTrail found 0 matching trails for the predicate
[CONSOLE] Assertion failed: getUniqueLeafTrail found 6 matching trails for the predicate

id: "Bayes Node Puppeteer"
Snapshot from 5/18/2023, 8:19:46 AM
jessegreenberg commented 1 year ago

Yes, I think so. Though unit tests are passing for me locally on Chrome and Firefox. This is probably another case of https://github.com/phetsims/aqua/issues/134.

jessegreenberg commented 1 year ago

Thanks for pointing out @zepumph! A way to handle this for puppeteer was added above and scenery unit tests are passing again on CT. We should try to find a better way in https://github.com/phetsims/aqua/issues/134.

jessegreenberg commented 1 year ago

@StevenClontz thank you for reporting this and bringing it to our attention! We have fixed this and republished our sims so this does not happen anymore.

This is no longer happening in the context you provided at https://pretextbook.org/examples/sample-article/annotated/section-interactive-server.html.

Closing.

zepumph commented 1 year ago

Amazing. Thank you so much.