1320900 - Bookmarking in reader view should not display "Saved Offline" when unable to fetch page contents

Status: VERIFIED FIXED

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

Description

[Kaartic Sivaraam]

Attachment: Saved Offline message when page load fails

Currently bookmarking pages in reader view saves them for offline reading. Sometimes when the contents fails to load in reader mode and the user tries to bookmark that page the "Saved offline" message is displayed even though the content is not available. This seems misleading as there is no content that is saved.

Steps to reproduce:
1. Save a page for offline reading
2. Enable flight mode
3. Clear cache for firefox (just to ensure the page doesn't load from cache)
3. Exit the reader view for that page (This should display the "Server not found" or similar message)
4. Remove the bookmark 
4. Click the reader view icon (yes, it's there even though the page displays a "Server not found" message !)
5. Add a bookmark for the page

Expected results:
"Saved offline" message is not displayed.

Actual results:
"Saved offline" message is displayed.

Comment 1

[Kaartic Sivaraam]

I guess this is also the case when a page falsly triggers the reader view icon.

Comment 2

[Kaartic Sivaraam]

Forgive for that numbering in the steps. Please, avoid it.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Not sure I follow here...

Kbrosnan, any luck repro-ing the issue here?

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Yes, those steps result in the described error.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Kaartic Sivaraam from comment #0)
> Created attachment 8815224 [details]
> Saved Offline message when page load fails
> 
> Currently bookmarking pages in reader view saves them for offline reading.
> Sometimes when the contents fails to load in reader mode and the user tries
> to bookmark that page the "Saved offline" message is displayed even though
> the content is not available. This seems misleading as there is no content
> that is saved.

Yep. This is a bug.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Sorry, I should have written more when flagging you. Yeah this is a bug, but what do you think we should do here (UX)? Show an error snackbar instead? Or none at all?

> Currently bookmarking pages in reader view saves them for offline reading.
> Sometimes when the contents fails to load in reader mode and the user tries
> to bookmark that page the "Saved offline" message is displayed even though
> the content is not available. This seems misleading as there is no content
> that is saved.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

There shouldn't be the "Saved offline" message here.

If nothing is even Bookmarked, then the star shouldn't be filled either (or press-able) for that matter. Here, we should be disabling the Bookmark button.

Comment 8

[Kaartic Sivaraam]

(In reply to Anthony Lam (:antlam) from comment #7)
> If nothing is even Bookmarked, then the star shouldn't be filled either (or
> press-able) for that matter. Here, we should be disabling the Bookmark
> button.

Did you would have noticed this line in the above description,

> 4. Click the reader view icon (yes, it's there even though the page displays
> a "Server not found" message !)

I guess the reader icon should not be shown at all when the page displays "Server not found". I hope this would cover the case noted in the description. 

(In reply to Kaartic Sivaraam from comment #1)
> I guess this is also the case when a page falsly triggers the reader view
> icon.

I guess that in some cases a few pages do falsly trigger the "Reader view" icon to be shown (not so sure). In that case the user would see a page similar to the one in the attached screenshot. If he does he shouldn't be shown the "Saved offline" message.

These are just my thoughts. Are they correct ?

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sorry Kaartic, I don't think I completely follow you. 

But IIUC, what you're saying here (below) makes sense. We should not be showing the Reader View icon when it's "Server not found".

(In reply to Kaartic Sivaraam from comment #8)
> I guess the reader icon should not be shown at all when the page displays
> "Server not found".

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1320900 - Do not show Reader View icon on error pages.

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

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review101004

::: mobile/android/chrome/content/content.js:103
(Diff revision 1)
> -    if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader ||
> +    if (!ReaderMode.isEnabledForParseOnLoad  ||
>          !(content.document instanceof content.HTMLDocument) ||
>          content.document.mozSyntheticDocument) {
>        return;
>      }

I think this check here exists so that we do not run the "is this a page we can improve with reader mode?" code. Now with that change we would run this for reader mode pages - this seems odd and unnecessary?

Comment 12

[Kaartic Sivaraam]

(In reply to Anthony Lam (:antlam) from comment #9)
> Sorry Kaartic, I don't think I completely follow you. 

I just wanted thought that it would be better if the following were done to avoid this bug.

1. Check if a page has an article and if it doesn't, don't show the "Reader view icon" (this is what you thought made sense. I guess the previous patch seems to cover this part)

2. In case a few pages "falsly" trigger reader view (i.e.) the reader view icon is displayed even though it is not possible to fetch contents from the page.
    i) Clicking on the "reader view" icon would display "Failed to load article from page" message (as shown in the attached screen sot)
   ii) Bookmarking with the above stated reader view page would show the "Saved Offline" message.
To avoid this, I think it would be good to check if the article load was successful when the user tries to bookmark a page in reader view and show the "Saved Offline" message only if it was. Alternatively what you have stated above would be a good choice too.

In reply to Anthony Lam (:antlam) from comment #7)
> There shouldn't be the "Saved offline" message here.
> 
> If nothing is even Bookmarked, then the star shouldn't be filled either (or
> press-able) for that matter. Here, we should be disabling the Bookmark
> button.

Am I correct with my thoughts ?

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think there might be several overlapping issues here. To keep things in scope, let's just focus on this:

Do not show Reader View icon on error pages.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review102476

::: mobile/android/chrome/content/content.js:60
(Diff revision 2)
>    get isAboutReader() {
>      return content.document.documentURI.startsWith("about:reader");
>    },
>  
> +  get isErrorPage() {
> +    return content.document.documentURI.startsWith("about:neterror");

Are there other error pages where we might need to do this? One I found via DXR is about:certerror for example.

::: mobile/android/chrome/content/content.js:109
(Diff revision 2)
>      }
>    },
>    updateReaderButton: function(forceNonArticle) {
> -    if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader ||
> +    // Do not show Reader View icon on error pages (bug 1320900)
> +    if (this.isErrorPage){
> +        this.scheduleReadabilityCheckPostPaint(true);

I'm not too familiar with this code - but shouldn't it be enough to just do nothing here in case of an error page?

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1320900 - Do not show Reader View icon on error pages.

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

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824821 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/103092/#review104250

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

There's still one open issue in the first commit - was it an accident to create two?

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review104596

::: mobile/android/chrome/content/content.js:111
(Diff revision 3)
>      }
>    },
>    updateReaderButton: function(forceNonArticle) {
> -    if (!ReaderMode.isEnabledForParseOnLoad || this.isAboutReader ||
> +    // Do not show Reader View icon on error pages (bug 1320900)
> +    if (this.isErrorPage) {
> +        this.scheduleReadabilityCheckPostPaint(true);

Sorry, looking at the code I still don't fully understand this:

Doesn't this still run the reader mode check in onPaintWhenWaitedFor() and show the reader mode button (forceNonArticle is only used when the article is not "readerable").

Do you have a test page for this?

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review104596

> Sorry, looking at the code I still don't fully understand this:
> 
> Doesn't this still run the reader mode check in onPaintWhenWaitedFor() and show the reader mode button (forceNonArticle is only used when the article is not "readerable").
> 
> Do you have a test page for this?

Yes. This will still run the reader mode check. But since the browser is displaying a error page (e.g. neterror), the reader mode button won't appear.

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review102476

> I'm not too familiar with this code - but shouldn't it be enough to just do nothing here in case of an error page?

We need this line to refresh the reader mode button so it'll disapear after find there's an error page.

Comment 23

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100456/diff/3-4/

Comment 24

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

Updated the patch. If it's error page, hide the reader mode button directly.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/100456/#review106676

LGTM

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b67585d1f518
Do not show Reader View icon on error pages. r=sebastian

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Sorina Florean [:sflorean]]

Verified as fixed in build 53.0a2 (2017-01-25);
Device: Huawei MediaPad M2 (Android 5.1.1).