"Add to reading list" button does not save it's state

VERIFIED FIXED in Firefox 27

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: u421692, Assigned: capella)

Tracking

({reproducible})

27 Branch
Firefox 27
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox24 affected, firefox25 affected, firefox26 affected, firefox27 verified, fennec+)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Envinronment:
Device: LG Nexus 4 (Android 4.2.2) / Samsung Galaxy Tab (Android 4.0.4)
Build: Firefox 25 Beta 4

Steps to reproduce:
1. Open cnn.com
2. Open an article
3. Tap on reader mode icon
4. Tap on "Add to reading list" button
5. Tap on back button
6. Tap on forward button
7. Tap on "Add to reading list" button

Expected result:
"Remove from reading list" button is present in the reader mode toolbar.

Actual result:
At step 6: "Add to reading list" button is present in the reader mode toolbar.
At step 7: A pop-up "Failed to add page to your reading list" is displayed, since the page is already in the reading list.
Reproducible on Nightly (10/02), on my S4. Probably a long-standing state issue.

Lucas, do you think this should track 25?
tracking-fennec: --- → ?
Flags: needinfo?(lucasr.at.mozilla)
Keywords: reproducible
(In reply to Aaron Train [:aaronmt] from comment #1)
> Reproducible on Nightly (10/02), on my S4. Probably a long-standing state
> issue.
> 
> Lucas, do you think this should track 25?

Hmm, not sure. The steps to reproduce are not a very common use case (the going back and forward part). I'm inclined to just fix this in 26. Capella, didn't you fix something similar before?
Flags: needinfo?(lucasr.at.mozilla) → needinfo?(markcapella)
I think this may be the same as what I reported https://bugzilla.mozilla.org/show_bug.cgi?id=863707
Flags: needinfo?(markcapella)
Posted patch bug923086 (obsolete) — Splinter Review
This fixes both test cases (this and bug 863707) in my local testing ...
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #813431 - Flags: review?(lucasr.at.mozilla)
Posted patch bug923086 (v1) (obsolete) — Splinter Review
Don't you hate it when you forget to qref  ;-)
Attachment #813431 - Attachment is obsolete: true
Attachment #813431 - Flags: review?(lucasr.at.mozilla)
Attachment #813433 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 813433 [details] [diff] [review]
bug923086 (v1)

Review of attachment 813433 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall but I think we need to ensure the reader mode UI doesn't feel glitchy on startup.

::: mobile/android/chrome/content/aboutReader.js
@@ +652,5 @@
>      this._toolbarEnabled = true;
>      this._setToolbarVisibility(true);
>  
>      this._requestFavicon();
> +    this._requestReadingListStatus();

If you trigger this request here, the toolbar will become visible and only then get its reading list toggle button state updated. This will probably look a bit glitchy/clunky. We should probably only show the toolbar once it has its full state.
Attachment #813433 - Flags: review?(lucasr.at.mozilla) → review-
I'm not sure I follow ...

Currently for the in/out readinglist icon (greyed/black) we basically do the same thing ... on program start we http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#125 which defaults the status to something: ("not" in readinglist) until the results of the "Reader:ListCountRequest" are processed and returned ... The message turnaround is quick enough that the user isnt exposed.

Are you thinking we might want to modify _setToolbarVisibility() behaviour to not make the banner visibible until all information has been determined sorta-thing? And would that be appropriate in this context or better as a seperate bug?
(In reply to Mark Capella [:capella] from comment #7)
> I'm not sure I follow ...
> 
> Currently for the in/out readinglist icon (greyed/black) we basically do the
> same thing ... on program start we
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutReader.js#125 which defaults the status to something: ("not" in
> readinglist) until the results of the "Reader:ListCountRequest" are
> processed and returned ... The message turnaround is quick enough that the
> user isnt exposed.
> 
> Are you thinking we might want to modify _setToolbarVisibility() behaviour
> to not make the banner visibible until all information has been determined
> sorta-thing?

Yep, exactly.

> And would that be appropriate in this context or better as a
> seperate bug?

I'd prefer this to be handled in this bug while you're at it :-)
Ouch ... since this issue exists currently (independent of this change), I'd favor we handle it in it's own bug ... (pre-reg or blocker?)

I'll look again .... maybe it's super trivial.
I'd like to do this as a part 1, "keep open" patch, then update my new patch to tie into the scheme ...

