1165745 - Reading View Caching Issues

Status: RESOLVED FIXED

(Firefox for iOS :: Reader View, defect)

Description

[Boris Dušek]

Steps to Reproduce:
1. Open cnn.com
2. Click the first article you see
3. Verify it has reader mode available (Reader mode icon appears in URL bar)
4. Tap the "Back" button
5. Tap the "Forward" button, verify Reader mode is still available (be patient, it appears ~5 seconds after the webpage loads)
6. Tap the "Back" button
7. Tap the "Forward" button

Expected Results:
Reader mode is available (after ~5 seconds of patience).

Actual Results:
Reader mode is not available (Reader mode icon does not appear). It is the same site for which Reader mode was available in step 5. (You might need one to perform one more "Back"+"Forward" cycle to make this happen.)

Configuration:
iOS 8.3, both (12F70) (Device) and (12F69) (Simulator)

Notes:
This is due to "pageshow" event not being fired after some time (I suspect it is after the requests are started being served from the cache or what; pageshow and pagehide were introduced exactly because they should be fired when going into/out of cache, unlike load/unload). Not sure, but feels like a bug in WebKit?

This can be tested with e.g. putting alert("pageshow"); in the pageshow handler at the end of ReaderMode.js  - after some time of going back/forward (usually 2-3 cycles), it stops alerting. The same happens for "pagehide".

I discovered this actually when working on my LangInfoHelper for Bug 1158514, I too want to do work (extract language tags) on pageshow, and get the same issue.

As webView:didFinishNavigation: gets called reliably (i.e. even when pageshow is not called), an alternative could be calling the needed stuff from there using evaluateJavaScript:completionHandler:, the downside being that that would be circumventing the BrowserHelper abstraction a little bit (we would not be relying on WKScriptMessage handling and dispatching to helpers done in the Browser class), and we would unconditionally need to expose the helpers' JavaScript to the webpages (no one-time-executed anonymous functions, but functions would be needed to be available in some _firefox_<FeatureName> namespace) - but ReaderMode does that already anyway, so no big deal.

Comment 1

[Boris Dušek]

Btw. I just managed to run Firefox for iOS with latest upstream WebKit (r184443, 601.1.33) on iOS 8.3 Simulator, and the problem still persists.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Thanks Boris, I am investigating and working on a fix.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Boris, curious, are able to run the latest webkit on the device too?

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I added a console.log() at the end of ReaderMode.js and it seems the user script is not executed at all. It is not just pageshow not being fired, it is the page being cached.

So  yeah I think we need to move the call to _firefox_ReaderMode.checkReadability() to a different place unfortunately.

Comment 5

[Boris Dušek]

(In reply to Stefan Arentz [:st3fan] from comment #3)
> Boris, curious, are able to run the latest webkit on the device too?

No because I did not try. I did not try because I gave it 1% chance to work (I deemed that the device would be, unlike simulator, cautious not to allow things like making a new process etc., unless the framework would be signed by Apple's own certificate), but I might be wrong.

In the webkit's build scripts, there is explicit support for *building* for device (the promising argument --device), but there is also note in the function for *running* with the built webkit for iOS saying "Only running Safari in iOS Simulator is supported now." (See https://github.com/WebKit/webkit/blob/master/Tools/Scripts/webkitdirs.pm#L2300 )

It would be great for me to be able to run on device, as most accessibility stuff can be tested only on device with actual VoiceOver and Switch Control (Accessibility Inspector in the simulator is only for very basic testing).

Comment 6

[Boris Dušek]

(In reply to Stefan Arentz [:st3fan] from comment #4)
> I added a console.log() at the end of ReaderMode.js and it seems the user
> script is not executed at all. It is not just pageshow not being fired, it
> is the page being cached.

I discovered that console.log is unreliable with WebKit Inspector (it agrees in steps 1., 2., 3. below, in in step 4. 'pageshow' blinks very quickly and then disappears and it does not show anything for 4., 5., 6., 7.), and alert is reliable.

My results with placing alerts into 'load' event, 'pageshow' event, and at the end of the script (I called that alert 'explicitload'):

1. Open cnn.com: 'explicitload', 'load', 'pageshow'
2. Open article: 'explicitload', 'load', 'pageshow'
3. back: 'explicitload', 'load', 'pageshow'
4. forward: 'pageshow'
5. back: 'pageshow'
6. forward: (nothing)
7. back: (nothing)

So pageshow does work correctly in 4. and 5. where the page is apparently being served from cache (and 'load' and 'explicitload' are not fired), i.e. 'pageshow' is behaving as it IMHO should. Just for some reason, it works only once. On subsequent back/forward navigation, it stops firing.

I would file a bug with webkit if I had easy repro of this with Safari on OS X (I tried).

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/487

This patch moves the call to checkReadability() from the pageShow event handler to the native code. This is because the event works unreliable because of caching issues. (This is likely a bug in WebKit since pageShow should always fire, even on cached pages)

Comment 8

[Stephan Leroux [:sleroux]]

Comment on attachment 8608863 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/487

LGTM!