rust-lang / mdBook

Create book from markdown files. Like Gitbook but implemented in Rust
https://rust-lang.github.io/mdBook/
Mozilla Public License 2.0
18.34k stars 1.64k forks source link

misc mobile safari issues in private browsing #317

Closed msporleder closed 7 years ago

msporleder commented 7 years ago

Many code snippets, which are supposed to run in the playground, simply do not work and render the "hidden" parts of their code, causing newbies like me great confusion.

# #![allow(unused_variables)]
#
#fn main() {

as found in enums.html, etc

When using private browsing on mobile safari (iphone) local storage is not available so: localStorage.setItem('theme', theme); throws an error. I think modernizr might have a check for this? ([https://github.com/Modernizr/Modernizr/blob/master/feature-detects/storage/localstorage.js](modernizr localstorage.js)

Also the hamburger menu does not click, which is probably because the localstorage thing.

azerupi commented 7 years ago

Are all those issues only occurring in private browsing sessions? Is there some extra limitations to JavaScript when running in private sessions? I would have to check how code hiding is implemented again. But as far as I can remember it is just using JavaScript with some JQuery. I'm not sure why it wouldn't work unless JavaScript is disabled.

budziq commented 7 years ago

hi @msporleder

msporleder commented 7 years ago

@budziq

I'm just browsing the (released today) rust book. I was complaining in IRC and was asked to open bugs here.

When I referenced 'enums.html' I was talking about: https://doc.rust-lang.org/book/first-edition/enums.html ; sorry for not being more specific.

msporleder commented 7 years ago

@azerupi private browsing is the cause of the localstorage failure as it is, basically, not allowed in private mode mobile safari. use a cookie instead, probably.

Private window desktop safari shows the same behavior of the menu buttons not working.

Anyway I was able to add a function using vanilla js document.getElementById("sidebar-toggle").addEventListener('click', function() { ...

googling around it looks like this is a long-standing jquery bug. If you want to stick with jquery the .on() might work better.

azerupi commented 7 years ago

Ok, thank you for the details! :) I don't have an iPhone to test on, but I can have a look at the bugs that also appear on dekstop safari. If it is really JQuery at fault then we should reduce our usage of it and return to plain javascript. It may however be just a strange safari bug (see stack overflow thread linked below).

For the localstorage, we should indeed check and fall back to a default if it is not present.

Note for myself: Maybe relevant stack overflow answer

budziq commented 7 years ago

I suspect that it is in fact problem with local storage most likely an exception raised early here: https://github.com/azerupi/mdBook/blob/master/src/theme/book.js#L13 (it internally calls localStorage.setItem('theme', theme);) and rest of the script never gets processed (including code hiding and sidebar toggling). Sidebar toggle also uses localStorage.setItem which might be the cause of button not being responsive.

Most likely wrapping the calls to localStorage in a function and handling the exception internally would solve the problem but unfortunately I have no access to MacOS / IOS device to verify :disappointed:

msporleder commented 7 years ago

Luckily millions and millions of people have iphone or safari on their macs to test. :)

budziq commented 7 years ago

hi @msporleder would you be willing to test on https://budziq.github.io/ - it is my private fork of rust-cookbook with POC local storage fallback to cookies (I've no way of reliably testing it). It should also autoclose the sidebar on mobiles.

msporleder commented 7 years ago

404

On Jun 10, 2017, at 4:15 AM, Michał Budzyński notifications@github.com wrote:

hi @msporleder would you be willing to test on https://budziq.github.io/ - it is my private fork of rust-cookbook with POC local storage fallback to cookies (I've no way of reliably testing it). It should also autoclose the sidebar on mobiles.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

budziq commented 7 years ago

@msporleder

404 sry i was posting from mobile and pasted half of url. please try with : https://budziq.github.io/rust-cookbook/

msporleder commented 7 years ago

Looking good. Now my other issue of getting stuck in the nav bar works in both private and non.

Do you have any playground links I can test the other parts with?

budziq commented 7 years ago

@msporleder can you also check if:

EDIT: I've updated the link to point to both expandable and runnable code.

budziq commented 7 years ago

For future reference the localStorage polyfill used to render the example pages was adapted from the first example on MDN.

Please note that this is just a POC at the moment more suited to diagnose the problem than actual production. Most likely the final implementation should either use the second (less verbose and more backward compatible solution from MDN or a separate library)

msporleder commented 7 years ago

Okay. Mobile safari private and regular now work pretty much as expected.

However you have broken desktop safari's (private and regular) ability to click the hamburger or the theme selector! It throws the following error: book.js: 250 let text = code_block.find(".language-rust").text(); is causing an error SyntaxError: Unexpected identifier 'text'

budziq commented 7 years ago

Uncool but looking via browserling.com i see that the problem exists in mainline mdbook not in the POC for this issue.

@msporleder If this (hamburger menu and theme selector in safari desktop error) problem exists in the mainline pages then please open a separete issue.

azerupi commented 7 years ago

A very recent PR has been merged to make the JS ES5 compliant again (somehow non ES5 JS got through review), maybe that is the issue here because it seems to throw the error on the same line that has changed in the PR?

budziq commented 7 years ago

@azerupi my POC variant did not include the latest PR for ES5 compatibility (I've missed it as some of my older PR's are still queued for review). It still uses let instead of var binding.

I'll try to find sone time tommorow, rebase my POC and publish new sample for testing. If everything will be ok and there is interest I can submit a PR for review.

azerupi commented 7 years ago

I've missed it as some of my older PR's are still queued for review

Yeah, it was a very small PR so it was quick to review in between my revision sessions. I will try to merge the other PRs in the queue when I'm done with my exams next week :wink:

there is interest I can submit a PR for review

For sure! I'm always open for bug fixes.

budziq commented 7 years ago

I will try to merge the other PRs in the queue when I'm done with my exams next week 😉

Sorry. I meant to excuse myself for missing that crucial PR rather than push for attention on mine. Don't worry, take your time and good luck :)

budziq commented 7 years ago

Hi,

I've rebased to latest master and published a testing sample https://budziq.github.io/mdBook/format/rust.html (from checks on windows safari via browserling.com it seams that hamburger menu started working)

@msporleder are you up for another round of testing :smiley_cat: ?

azerupi commented 7 years ago

I just tried on Safari and the hamburger menu works! I just realised that mdBook is completely broken on private navigation sessions in safari...

Normal session (safari) image

Private session (safari) image

Private session (chrome) image

msporleder commented 7 years ago

Phone is working as expected still (good) but safari private desktop is still broken, probably because of a security violation "Attempting to change the getter of an unconfigurable property." on

function installLocalStorageFallback() {
  Object.defineProperty(window, "localStorage", new (function () {

I'm guessing safari desktop private does not allow access to the window object like that but quick googling did not find anything to back me up.

budziq commented 7 years ago

darn you Apple ;)

well probably indirecting all storage requests through an additional global object might be enought to circumvent this problem.

All in all will most likely need to dustup my windows dualboot install safari to actualy solve this one.

On 11 Jun 2017 7:26 p.m., "msporleder" notifications@github.com wrote:

Phone is working as expected still (good) but safari private desktop is still broken, probably because of a security violation "Attempting to change the getter of an unconfigurable property." on

function installLocalStorageFallback() { Object.defineProperty(window, "localStorage", new (function () {

I'm guessing safari desktop private does not allow access to the window object like that but quick googling did not find anything to back me up.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/azerupi/mdBook/issues/317#issuecomment-307643780, or mute the thread https://github.com/notifications/unsubscribe-auth/AANfSDxwvzy2Tcpfu-RVKIxDStbkKCioks5sDCNQgaJpZM4N0lZ3 .

budziq commented 7 years ago

@azerupi current build of my POC should work around problems with chromium blocked cookies / local storage https://github.com/azerupi/mdBook/issues/322 and mobile safari private mode (but the security violation in desktop safari private mode is not addressed yet).

I have an idea how to workaround safari limitations here but I'm afraid that there is a compatibility can of worms yet to discover. I think that I'll check https://github.com/marcuswestin/store.js for a more complete solution instead.

azerupi commented 7 years ago

@budziq I just tried your link and it is working just fine in Safari private session now. So I guess you managed to fix it? 😄

budziq commented 7 years ago

@azerupi actualy there are now two variants and both have their problems:

I feel that the store.js is a way to go and it just needs a little more tweaking. If someone with access to safari could verify then it would be great help (I guess that I'll find some time to install it later this week ).

Right... I only checked now but it seams that safari private mode support is asses to store.js recently (v.2.x.x) and the newest version available from cdnjs is 1.3.20 darn... I guess that we'll have to vendor the latest build ourselves

budziq commented 7 years ago

@azerupi I've made a preliminary PR https://github.com/azerupi/mdBook/pull/324 with vendored store.js v2.0.3 as I was not able to find any problems with it.

@msporleder would you be able to test once more on the latest variant. I am reasonably hopeful in regard to desktop safari but there was no way to verify on mobile (and there is a bugreport on their repo https://github.com/marcuswestin/store.js/issues/232 describing the exact problem we were starting with which would be a shame)

msporleder commented 7 years ago

things look much better on mobile and desktop safari private mode. The menus work and the #fn main's are hidden (the play button is missing but I am not sure if that is intentional or not).

I'd say this is good enough.

budziq commented 7 years ago

@msporleder thanks for all your help!

The missing buttons are in no way intentional but it looks like another bug. The latest master without my changes to localStorage shows identical behaviour (actually on iphone 6 I was able to borrow for a few minutes and on browserstack.com all of playpen buttons are hidden both on mobile safari / chromium standard and private mode). The buttons are visible only if the theme does not load properly (and then they are at the left instead of right side of playpen). I'll open another issue.