Closed Bug 1267467 Opened 4 years ago Closed 4 years ago

Opening a reading list/reader mode icon should bump its history

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: liuche, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Opening a reading list item or reader mode item does not update its position in history.

STR:
1. Have an item in the reading list.
2. Open a bunch of other websites.
3. Open reading list item in a tab.
4. Go to History panel.

Expected:
Item is at the top of history because it was just opened.

Actual:
Item is not at the top of history because it was opened from cache and no "page" was loaded.

We should update the history count and lastVisited.
Summary: Fennec Home should not have a "switch to tab" → Opening a reading list/reader mode icon should bump its history
Flags: needinfo?(alam)
Talking to Margaret about this too, this is probably something for 49. 

But given the direction we want to take "History" and how it's for "recalling", we should make sure it also shows up in there.
Flags: needinfo?(alam)
Blocks: migrate-RL
No longer blocks: home-panels
I have a suspicion about: pages are filtered out on platform side, specifically if we treat about: pages as typeChrome (as opposed to typeContent) then we'd never pass the URI to our history manager at:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#12809

(I haven't actually debugged this yet, so I can't be certain that this is what's happening.)


Similarly going directly to an about:reader url doesn't result in a history entry on desktop. E.g. try to open the following URL -> no history entry
about:reader?url=https://support.mozilla.org/en-US/kb/save-web-pages-your-reading-list-firefox-android
Duplicate of this bug: 1269789
This patch lets us save about:reader URLs. However we probably also want to strip the about:reader part of the URL (perhaps on the Java side) so that we don't have separate about:reader and plain versions of the site in history?
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/51753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51753/
Attachment #8749815 - Attachment description: MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history → MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen
Attachment #8751015 - Flags: review?(margaret.leibovic)
Attachment #8749815 - Flags: review?(nchen)
Comment on attachment 8749815 [details]
MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51169/diff/1-2/
Attachment #8749815 - Flags: review?(nchen) → review+
Comment on attachment 8749815 [details]
MozReview Request: Bug 1267467 - Don't filter out reader view pages when storing history r?jchen

https://reviewboard.mozilla.org/r/51169/#review48611
Comment on attachment 8751015 [details]
MozReview Request: Bug 1267467 - Strip about:reader when storing URIs in history r?margaret

https://reviewboard.mozilla.org/r/51753/#review49111

I'm worried there may be some nuance I'm missing here, but this seems fine to me. I don't like that we're continuing to spread the pattern of special-casing these about:reader URLs, but I suppose that's just the nature of our implementation decision to make reader view into a special page.
Attachment #8751015 - Flags: review?(margaret.leibovic) → review+
Gijs, this bug also applies to desktop Firefox. You may want to consider filing a desktop bug as well.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Margaret Leibovic from comment #10)
> Gijs, this bug also applies to desktop Firefox. You may want to consider
> filing a desktop bug as well.

Thanks, filed bug 1272373.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Margaret Leibovic from comment #9)
> I'm worried there may be some nuance I'm missing here, but this seems fine
> to me. I don't like that we're continuing to spread the pattern of
> special-casing these about:reader URLs, but I suppose that's just the nature
> of our implementation decision to make reader view into a special page.

I feel like filtering out about:reader at this stage is the least intrusive thing we can do: we currently assume all pages in bookmarks/other homepanels are the raw URL (without about:reader), and we add the offline icon for any bookmark/history-item/etc that is stored offline. If we store the about:reader URL in history, then we'd probably need to strip that when displaying the history item. There are probably a bunch of other special cases (e.g. sharing), so this is probably the least painful approach.

The advantage of this approach is that we show one page in history when you visit a page, and then enter reader mode (otherwise we'd have one normal version, one reader version). The disadvantage is that (unless the page is bookmarked from RV), the history item will open the non-readerview version of the page.
I'll probably wait a bit with landing this since there's some interesting discussion happening in Bug 1272373 (the equivalent desktop bug), and whatever the solution there is might help make the android side better too. (I don't really have much knowledge of the docshell / history / etc, so my solution is unlikely to be optimal.)
https://hg.mozilla.org/integration/fx-team/rev/b1f926af38405c963cb7cc6db9e35ec099c7619a
Bug 1267467 - Don't filter out reader view pages when storing history r=jchen

https://hg.mozilla.org/integration/fx-team/rev/2dc0440c0d6a3741a9d7791f9c0a180f78a91747
Bug 1267467 - Strip about:reader when storing URIs in history r=margaret
https://hg.mozilla.org/mozilla-central/rev/b1f926af3840
https://hg.mozilla.org/mozilla-central/rev/2dc0440c0d6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
After opening some websites and after that an item from reading list in a new tab, The reading list item is at the top of history.

Verified as fixed using:
Device: Galaxy note 5 (Android 5.1.1)
Build: Firefox for Android 49.0a1 (2016-05-26)
You need to log in before you can comment on or make changes to this bug.