Also, I added a small null protection for an existing error I see during FF restart with "open all tabs from last time" where aboutreader.js generates Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 508}]
Attachment #814922 - Flags: review?(lucasr.at.mozilla)
Posted patch bug923086p2 Main Patch (obsolete) — Splinter Review
Then this piece adds the final / actual code for the readingListStatus button
Attachment #813433 - Attachment is obsolete: true
Attachment #815251 - Flags: review?(lucasr.at.mozilla)
tracking-fennec: ? → +
Duplicate of this bug: 863707
Comment on attachment 814922 [details] [diff] [review]
bug923086p1 Ensure readinglist banner not visible until state is finalized

Review of attachment 814922 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just need the question answered before giving r+

::: mobile/android/chrome/content/aboutReader.js
@@ +213,5 @@
>        case "Reader:ListCountReturn":
>        case "Reader:ListCountUpdated":  {
>          let count = parseInt(aData);
>          if (this._readingListCount != count) {
> +          let initialListButtonStateChanged = (this._readingListCount == -1);

nit: rename to isInitialStateChange?

@@ +218,2 @@
>            this._readingListCount = count;
>            this._updateListButton();

nit: add empty line here.

@@ +513,5 @@
>      });
>    },
>  
>    _loadFavicon: function Reader_loadFavicon(url, faviconUrl) {
> +    if (!this._article || this._article.url !== url)

What's this change about?
Attachment #814922 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 815251 [details] [diff] [review]
bug923086p2 Main Patch

Review of attachment 815251 [details] [diff] [review]:
-----------------------------------------------------------------

Looks nice, just needs nits fixed.

::: mobile/android/base/BrowserApp.java
@@ +349,5 @@
> +
> +                final JSONObject json = new JSONObject();
> +                try {
> +                    json.put("url", url);
> +                    json.put("inReadingList", Boolean.toString(inListStatus));

I'd prefer something simpler line a 0 or 1 int for inReadingList.

@@ +351,5 @@
> +                try {
> +                    json.put("url", url);
> +                    json.put("inReadingList", Boolean.toString(inListStatus));
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "JSON error - failing to return inReadingList status", e);

failing -> failed

::: mobile/android/chrome/content/aboutReader.js
@@ +221,5 @@
>            this._updateListButton();
>            // Display the toolbar when all its initial component states are known
>            if (initialListButtonStateChanged) {
>              this._setToolbarVisibility(true);
>            }

nit: add empty line here.

@@ +237,5 @@
> +          let inReadingList = (args.inReadingList == "true");
> +          if (this._isReadingListItem != inReadingList) {
> +            let initialReadingListStateChanged = (this._isReadingListItem == null);
> +            this._isReadingListItem = inReadingList;
> +            this._updateToggleButton();

nit: add empty space here.

@@ +491,5 @@
>      if (!this._toolbarEnabled)
>        return;
>  
>      // Don't allow visible toolbar until banner state is known
> +    if ((this._readingListCount == -1) || (this._isReadingListItem == null))

nit: no need for the extra inner parens here.
Attachment #815251 - Flags: review?(lucasr.at.mozilla) → review+
re: What's this change about?
>    },
>  
>    _loadFavicon: function Reader_loadFavicon(url, faviconUrl) {
> +    if (!this._article || this._article.url !== url)

Not directly related ... in comment 10 I pointed out a null error bug I noticed during testing was generating messages ... I shortcut fixed it instead of filing a whole new bug
Var renamed ... null check removed (will file different ug)
Attachment #814922 - Attachment is obsolete: true
Attachment #815251 - Attachment is obsolete: true
Attachment #816114 - Flags: review?(lucasr.at.mozilla)
|something simpler line a 0 or 1 int for inReadingList| changed it just enough for me to request followup review
Attachment #816178 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 816114 [details] [diff] [review]
bug923086p1 Ensure readinglist banner not visible until state is finalized

Review of attachment 816114 [details] [diff] [review]:
-----------------------------------------------------------------

Yep.
Attachment #816114 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 816178 [details] [diff] [review]
bug923086p2 The Actual patch

Review of attachment 816178 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #816178 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a21ab2d667de
https://hg.mozilla.org/mozilla-central/rev/bc531ef1dfa7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.