Fix ?allowLinks behavior

kathy-phet commented 1 year ago

There are a few places where links / opportunities for students to navigate away from the simulation are still available when ?allowLinks=false

Specifically, these links are still active:

PhETmenu--> PhET Website ... PhETmenu--> Report a Problem ...

We need a solution for this when allowLinks=false.

Options off the top of my head:

hide these options in the PhETmenu disable these options in the PhETmenu others???

jonathanolson commented 1 year ago

Also, any usage of openPopup should be checked (perhaps openPopup could check to see if the query parameter is provided?)

kathy-phet commented 1 year ago

Original issue: https://github.com/phetsims/joist/issues/592

kathy-phet commented 1 year ago

@zepumph - Can you review JO's fix to allowLinks? You actually recognized this need previously in the original issue - in this comment https://github.com/phetsims/joist/issues/592#issuecomment-565555239, but I think you were on vacation and then the thought got lost in the final review and sign off on that issue.

Behavior currently doesn't hide the option (e.g. PhET Website ...) will be visible, but no action is taken when you click on it. Nothing happens. @arouinfar and I felt this was fine behavior for this purpose. If you are a PhET-iO client, you can do something more sophisticated and hide the option or something, but for the PhET brand sims, we just want to prevent students from clicking their way away from the simulation.

@jonathanolson - I think you were also going to review and make sure this captured all of the cases that you needed to capture for redirects.

kathy-phet commented 1 year ago

@jonathanolson - also, this is a candidate for a maintenance release to PhET brand, once we are sure we have fixed all cases and are happy with the PhET brand behavior.

zepumph commented 1 year ago

Adding behavior for openPopup makes sense, but it doesn't capture the spirit of converting query parameters to behavior that is covered/duplicated by the PhET-iO API so that it can be mutated in PhET-iO and thus supported in the standard wrapper. To do that, this would need to run off of a Property, which is kinda a no-no in phet-core.

UPDATE: Noting that this issue is in part coming out of https://github.com/phetsims/phet-io/issues/1871 where we also want to have the PhET-iO support to control this query parameter.

zepumph commented 1 year ago

@jonathanolson and I discussed this, and in order to have allowLinks running from a Property, we are going to move openPopup to sun, so that it can depend on axon Property.

zepumph commented 1 year ago

Steps for this issue:

~Have ?allowLinks set the initialValue of a PhET-iO instrumented Property, that is customizable in studio. This shouldn't be in joist/ because we still want to be able to use the Property in openPopup~
~Convert usages of the allowLinks query parameter to instead dynamically adjust behavior based on the state of the Property~
~Move openPopup to a place where it can use the Property. @jonathanolson and I were ok with sun/, but were open to other ideas.~

Assigning to whoever gets to it first.

zepumph commented 1 year ago

@samreid, @arouinfar, @kathy-phet and I discussed this a bit more, and we have a new recommendation.

[ ] We want to move openPopup to scenery, and create the allowLinksProperty singleton in scenery.


new BooleanProperty( phet.chipper.queryParameters.allowLinks, {
  tandem: Tandem.ROOT.GENERAL_MODEL.createTandem( 'allowLinksProperty' )
} );

@kathy-phet asked us to also assign @jonathanolson to this issue.

@jonathanolson, please make sure to move the git history of openPopup. Please don't use the shell script because openPopup has been renamed to ts. Instead use copy-history-to-different-repo.js.

jonathanolson commented 1 year ago

Implemented above. Do we want to toggle all of the appearances that are based on allowLinks dynamically also?

zepumph commented 1 year ago

I thought so, but the original solution was to just no-op these phet menu buttons, instead of hiding them. The links in the about dialog should update though.

kathy-phet commented 1 year ago

I don’t know what’s easy. At a minimum, just not actually popping to a new window is fine. If easy, toggling visibility is nice so they aren’t there. Except for the urls in about where we just make them not linked.

Kathy

Sent from my iPhone

On Aug 24, 2022, at 5:15 PM, Michael Kauzmann @.***> wrote:

