Closed Bug 1097906 Opened 10 years ago Closed 10 years ago

Reader icon briefly appears for non-reader pages

Categories

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

defect
Not set
normal

Tracking

(firefox35 unaffected, firefox36 verified, fennec36+)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- unaffected
firefox36 --- verified
fennec 36+ ---

People

(Reporter: bnicholson, Assigned: Margaret)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
tracking-fennec: ? → 36+
Assignee: nobody → margaret.leibovic
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...
Attachment #8522602 - Flags: review?(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.
Attachment #8522602 - Flags: review?(bnicholson) → review+
(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".
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
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.
https://hg.mozilla.org/mozilla-central/rev/e4b4778ecf4d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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)
Status: RESOLVED → VERIFIED
Verified fixed on Firefox 36 Beta 6 on Nexus 4 (Android 4.4.2), using the steps from the above comment.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: