Closed
Bug 1320900
Opened 9 years ago
Closed 8 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)
Tracking
(fennec+, firefox53 verified)
VERIFIED
FIXED
Firefox 53
People
(Reporter: kaarticsivaraam91196+bugzilla, Assigned: cnevinchen)
References
Details
Attachments
(2 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•9 years ago
|
||
I guess this is also the case when a page falsly triggers the reader view icon.
Reporter | ||
Updated•9 years ago
|
OS: Unspecified → Android
Hardware: Unspecified → All
Reporter | ||
Comment 2•9 years ago
|
||
Forgive for that numbering in the steps. Please, avoid it.
Updated•9 years ago
|
Flags: needinfo?(alam)
Comment 3•9 years ago
|
||
Not sure I follow here...
Kbrosnan, any luck repro-ing the issue here?
Flags: needinfo?(alam) → needinfo?(kbrosnan)
Comment 5•9 years ago
|
||
(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)
Comment 6•9 years ago
|
||
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)
Comment 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → cnevinchen
Reporter | ||
Comment 8•8 years ago
|
||
(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: ? → +
Comment 9•8 years ago
|
||
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 hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
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)
Reporter | ||
Comment 12•8 years ago
|
||
(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)
Comment 13•8 years ago
|
||
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 hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Priority: -- → P2
Comment 17•8 years ago
|
||
mozreview-review |
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+
Comment 18•8 years ago
|
||
There's still one open issue in the first commit - was it an accident to create two?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8824821 -
Attachment is obsolete: true
Comment 20•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
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•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 26•8 years ago
|
||
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
Comment 27•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 28•8 years ago
|
||
Verified as fixed in build 53.0a2 (2017-01-25);
Device: Huawei MediaPad M2 (Android 5.1.1).
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•