I thought so, but the original solution was to just no-op these phet menu buttons, instead of hiding them. The links in the about dialog should update though.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.

jonathanolson commented 1 year ago

Added dynamic updates in the commit above.

samreid commented 1 year ago

If easy, toggling visibility is nice so they aren’t there. Except for the urls in about where we just make them not linked.

What about graying it out?

@arouinfar agrees graying it out is a nice step.

@zepumph says it would be better to just remove it.

@samreid says: Sounds good to me! @arouinfar agrees visibleProperty is best.

@kathy-phet: Is this maintenance releasable? @jonathanolson: Yes, but I would implement it differently for the maintenance release. Would have to be careful not to disrupt phet-io APIs. @kathy-phet: Maybe just maintenance release by disabling the openPopup in its original location? @jonathanolson: Yes, that sounds best.

Assigning @zepumph to implement the "make it invisible" part and to @jonathanolson for the "maintenance release" part.

samreid commented 1 year ago

I'll do the "make it invisible" part, for the PhetMenu, and I'll search for openPopup to see if there are other cases. After this is done, the maintenance release part doesn't need to block Gravity and Orbits RC.

@zepumph says we should always create the items, but toggle visibility, so the API doesn't change.

samreid commented 1 year ago

Some observations:

The "Webgl is not working"/canvas/mobius dialog opens a popup when you click on it. Do we still want to show that dialog? I'm guessing yes.
Screenshots are generated with openPopup even they don't link to a website. Do we want to allow that? In the coming commit, I remove the screenshot button, but only because it would be broken (screenshot button does nothing).
How do we want the "update" dialog to work when links are not allowed? For now, I'll hide the button that links to the new version, but that means it will just show a "No Thanks" button.
RichText has a call to openPopup when it has an href. I wanted to tie it to the visibility of the Text like so: allowLinksProperty.link( allowLinks => {this.visible = allowLinks;} ); but that seems awkward--hiding the text when an outer button or other UI component should be hidden.
PhET Menu items already have a guard called present but it is not dynamic--only checked on startup.

samreid commented 1 year ago

I implemented a first draft and the design questions above should be discussed.

zepumph commented 1 year ago

Another design question:

Do we need an intermediate Property for PhET-iO to control the visibility of these menu items? Before PhET-iO clients could hide menu items they didn't want, but now this is derived solely based on allowLinks. I would consider this a regression of behavior, but it isn't clear that it is necessarily wrong. Like @samreid said, lets discuss today.

zepumph commented 1 year ago

Design questions are the quoted text, answers are below:

The "Webgl is not working"/canvas/mobius dialog opens a popup when you click on it. Do we still want to show that dialog? I'm guessing yes.

Yes.

Screenshots are generated with openPopup even they don't link to a website. Do we want to allow that? In the coming commit, I remove the screenshot button, but only because it would be broken (screenshot button does nothing).

Index: js/util/openPopup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/util/openPopup.ts b/js/util/openPopup.ts
--- a/js/util/openPopup.ts  (revision d39dc1d531533d3640c1ece96c3675a0bcb7c099)
+++ b/js/util/openPopup.ts  (date 1662058804078)
@@ -12,11 +12,11 @@
 /**
  * Opens the URL in a new window or tab.
  */
