Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pan the screen when pH paper has focus and floats to the top of the beaker. #237

Closed
pixelzoom opened this issue Sep 12, 2023 · 13 comments
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 12, 2023

Investigate whether phetsims/center-and-variability#539 and phetsims/scenery#1558 is a problem for this sim before creating the 1.3 release branch. If its is a problem, decide when/how to address it. Discuss with @jessegreenberg.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 12, 2023

pH meter, pH paper, and condictivity tester are the things that can be dragged with the keyboard. I tested them all when zoomed in (Ctrl+ on macOS), and the screen pans while dragging them with the keyboard.

I asked @jessegreenberg via Slack#DM:

I’m about to create a release branch for Acid-Base Solutions, and wanted to verify whether that sim has the same problem that was reported in phetsims/center-and-variability#539. I was surprised to find that it does not seem to be a problem in Acid-Base Solutions, at least with the things that have a KeyboardDragListener. In what cases is this a problem? And are there any upcoming changes that might be needed by Acid-Base Solutions?

@jessegreenberg
Copy link
Contributor

For most cases, panning should work well while using keyboard input. There are a few cases though that might not, and that is what we want to improve in phetsims/scenery#1558. Specifically in CAV, alt input for the "Predict Mean" slider was added to the entire slider and Scenery doesn't know yet to pan so that the thumb stays in view.

Anyway, I took a look at acid-base-solutions and almost everything is working as I would expect.

The only question is the PHPaperNode, which floats back to the top when you stop dragging it. The sim currently doesn't pan to keep it in view while it floats, but it will once phetsims/scenery#1558 is done.

In my opinion that is not blocking, but others might disagree.

@pixelzoom
Copy link
Contributor Author

I'd be OK with the sim not panning when the pH paper floats to the top. @arouinfar your feeling?

@jessegreenberg What is the ETA for completing phetsims/scenery#1558? How would you feel about patching the 1.3 release branch when scenery#1558 is completed? The ABS dev version is already 11 days old, so if we wait too much longer to create the release branch, we may need to do another dev test.

@jessegreenberg
Copy link
Contributor

I expect it to be done by Friday, Ill be working on it tomorrow and then will ask for some help from the QA team for some testing.

@arouinfar
Copy link
Contributor

I'd be OK with the sim not panning when the pH paper floats to the top. @arouinfar your feeling?

I agree @pixelzoom.

@arouinfar arouinfar removed their assignment Sep 12, 2023
@pixelzoom
Copy link
Contributor Author

I'm going to move forward with the 1.3 release branch. If we decide that we want to make panning track the pH paper when it floats to the top, we can patch the 1.3 branch (if that's possible before publishing) or do a maintenance release.

@pixelzoom pixelzoom changed the title Does panning occur when dragging objects with the keyboard? Pan the screen when pH paper has focus and float to the top of the beaker. Sep 13, 2023
@pixelzoom pixelzoom changed the title Pan the screen when pH paper has focus and float to the top of the beaker. Pan the screen when pH paper has focus and floats to the top of the beaker. Sep 13, 2023
@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Sep 15, 2023

@pixelzoom I have a question related to this for phetsims/qa#981. ....Why does the pH paper start to float to the top while it still has focus? Shouldn't it float once I tab away? It makes it a little awkward to move the pH paper around because it start to float to the top when I change direction.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 18, 2023

@arouinfar @Nancy-Salpepi and I discussed @Nancy-Salpepi's question in #237 (comment). We decided to keep the behavior the way it is, because moving the pH paper around is not the goal. The goal is to dip the paper in solution, immediately pull it out of solution, and compare it to the pH color key. The current behavior supports this, and also avoids the problem where the pH paper has focus but is hidden behind the magnifying glass -- a corner case that would be problematic in the Standard PhET-iO Wrapper.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 19, 2023

It looks like this has been addressed in scenery, see phetsims/scenery#1558 (comment). And I confirmed that it's working in Acid-Base Solutions. The pH paper floats to the top of the solution and retains focus.

@jessegreenberg After your review with @jonathanolson ... Would you prefer to patch this into the 1.3 branch prior to the next RC test? (1.3.0-rc.2, spot check) Handle as a maintenance release for multiple sims? Or defer until 1.4 is published in the future (which could be years from now).

@jessegreenberg
Copy link
Contributor

I would prefer to to publish without this enhancement and I am guessing 1.3 will be ready before phetsims/scenery#1558 is closed. I am not planning to do a batch of maintenance releases. But if that issue is closed up quickly I will put the SHAs to cherry-pick here.

@jessegreenberg
Copy link
Contributor

Over in phetsims/scenery#1558 we are going to add a couple more things but I am not going to be able to work on it again until next week, so I do recommend continuing with 1.3 if you are ready.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 20, 2023

Sounds good, thanks for the update @jessegreenberg.

@pixelzoom
Copy link
Contributor Author

@jessegreenberg preferred not to address this in 1.3, and he is not planning to do an MR. So this will have to wait until a future 1.4 release that picks up the fix from main. Since that's liable to be on the order of years, and the fix has already been verified in main, I see no reason to keep this issue open. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants