Closed Bug 1320900 Opened 4 years ago Closed 4 years ago

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

Categories

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

Product:
Firefox for Android Graveyard
Component:
Reader View
Version:
52 Branch
Platform:
All
Android
Type:
defect

Tracking

(fennec+, firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
fennec + ---
firefox53 --- verified

People

(Reporter: kaarticsivaraam91196+bugzilla, Assigned: cnevinchen)

References

Details

Attachments

(2 files, 1 obsolete file)

Saved Offline message when page load fails
23.38 KB, image/png
Details
Bug 1320900 - Do not show Reader View icon on error pages.
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
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.
I guess this is also the case when a page falsly triggers the reader view icon.
OS: Unspecified → Android
Hardware: Unspecified → All
Forgive for that numbering in the steps. Please, avoid it.
Flags: needinfo?(alam)
Not sure I follow here...

Kbrosnan, any luck repro-ing the issue here?
Flags: needinfo?(alam) → needinfo?(kbrosnan)
Yes, those steps result in the described error.
Flags: needinfo?(kbrosnan)
(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.
Flags: needinfo?(s.kaspari)
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.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari) → needinfo?(alam)
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.
Flags: needinfo?(alam)
Assignee: nobody → cnevinchen
(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 ?
Flags: needinfo?(alam)
tracking-fennec: ? → +
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".
Flags: needinfo?(alam)
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?
Attachment #8821079 - Flags: review?(s.kaspari)
(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 ?
Flags: needinfo?(alam)
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.
Flags: needinfo?(alam)
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?
Attachment #8821079 - Flags: review?(s.kaspari)
Priority: -- → P2
Comment on attachment 8824821 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

https://reviewboard.mozilla.org/r/103092/#review104250
Attachment #8824821 - Flags: review?(s.kaspari) → review+
There's still one open issue in the first commit - was it an accident to create two?
Attachment #8824821 - Attachment is obsolete: true
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?
Attachment #8821079 - Flags: review?(s.kaspari)
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 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 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 on attachment 8821079 [details]
Bug 1320900 - Do not show Reader View icon on error pages.

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

LGTM
Attachment #8821079 - Flags: review?(s.kaspari) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b67585d1f518
Do not show Reader View icon on error pages. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b67585d1f518
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Verified as fixed in build 53.0a2 (2017-01-25);
Device: Huawei MediaPad M2 (Android 5.1.1).
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Duplicate of this bug: 782545
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.