1300532 - Contextual hint about offline bookmarks is displayed in normal view instead of reader view

Status: RESOLVED FIXED

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

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: reader view.png

Steps to reproduce:
1. Go to news.google.com and open 3 articles in different tabs
2. Tap the reader view icon from the URL Bar to enter reader view for each page

Expected results:
- After tapping the third reader view icon, the page enters reader view and a prompt is displayed:
"Available offline. Bookmark Reader View items to read them offline. Add to Bookmarks"

Actual results:
Scenario 1. After tapping the third reader view icon, the page enters reader view. The prompt is not displayed. Tapping the back button, the page exits reader view and the prompt is displayed in normal view.

Scenario 2. Tapping the reader view icon, the prompt is displayed. But the page does not enter reader view, it remains in normal view, with the reader view icon in the URL Bar.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

ahunt: Do you have an idea why this is happening?

Comment 2

[Andrzej Hunt :ahunt]

Wow, I realy messed this up (this may have happened during refactoring while switching to showing the prompt the first time readerview is used, instead of after 3 uses?).

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1300532 - Post: Rename isEnteringReaderMode()'s parameters for more clarity

Review commit: https://reviewboard.mozilla.org/r/77090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77090/

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1300532 - Ensure old and new URL are ordered correctly to show prompt when entering ReaderView

Review commit: https://reviewboard.mozilla.org/r/77092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77092/

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8788683 [details]
Bug 1300532 - Post: Rename isEnteringReaderMode()'s parameters for more clarity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77090/diff/1-2/

Comment 6

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #2)
> Wow, I realy messed this up (this may have happened during refactoring while
> switching to showing the prompt the first time readerview is used, instead
> of after 3 uses?).

Nope, this landed in the original patch in  Bug 1247689 : (.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8788684 [details]
Bug 1300532 - Ensure old and new URL are ordered correctly to show prompt when entering ReaderView

https://reviewboard.mozilla.org/r/77092/#review75936

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8788683 [details]
Bug 1300532 - Post: Rename isEnteringReaderMode()'s parameters for more clarity

https://reviewboard.mozilla.org/r/77090/#review75938

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c2e946c182a
Ensure old and new URL are ordered correctly to show prompt when entering ReaderView r=sebastian
https://hg.mozilla.org/integration/autoland/rev/ad0d6c3a73e1
Post: Rename isEnteringReaderMode()'s parameters for more clarity r=sebastian

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8788684 [details]
Bug 1300532 - Ensure old and new URL are ordered correctly to show prompt when entering ReaderView

Approval Request Comment
[Feature/regressing bug #]: Bug 1247689
[User impact if declined]: The reader View bookmarking prompt is shown when exiting reader view instead of entering reader view - this is confusing: this prompt is meant to let users know that they can save reader view content offline, and it should therefore be shown while showing reader view content.
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: low: ordering of arguments supplied to the method that determines whether we are entering or exiting reader view has been swapped.
[String/UUID change made/needed]: none.

Note: this prompt is shown as part of a switchboard "experiment", this experiment is only enabled on nightly/aurora/beta, but not on release. It's not a huge problem if this doesn't make it into current beta (uplift is start of next week I believe), but uplifting would allow us to enable the experiment on 49's release if appropriate.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

I would rather keep this to 50 as we have a lot of last minute churn in 49.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8788684 [details]
Bug 1300532 - Ensure old and new URL are ordered correctly to show prompt when entering ReaderView

Too late for 49, but we could still potentially take this in 50.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c2e946c182a
https://hg.mozilla.org/mozilla-central/rev/ad0d6c3a73e1

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Teodora, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788684 [details]
Bug 1300532 - Ensure old and new URL are ordered correctly to show prompt when entering ReaderView

Fixes a recent regression, Aurora50+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0bfcfe38984f
https://hg.mozilla.org/releases/mozilla-aurora/rev/72689dcc8fe2

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: One A2001(Android 6.0)
Build: Firefox for Android 52.0a1 (2016-09-19)

1. Load an article from news.google.com
2. Tap the reader view icon from the URL Bar
=> the page enters reader view and a prompt is displayed: "Available offline. Bookmark Reader View items to read them offline. Add to Bookmarks"
3. Tap the "Add to Bookmarks" button.
=> the second prompt is displayed: "Read offline. Find your Reader View items in Bookmarks, even offline. Go to Bookmarks".
So, verified as fixed on latest Nightly.

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: One A2001 (Android 6.0)
Build: Firefox for Android 50 Beta 1

After tapping the reader view icon from the URL Bar, the page enters reader view and a prompt is displayed: "Available offline. Bookmark Reader View items to read them offline. Add to Bookmarks"
Tapping the "Add to Bookmarks" button, the second prompt is displayed: "Read offline. Find your Reader View items in Bookmarks, even offline. Go to Bookmarks".

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: One A2001 (Android 6.0)
Build: Firefox for Android 51.0a2 (2016-09-21)

After tapping the reader view icon from the URL Bar, the page enters reader view, but no prompt is displayed. If I open the menu and tap the bookmark icon, then "Read offline. Find your Reader View items in Bookmarks, even offline. Go to Bookmarks" prompt is displayed. 

But the prompt "Available offline. Bookmark Reader View items to read them offline. Add to Bookmarks" is never displayed.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

ahunt, can you look at that ----^

If there's something left to fix then let's move this to a new bug.

Comment 21

[Andrzej Hunt :ahunt]

It looks like we didn't enable the experiment on aurora (but it is enabled on nightly and beta), that'll need fixing on the switchboard side.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/experiments/records

Is this triple-readerview-bookmark-prompt? It should be enabled for "^org.mozilla.fennec|^org.mozilla.firefox_beta$" which includes Nightly (org.mozilla.fennec), Aurora (org.mozilla.fennec_aurora) and Beta (org.mozilla.firefox_beta).

Comment 23

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #22)
> https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/
> experiments/records
> 
> Is this triple-readerview-bookmark-prompt? It should be enabled for
> "^org.mozilla.fennec|^org.mozilla.firefox_beta$" which includes Nightly
> (org.mozilla.fennec), Aurora (org.mozilla.fennec_aurora) and Beta
> (org.mozilla.firefox_beta).

Yup. And yes, I'm not sure why that isn't working - in a self-built aurora, the following is returning false (app id is org.mozilla.fennec_ahunt, which in this case is equivalent to aurora)
SwitchBoard.isInExperiment(browserApp, Experiments.TRIPLE_READERVIEW_BOOKMARK_PROMPT)

Comment 24

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #23)
> (In reply to Sebastian Kaspari (:sebastian) from comment #22)
> > https://firefox.settings.services.mozilla.com/v1/buckets/fennec/collections/
> > experiments/records
> > 
> > Is this triple-readerview-bookmark-prompt? It should be enabled for
> > "^org.mozilla.fennec|^org.mozilla.firefox_beta$" which includes Nightly
> > (org.mozilla.fennec), Aurora (org.mozilla.fennec_aurora) and Beta
> > (org.mozilla.firefox_beta).
> 
> Yup. And yes, I'm not sure why that isn't working - in a self-built aurora,
> the following is returning false (app id is org.mozilla.fennec_ahunt, which
> in this case is equivalent to aurora)
> SwitchBoard.isInExperiment(browserApp,
> Experiments.TRIPLE_READERVIEW_BOOKMARK_PROMPT)

After some digging, it looks like our regex isn't actually matching, I've filed Bug 1312045 to address that.