-function openPopup( url: string ): void {
+function openPopup( url: string, overrideAllowLinks = false ): void {

   // Don't allow openPopup IF we have query parameters AND allowLinks is false,
   // see https://github.com/phetsims/joist/issues/830
-  if ( allowLinksProperty.value ) {
+  if ( allowLinksProperty.value || overrideAllowLinks ) {
     const popupWindow = window.open( url, '_blank' ); // open in a new window/tab

     // We can't guarantee the presence of a window object, since if it isn't opened then it will return null.

How do we want the "update" dialog to work when links are not allowed? For now, I'll hide the button that links to the new version, but that means it will just show a "No Thanks" button.

Hide the update phetMenu item entirely. Ensure that there are no links in the about dialog if out of date.

RichText has a call to openPopup when it has an href. I wanted to tie it to the visibility of the Text like so: allowLinksProperty.link( allowLinks => {this.visible = allowLinks;} ); but that seems awkward--hiding the text when an outer button or other UI component should be hidden.

"no discussion needed" - Sam Reid

Do we need an intermediate Property for PhET-iO to control the visibility of these menu items? Before PhET-iO clients could hide menu items they didn't want, but now this is derived solely based on allowLinks. I would consider this a regression of behavior, but it isn't clear that it is necessarily wrong. Like @samreid said, lets discuss today.

We don't want to exclusively use allowLinks. You also need to be able to toggle visibility of the phet menu buttons manually for each button ("and" the two together).

pixelzoom commented 1 year ago

In the above commit, I fixed the allowLinks behavior for Molecule Polarity's "Real Molecules" screen. Needed to pass a Property to RichText, not a static string.

samreid commented 1 year ago

We don't want to exclusively use allowLinks. You also need to be able to toggle visibility of the phet menu buttons manually for each button ("and" the two together).

Testing Studio, I didn't see any buttons in the phet menu that are altered by allowLinks.

The "check for updates" button only shows in PhET brand
Report a problem only shown in PhET brand
PhET Website button only shown in PhET brand.

So perhaps no work is necessary for this part?

All other known issues addressed. @kathy-phet would you like to assign a reviewer?

kathy-phet commented 1 year ago

Hey Sam, Agreed - sounds like nothing is necessary!

@jonathanolson - Can you please review this? (Since you will need to maintenance release basic behavior in the easiest way possible ... we don't need maintenance release to restructure where OpenPopup is and all of that, but it would be great if maintenance release did not block screenshots and full screen).

zepumph commented 1 year ago

Noted today during PhET-iO meeting that this is still out for review.

zepumph commented 1 year ago

High priority (and why this is on the phet-io batch release board) is a review of the above commits.

MR release requirements: ?allowLinks query parameter prevent openPopup behavior generally, but make sure screenShots and other internal popups are allowed.

@kathy-phet says that the MR is not the highest priority. After other blocking items.

jonathanolson commented 1 year ago

Continuing my review, but @samreid can you check the above commit? I believe it would leak memory before when a RichText with a link is changed/created.

samreid commented 1 year ago

The memory fix looks good, thanks!

jonathanolson commented 1 year ago

For https://github.com/phetsims/joist/commit/df9633ae7cb3e1fc9e4c8ae4555b546935d13cc3, would visibleProperty: allowLinksProperty work and be simpler?

jonathanolson commented 1 year ago

https://github.com/phetsims/joist/commit/df9633ae7cb3e1fc9e4c8ae4555b546935d13cc3 also seems to include a change that triggers update checks from iOS-app versions of the sim. That change seems unrelated to what's done here, was that on purpose?

EDIT: nevermind, I see https://github.com/phetsims/joist/commit/c0ce854f4dd0f1dd8cc13294a277e7f04af02018

jonathanolson commented 1 year ago

Review completed. @samreid thoughts on https://github.com/phetsims/joist/issues/830#issuecomment-1245407193? (it's superficial)

samreid commented 1 year ago

https://github.com/phetsims/joist/issues/830#issuecomment-1245407193 looks like an improvement, but while I was looking, I saw that it looks like there is no unlink/dispose on allowLinksProperty.link( updatePanelAllowLinksListener );. We need that, right?

The reason I ask, is that I don't know how dispose would work in that case. How do you recommend to proceed?

samreid commented 1 year ago

We saw that disposal is not needed since the AboutDialog is not disposed after opening. I'll use the optimization @jonathanolson recommended, then close the issue.

samreid commented 1 year ago

Fixed, closing.

jonathanolson commented 1 year ago

Reopening to track for the maintenance release.

jonathanolson commented 1 year ago

Maintenance complete, closing.

phetsims / joist

Fix ?allowLinks behavior #830