1097906 - Reader icon briefly appears for non-reader pages

Status: VERIFIED FIXED

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

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Go to nytimes.com.
2) Click a reader-enabled article.
3) Click back.

After step 3, the reader icon shows up for a second or two before disappearing.

Comment 1

[:Margaret Leibovic]

Attachment: Be smarter about maintaining reader mode page action state

So the real problem here is that we're waiting until we find out there isn't an article on a page to reset the reader enabled state to false. We should just always default to false when we show a new page, then enable it after if we find an article.

However, while working on this issue, I found some other parts of logic we can improve. Instead of keeping track of a boolean flag that reflects whether or not reader mode is "active", we can just check the URL of the tab, since it's baked in there as "about:reader". Also, we don't need to parse the document if it's an about:reader tab!

I tested going back/forward as described in the STR, and going in/out of reader mode and switching tabs.

I want to try to make a test for this... maybe I can make a unit test that just pokes directly into the PageActions object. We should really freeze that...

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8522602 [details] [diff] [review]
Be smarter about maintaining reader mode page action state

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

Nice!

::: mobile/android/chrome/content/browser.js
@@ +4245,5 @@
>              return;
>            }
>  
>            this.savedArticle = article;
> +          this.readerEnabled = true;

Could we go a step further and remove the readerEnabled flag, assuming reader is enabled if savedArticle is non-null? I can't think of a situation where we would show the reader icon without having an article ready for the page.

Comment 3

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #2)

> ::: mobile/android/chrome/content/browser.js
> @@ +4245,5 @@
> >              return;
> >            }
> >  
> >            this.savedArticle = article;
> > +          this.readerEnabled = true;
> 
> Could we go a step further and remove the readerEnabled flag, assuming
> reader is enabled if savedArticle is non-null? I can't think of a situation
> where we would show the reader icon without having an article ready for the
> page.

Good call, I think that makes sense. Under the hood, "reader enabled" does really just mean "we have an article we can show for this tab".

Comment 4

[:Margaret Leibovic]

I also decided to get rid of the getters and just call Reader.updatePageAction directly. This patch makes me happy.

https://hg.mozilla.org/integration/fx-team/rev/53a90acc7611

Comment 5

[:Margaret Leibovic]

Oops, didn't read carefully when removing the readerActive getter, backed out:
https://hg.mozilla.org/integration/fx-team/rev/31e5be937d15

I'll land a fixed version.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/e4b4778ecf4d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4b4778ecf4d

Comment 8

[Teodora Vermesan (:TeoVermesan)]

On latest Nightly, after going to nytimes.com and accessing a reader-enabled article, going back, the reader mode icon is not displayed anymore, so verified fixed on:
Device: Nexus 5 (Android 4.4)
Build: Firefox for Android 36.0a1 (2014-11-16)

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on Firefox 36 Beta 6 on Nexus 4 (Android 4.4.2), using the steps from the above comment.