775346 - Some page interactivity lost until reader mode icon has loaded

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

1) Open Firefox (17).
2) Go to http://en.m.wikipedia.org/wiki/United_States (or any other reader mode compatible page with a lot of content).
3) Click the "US (disambiguation)" link (or any other) before the reader mode icon appears in the URL bar

Expected: The browser begins to load the selected link.
Actual: Nothing. The page can scroll but links are not unclickable.

The links will be clickable when the reader mode icon loads in the URL bar.

Therefore, links cannot be clicked until the page is fully loaded as the reader mode icon is the last part to load. This gives an impression of unresponsiveness to the user. I assume the effect will be amplified on slow devices and slow connections.

Another issue I found is that double-tap to zoom does not work until the icon has loaded (you can try on the page listed above).

Also, when loading a new page, the "X" in the URL bar to stop loading the page seems to be unresponsive after some point in the loading process – this could be related as it does not appear in FF16.

Tested on Galaxy Nexus, Android 4.0.4.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Since the reader check is happening in the Gecko JS thread (the main thread), all other Gecko activity is blocked until the check is finished.

Clicking links and double-taps are handled by the Gecko JS code.

We could look into moving the reader check into a chrome worker thread, or some other way of yielding the event loop.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Use generator function in grabArticle() to yield readability parsing

A web worker would be nice, but unfortunately, web workers cannot use the DOM [1] (including a DOMParser). I was going to try creating a separate thread in the core and try doing this in a sandbox there, but separate threads cannot touch the DOM [2] (which is the same reason the DOM isn't available to web workers).

Using a combination of generator functions and setTimeout(), I broke up the readability parsing, allowing it to periodically yield so other items in the event queue can be processed. This required making parts of Readability async.

Based on my testing, 100 iterations per batch seems to work pretty well (parsing/check times are approximately the same, and the page is still functional).

[1] https://developer.mozilla.org/en/Using_web_workers#About_thread_safety
[2] https://developer.mozilla.org/en/Code_snippets/Threads#Creating_a_real_thread

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Use generator function in grabArticle() to yield readability parsing, v1.1

Some minor cleanup.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 645531 [details] [diff] [review]
Use generator function in grabArticle() to yield readability parsing, v1.1

I'll break this patch up a bit for easier review.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Make grabArticle() non-recursive

This patch simply removes the recursion from grabArticle() by adding a while loop. Removing the recursion simplifies the process of making it a generator function.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Convert grabArticle() to a generator function

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Make Readability asynchronous

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 645550 [details] [diff] [review]
Part 1: Make grabArticle() non-recursive

Review of attachment 645550 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 645554 [details] [diff] [review]
Part 2: Convert grabArticle() to a generator function

Review of attachment 645554 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Very smart change Brian :-)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 645556 [details] [diff] [review]
Part 3: Make Readability asynchronous

Review of attachment 645556 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome. Now that article parsing doesn't block the JS execution for long period, we should probably make check() run more of grabArticleGenerator() instead of simply returning when a good candidate is found. We're getting a lot of false positives because check() doesn't match the full parse() in some ways. This can be done as a follow-up in bug 767956 though.

Comment 11

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/58f8e3015071
http://hg.mozilla.org/integration/mozilla-inbound/rev/474db0a1661c
http://hg.mozilla.org/integration/mozilla-inbound/rev/58811938c229

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58f8e3015071
https://hg.mozilla.org/mozilla-central/rev/474db0a1661c
https://hg.mozilla.org/mozilla-central/rev/58811938c229
https://hg.mozilla.org/mozilla-central/rev/038c90c22adf

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: browser can freeze while document is being checked
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

This patch has actually been obsoleted by bug 779796, but I'm requesting approval on this one to make 779796 easier to uplift.

Comment 14

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0d83353adfe