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

Adding M&S to "Reading List" on iPad 2 doesn't work #301

Closed
ghost opened this issue Jul 10, 2018 · 17 comments
Closed

Adding M&S to "Reading List" on iPad 2 doesn't work #301

ghost opened this issue Jul 10, 2018 · 17 comments
Labels

Comments

@ghost
Copy link

ghost commented Jul 10, 2018

For phetsims/qa/issues/129.

On iOS 9.3.5 + Safari 9.0, adding M&S to "Reading List" (HTML download) doesn't work.

When I try to interact with the simulation, this happens:

img_0201

The sim is stuck like this and is otherwise unresponsive.

I will tether the iPad tomorrow morning and look into this further...

@ghost ghost added the type:bug label Jul 10, 2018
@ghost ghost assigned Denz1994 Jul 10, 2018
@ghost
Copy link
Author

ghost commented Jul 11, 2018

I'd like to make a retraction: the "Reading List" feature does work as expected with two caveats...

Caveat 1:

img_0203

For some reason there are numbers before the description of the simulation. I am seeing the same thing with Area Model Algebra 1.0.0-rc.3.

Caveat 2: The splash screen sometimes looks a bit odd, i.e. not centered. But this resolves itself.

What I was seeing in my first comment was simply abysmal performance. Performance is similar for other multi-screen sims.

I tethered the iPad, and there aren't any errors other than those related to not being connected to the internet.

I can look into this further, but I want a go-ahead. (It's such a small and obscure thing; I'm not sure it warrants further investigation.)

@phet-steele
Copy link
Contributor

phet-steele commented Jul 11, 2018

For some reason there are numbers before the description of the simulation.

Those numbers are related to a11y and are expected to be there (pretty sure), albeit it's ugly and unfortunate that they have to be seen. I don't think the work to remove those numbers is worth the benefit.

@arouinfar
Copy link

Those numbers are related to a11y and are expected to be there (pretty sure), albeit it's ugly and unfortunate that they have to be seen. I don't think the work to remove those numbers is worth the benefit.

Agreed. I don't think it's worth the effort to remove the numbers.

Caveat 2: The splash screen sometimes looks a bit odd, i.e. not centered. But this resolves itself.

@lmulhall-phet does this happen for other multi-screen sims, or is it specific to MAS?

@arouinfar arouinfar assigned ghost Jul 18, 2018
@ghost
Copy link
Author

ghost commented Jul 18, 2018

The splash screen weirdness seems to be a problem with other multi-screen sims as well.

@ghost ghost removed their assignment Jul 18, 2018
@arouinfar
Copy link

Thanks @lmulhall-phet! In that case, I don't think @Denz1994 needs to do anything specific for MAS.

That said, there should probably be some general issue in one of the common repos documenting the non-centered splash screen. Not sure what the best fit would be, so leaving that to @Denz1994 and/or @lmulhall-phet to decide.

@arouinfar arouinfar assigned ghost Jul 18, 2018
@ghost
Copy link
Author

ghost commented Jul 18, 2018

Will do @arouinfar.

(EDIT: Note to self: I can't reproduce this consistently. The behavior is different each time a load a sim.)

@Denz1994
Copy link
Contributor

Denz1994 commented Jul 19, 2018

@lmulhall-phet
Caveat 1: Agreed with the above comments regarding a11y.
Caveat 2: Perhaps we can make a common code issue in Joist for this? If this is a browser issue it shouldn't block publication or further RC testing.

Any thoughts to continue with this issue? @ariel-phet

@ariel-phet
Copy link

@Denz1994 this is definitely not a sim-specific issue, would be good to discuss at dev meeting to see where to move this issue, and understand what it would take to fix. I do not see it as blocking publication (people adding to the reading list is pretty rare).

@ariel-phet ariel-phet assigned Denz1994 and unassigned ariel-phet, Denz1994 and ghost Jul 19, 2018
@jessegreenberg
Copy link
Contributor

Regarding the numbers, I don't think they are for a11y, but maybe they are related to Heartbeat.js.

@samreid
Copy link
Member

samreid commented Jul 19, 2018

In phetsims/joist#382 and phetsims/joist#470 we determined that "Save to Reading List" automatically saves a "snapshot" of the current DOM which is not automatically cleared when the page is loaded from the reading list. That is most likely what is causing the duplicate rendering and interference.

@samreid
Copy link
Member

samreid commented Jul 19, 2018

Regarding the numbers, I don't think they are for a11y,

Guessing here: maybe related to the cache buster?

@jessegreenberg
Copy link
Contributor

Also guessing, but Heartbeat.js adds a div to the document that looks like

<div>0.32540102738293863</div>

When a11y is enabled the PDOM comes before the heartbeat div in the document, which is probably why there are no numbers for RIAW in #301 (comment).

@samreid
Copy link
Member

samreid commented Jul 19, 2018

I agree that looks suspicious, and iOS may just be scraping the DOM to generate the preview text.

@samreid
Copy link
Member

samreid commented Jul 19, 2018

From today's discussion, the iOS app supercedes Reading Mode, and it would be good if clients can use the app instead.

@ariel-phet requested @samreid to take an hour and look into this.

@samreid samreid assigned samreid and ariel-phet and unassigned Denz1994 and samreid Jul 19, 2018
@samreid
Copy link
Member

samreid commented Jul 19, 2018

On second thought, @ariel-phet wants to investigate and discuss further with QA.

@ariel-phet
Copy link

Discussed with @lmulhall-phet - he is going to make a more general issue (since this is not sim specific). He will link that issue to this one, and close this M&S issue.

In the new issue @lmulhall-phet will try to layout more specific steps and issues to the reading list problems.

@ariel-phet
Copy link

General issue made, closing this one.

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

No branches or pull requests

6 participants