Reader Mode: Reader mode should not be entered automatically from an Awesomescreen choice

VERIFIED FIXED in Firefox 16

Status

()

Firefox for Android
Reader View
P1
normal
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: heycam, Assigned: lucasr)

Tracking

Trunk
Firefox 18
All
Android
Points:
---

Firefox Tracking Flags

(firefox16+ verified, firefox17 verified, firefox18 verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
I was viewing a page that I often open and which is in my Awesomescreen's Top Sites.  I tried out Reader mode by tapping the icon in the location bar which added that page to my reading list.  I find that I don't need reader mode for this particular page, but when I re-choose that page from my Top Sites it defaults to showing the page in Reader mode, with no apparent way to go back to the normal site.  (I can edit the location bar to remove the "about:reader?..." stuff.  But that still doesn't help with the now updated Top Sites entry.)

Updated

6 years ago
Blocks: 696921
Summary: Reader mode should not be entered automatically from an Awesomescreen choice → Reader Mode: Reader mode should not be entered automatically from an Awesomescreen choice
(Assignee)

Comment 1

6 years ago
(In reply to Cameron McCormack (:heycam) from comment #0)
> I was viewing a page that I often open and which is in my Awesomescreen's
> Top Sites.  I tried out Reader mode by tapping the icon in the location bar
> which added that page to my reading list.  I find that I don't need reader
> mode for this particular page, but when I re-choose that page from my Top
> Sites it defaults to showing the page in Reader mode, with no apparent way
> to go back to the normal site.  (I can edit the location bar to remove the
> "about:reader?..." stuff.  But that still doesn't help with the now updated
> Top Sites entry.)

about:reader page views are not recorded in your browsing history. I'm a bit surprised that this is showing as one of your top site. Do you see a "Reading List" folder in the "Bookmarks" tab in the awesomescreen?
(Reporter)

Comment 2

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #1)
> about:reader page views are not recorded in your browsing history. I'm a bit
> surprised that this is showing as one of your top site. Do you see a
> "Reading List" folder in the "Bookmarks" tab in the awesomescreen?

I do, but the page that is in my awesomescreen is not listed there.  I should note that this page that I don't want to view in reading mode does not show the reader icon in the awesomescreen top sites entry, but that another page that *is* listed in Bookmarks > Reading List and is also in my awesomescreen top sites does.
(Assignee)

Updated

6 years ago
Component: General → Reader Mode
(Assignee)

Updated

6 years ago
tracking-firefox16: --- → ?
(Reporter)

Comment 3

6 years ago
With the new "toolbar" buttons that appear at the bottom of a page in Reader mode, I was able to turn off reading mode for the page that was stuck in that mode in my top sites.  Choosing it from my top sites now shows the regular page.
(Assignee)

Comment 4

6 years ago
Cameron, were you seeing two entries for the same url on top sites (one with the reader indicator and another without it) when the website was "stuck" in reader?
(Reporter)

Comment 5

6 years ago
No I only saw the one.  For this one that was stuck in reader mode, I never saw an entry in top sites that had the reader indicator in it.
(Assignee)

Comment 6

6 years ago
(In reply to Cameron McCormack (:heycam) (limited attention August 13 - 17) from comment #5)
> No I only saw the one.  For this one that was stuck in reader mode, I never
> saw an entry in top sites that had the reader indicator in it.

Do you see two items (one with reader icon and another without it) when you add an item to the reading list?
(Reporter)

Comment 7

6 years ago
(I might be confusing reader mode and reading list.  I guess they are separate features?)

With some page open, that is not in my Top Sites to begin with, if I tap the little book icon in the address bar, the page gets reshown with the more-readable styles.  Nothing changes in my Top Sites or the Reading List bookmarks folder.  If I tap on the bottom-left "add to reading list" button, an entry is added to the Reading List bookmarks folder.  It doesn't have a little book icon in it.  If I select that item, I get the page with the readable styles again.  So that seems OK.

If I instead start with a page that is already in my Top Sites, and tap the book icon in the address bar, I again get the page shown with the readable styles.  If I look in Top Sites, the entry for that page doesn't show the book icon and selecting it shows the regular styled page.  If while viewing the page with the readable styles I tap the "add to reading list" button, then in Top Sites on the awesomescreen that entry now does have the book icon next to it.  There is still just the one entry in Top Sites for that page.  The entry that is added to the Reading List bookmarks folder does not.  Selecting that Top Sites entry shows the page with the readable styles.  Tapping the "remove from reading list" button causes the book icon to be removed from the Top Sites entry.  BUT if I now select the entry from the Top Sites awesomescreen list again -- even without the book icon showing in it -- the page is shown still with readable styles, and I would expect it to be shown with its own style.

So my comment 3 seemed not to work with this entry.
tracking-firefox16: ? → +
Firefox Mobile trunk 20120821, Android 4.1.1 (stock), Google Nexus S

In my top sites:
http://www.heise.de/newsticker/classic/
http://m.heise.de/newsticker/classic/?from-classic=1
The first one redirects to the second one.

Tapping the first one in the Top Sites of the Awesomescreen opens the normal page, tapping the second one launches the page in the reader mode. Opening them in a new tab from the context menu opens the normal page.

I have no sites with reader mode or reader list icon and no bookmark folders at all.

Comment 9

6 years ago
Sending this your way Lucas, since you seem to be investigating.
Assignee: nobody → lucasr.at.mozilla
(Assignee)

Comment 10

6 years ago
(In reply to Archaeopteryx [:aryx] from comment #8)
> Firefox Mobile trunk 20120821, Android 4.1.1 (stock), Google Nexus S
> 
> In my top sites:
> http://www.heise.de/newsticker/classic/
> http://m.heise.de/newsticker/classic/?from-classic=1
> The first one redirects to the second one.
> 
> Tapping the first one in the Top Sites of the Awesomescreen opens the normal
> page, tapping the second one launches the page in the reader mode. Opening
> them in a new tab from the context menu opens the normal page.
> 
> I have no sites with reader mode or reader list icon and no bookmark folders
> at all.

Have you ever added any website to reading list before this happened? It would be helpful if someone had exactly steps to reproduce this issue.
I can't remember to have added a website to the reading list before. Opening the reading list from the icon in reader mode shows an empty list.
(Assignee)

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 12

6 years ago
Just realized that the fix for bug 757776 didn't land in Aurora which means that there was no visual indication that an item in awesomescreen is from the reading list or not.

I still need to confirm if certain websites only have a reading list item and no separate history item in awesomescreen.
(Assignee)

Comment 13

6 years ago
FYI: Found the bug causing this. When you remove an item from the reading list, the respective page will keep displaying the page in reader mode indefinitely.
(Reporter)

Comment 14

6 years ago
Great, looking forward to getting my local news site back in its original style. :)
(Assignee)

Comment 15

6 years ago
Created attachment 658900 [details] [diff] [review]
Never show duplicate URLs in awesomebar results

This patch ensures that we don't show any duplicate URLs on awesomebar search results. There's no performance regression AFAIK (I ran testBrowserProviderPerf a few times locally, no relevant changes).

One important bit about the display column: DISPLAY_READER has precedence over DISPLAY_NORMAL in case the a history entry has both a corresponding reading list item and a bookmark entry.

This also adds an index on Bookmarks(type and deleted) to avoid a full scan of the bookmarks table on the first query of the combined view (the one that fetches bookmarks with no history entries).

I updated the existing unit tests to cover the new behaviour and added a new unit test to verify that this specific bug is fixed.
Attachment #658900 - Flags: review?(margaret.leibovic)
Attachment #658900 - Flags: feedback?(rnewman)
(Assignee)

Comment 16

6 years ago
Created attachment 658901 [details] [diff] [review]
Always open original page in awesomebar search

We decided to always go to normal page when tapping on awesomebar results.
Attachment #658901 - Flags: review?(mark.finkle)
(Assignee)

Comment 17

6 years ago
Created attachment 658902 [details] [diff] [review]
Move getReaderMode() method to AwesomeBar
Attachment #658902 - Flags: review?(mark.finkle)
(Assignee)

Comment 18

6 years ago
Created attachment 658903 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items

When a reading list item is found in the awesomebar search, we show a reading list icon. This patch adds a "Open in Reader Mode" menu item in the context menu for these items.

Ian, need your feedback on the UI and wording.
Attachment #658903 - Flags: review?(mark.finkle)
Attachment #658903 - Flags: feedback?(ibarlow)
Attachment #658901 - Flags: review?(mark.finkle) → review+
Attachment #658902 - Flags: review?(mark.finkle) → review+
Comment on attachment 658903 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items

Looks OK to me, but Ian should think about using "Reader Mode" or just "Reader".
Attachment #658903 - Flags: review?(mark.finkle) → review+
Lucas, would you mind posting a build I can try?
We have one other string that uses "Reader Mode" and I thin that is used for accessibility:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#163
Comment on attachment 658900 [details] [diff] [review]
Never show duplicate URLs in awesomebar results

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

This approach seems fine, but you probably want to put some thought into making the results deterministic. If you're just grouping and aggregating the reader flag, AIUI you don't get control over, say, the title, visit counts…

::: mobile/android/base/db/BrowserProvider.java.in
@@ +213,3 @@
>          map.put(Combined.URL, Combined.URL);
>          map.put(Combined.TITLE, Combined.TITLE);
>          map.put(Combined.VISITS, Combined.VISITS);

Question: if we're running this through a GROUP BY, don't we also want to aggregate these columns in some way?
Attachment #658900 - Flags: feedback?(rnewman) → feedback+
(Assignee)

Comment 23

6 years ago
(In reply to Richard Newman [:rnewman] from comment #22)
> Comment on attachment 658900 [details] [diff] [review]
> Never show duplicate URLs in awesomebar results
> 
> Review of attachment 658900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This approach seems fine, but you probably want to put some thought into
> making the results deterministic. If you're just grouping and aggregating
> the reader flag, AIUI you don't get control over, say, the title, visit
> counts…

Visit counts won't matter much because it will be the same for any number of bookmarks for the same URL (we'll always have one URL in history for one or more bookmarks). So, the visits column is stable, unless I'm missing something.

As for the title, the thing is: I couldn't think of a meaningful way of controlling which one has precedence. We already make bookmark titles have precedence over history titles. But if we have 2 bookmarks for the same history URL, which bookmark we pick will be a bit arbitrary anyway.

Let me know if you have any ideas.

> ::: mobile/android/base/db/BrowserProvider.java.in
> @@ +213,3 @@
> >          map.put(Combined.URL, Combined.URL);
> >          map.put(Combined.TITLE, Combined.TITLE);
> >          map.put(Combined.VISITS, Combined.VISITS);
> 
> Question: if we're running this through a GROUP BY, don't we also want to
> aggregate these columns in some way?

What do you mean by aggregate here?
(Assignee)

Comment 24

6 years ago
(In reply to Ian Barlow (:ibarlow) from comment #20)
> Lucas, would you mind posting a build I can try?

Here's a test build with the patches applied:
https://dl.dropbox.com/u/1187037/open-in-reader.apk
(In reply to Mark Finkle (:mfinkle) from comment #19)
> Looks OK to me, but Ian should think about using "Reader Mode" or just
> "Reader".

I like that a lot! Lucas, let's tweak the context menu string to read "Open in Reader", as well as the string mentioned in comment 21, everything else looks great.
(In reply to Lucas Rocha (:lucasr) from comment #23)
> But if we have 2 bookmarks for the same
> history URL, which bookmark we pick will be a bit arbitrary anyway.
> 
> Let me know if you have any ideas.

Add Combined.TITLE as a low-priority ORDER BY clause? That'll ensure a consistent ordering. (Ordering by date modified would achieve the same result.)

Optionally also use the group_concat() aggregate function, which will return

  Title 1, Title 2


> What do you mean by aggregate here?

Just an inline restatement of my Overview question. Yay Splinter!

Comment 27

6 years ago
Comment on attachment 658900 [details] [diff] [review]
Never show duplicate URLs in awesomebar results

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

Sorry for the slow response, I've been traveling.

I agree with what rnewman we should think about how we're reconciling differing column data here, but I also agree with your response that it probably doesn't matter much which bookmark we're showing, since they will take the user to the same place. Also, I think that slight randomness is a worthwhile price to pay for avoiding looking buggy. We can probably dupe bug 741630 to this, now that we're actually making a fix.

It still bothers me that we need to recreate the combined view like this all the time, but I guess there's nothing else we can do :/ 

For the record, I ran a diff of createCombinedViewWithImagesOn10 and createCombinedViewWithImagesOn11, and this is what I got:
leibovic$ diff combined10.java combined11.java 
41,42c41,45
<                                      "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
<                                         Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
---
>                                      // Only use DISPLAY_READER if the matching bookmark entry inside reading
>                                      // list folder is not marked as deleted.
>                                      "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN CASE " +
>                                         qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " + Bookmarks.FIXED_READING_LIST_ID +
>                                         " THEN " + Combined.DISPLAY_READER + " ELSE " + Combined.DISPLAY_NORMAL + " END ELSE " +
52c55
<                                         qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + ")" +
---
>                                         qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + ") " +

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +1424,5 @@
>              mAsserter.is(c.moveToNext(), true, "Found third combined entry");
>              mAsserter.is(new Long(c.getLong(c.getColumnIndex(mCombinedIdCol))), new Long(0),
>                           "Combined _id column should always be 0");
> +            mAsserter.is(c.getLong(c.getColumnIndex(mCombinedBookmarkIdCol)) == combinedBookmarkId ||
> +                         c.getLong(c.getColumnIndex(mCombinedBookmarkIdCol)) == combinedBookmarkId2, true,

I think it would be worthwhile to add a comment about why this isn't deterministic.
Attachment #658900 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Updated

6 years ago
Duplicate of this bug: 741630
(Assignee)

Comment 29

6 years ago
(In reply to Margaret Leibovic [:margaret] from comment #27)
> Comment on attachment 658900 [details] [diff] [review]
> Never show duplicate URLs in awesomebar results
> 
> Review of attachment 658900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the slow response, I've been traveling.
> 
> I agree with what rnewman we should think about how we're reconciling
> differing column data here, but I also agree with your response that it
> probably doesn't matter much which bookmark we're showing, since they will
> take the user to the same place. Also, I think that slight randomness is a
> worthwhile price to pay for avoiding looking buggy. We can probably dupe bug
> 741630 to this, now that we're actually making a fix.

Also, reconciling different column data would probably cause the query to be more complex and slower—which is not worth it given that we don't really care that much which bookmark entry takes precedence for things like the title.
(Assignee)

Updated

6 years ago
Attachment #658903 - Flags: feedback?(ibarlow)
(Assignee)

Comment 30

6 years ago
Created attachment 659706 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items
Attachment #659706 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #658903 - Attachment is obsolete: true
(Assignee)

Comment 31

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #30)
> Created attachment 659706 [details] [diff] [review]
> Add "Open in Reader Mode" menu item to be shown on reading list items

Had to rename an existing string. Just requesting a review to make sure I didn't miss anything.
Comment on attachment 659706 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items

Looks good. You modified the existing string entity for "Reader" so l10n is in the loop. (good thing)
Attachment #659706 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 35

6 years ago
Comment on attachment 658900 [details] [diff] [review]
Never show duplicate URLs in awesomebar results

[Approval Request Comment]
User impact if declined: Serious bug. Pages added to reading list then removed might be permanently open in reader mode when accessed from the awesomescreen. Furthermore, we show duplicate entries for the same urls if the page is both a bookmark and is in the reading list.
Testing completed (on m-c, etc.): Unit tests cover the changes made here. Landed in m-c, no problems.
Risk to taking this patch (and alternatives if risky): Very low as this code is well covered by unit tests.
String or UUID changes made by this patch: None.
Attachment #658900 - Flags: approval-mozilla-beta?
Attachment #658900 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

6 years ago
Comment on attachment 658901 [details] [diff] [review]
Always open original page in awesomebar search

[Approval Request Comment]
User impact if declined: Without this patch, pages in awesomescreen will behave inconsistently. They will automatically open in Reader Mode if they are in the reading list which might cause confusion. This patch ensures that awesomescreen results always open the original page.
Testing completed (on m-c, etc.): Landed in m-c, no issues.
Risk to taking this patch (and alternatives if risky): Very low, small patch. Simply ensures that we always open the original page from awesomescreen results.
String or UUID changes made by this patch: None.
Attachment #658901 - Flags: approval-mozilla-beta?
Attachment #658901 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 37

6 years ago
Comment on attachment 658902 [details] [diff] [review]
Move getReaderMode() method to AwesomeBar

[Approval Request Comment]
User impact if declined: Small refactoring to avoid code duplication.
Testing completed (on m-c, etc.): Landed on m-c, no issues.
Risk to taking this patch (and alternatives if risky): Very low, just a small refactoring.
String or UUID changes made by this patch: None.
Attachment #658902 - Flags: approval-mozilla-beta?
Attachment #658902 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 38

6 years ago
Comment on attachment 659706 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items

[Approval Request Comment]
User impact if declined: After the other patches land, there will be no way to open reading list items in Reader Mode when they appear in awesomescreen results, which is an unfortunate usability issue. This patch adds one new string and changes one existing string which I guess is a definite no-go on Beta but I'd like to try to get this in Aurora at least as it's a worthy usability improvement that should ideally reach users asap.
Testing completed (on m-c, etc.): Landed on m-c, no issues.
Risk to taking this patch (and alternatives if risky): Low, simply adds a new context menu option for reading list items in awesomescreen results.
String or UUID changes made by this patch: Changes one existing string (&reader_mode -> &reader) and adds one string (&open_in_reader).
Attachment #659706 - Flags: approval-mozilla-aurora?
Articles saved in Reading List are not open anymore straight into Reader Mode from Top Sites. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-11)
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
status-firefox18: --- → verified

Updated

6 years ago
Attachment #658900 - Flags: approval-mozilla-beta?
Attachment #658900 - Flags: approval-mozilla-beta+
Attachment #658900 - Flags: approval-mozilla-aurora?
Attachment #658900 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Attachment #658901 - Flags: approval-mozilla-beta?
Attachment #658901 - Flags: approval-mozilla-beta+
Attachment #658901 - Flags: approval-mozilla-aurora?
Attachment #658901 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Attachment #658902 - Flags: approval-mozilla-beta?
Attachment #658902 - Flags: approval-mozilla-beta+
Attachment #658902 - Flags: approval-mozilla-aurora?
Attachment #658902 - Flags: approval-mozilla-aurora+
(In reply to Lucas Rocha (:lucasr) from comment #33)
> Pushed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4be8144c6149
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4425347713b7
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6c25268999b5
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d72e44652418

Just so we know: These pushes caused a minor performance regression in the tprovider Talos test. I haven't seen an alert for the change on m-i, but here's the one for the m-c merge:

Message: 2
Date: Tue, 11 Sep 2012 19:40:44 -0000
From: nobody@cruncher.build.mozilla.org
To: dev-tree-management@lists.mozilla.org
Subject: Talos Regression Robocop Database Benchmark increase 3.15%
        on        Android 2.2 (Native) mobile
Message-ID:
        <20120911194044.232CC1042C0@cruncher.srv.releng.scl3.mozilla.com>
Content-Type: text/plain; charset="us-ascii"

Regression Robocop Database Benchmark increase 3.15% on Android 2.2 (Native) mobile
--------------------------------------------------------------------------------------
    Previous: avg 362.467 stddev 3.043 of 30 runs up to revision 7585c9b91877
    New     : avg 373.883 stddev 0.857 of 5 runs since revision 96287ad60bef
    Change  : +11.417 (3.15% / z=3.752)
    Graph   : http://mzl.la/NmOQBh

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7585c9b91877&tochange=96287ad60bef
(Assignee)

Comment 41

6 years ago
(In reply to Geoff Brown [:gbrown] from comment #40)
> (In reply to Lucas Rocha (:lucasr) from comment #33)
> > Pushed:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/4be8144c6149
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/4425347713b7
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/6c25268999b5
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/d72e44652418
> 
> Just so we know: These pushes caused a minor performance regression in the
> tprovider Talos test. I haven't seen an alert for the change on m-i, but
> here's the one for the m-c merge:

This is to be expected given that the query now does grouping on the results which causes a slight overhead.
(Assignee)

Comment 44

6 years ago
Backed out all patches:
https://hg.mozilla.org/releases/mozilla-beta/rev/4cbe38db36da

Will fix the empty patch and push again.
Comment on attachment 659706 [details] [diff] [review]
Add "Open in Reader Mode" menu item to be shown on reading list items

[Triage Comment]
Our understanding is that the user can still reach reader mode by going to the reading list item and then tapping reader mode. This is a pretty minor usability issue with l10n impact, so we should reconsider only if we get FF16 feedback about this.
Attachment #659706 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
status-firefox16: --- → fixed
status-firefox17: --- → fixed
(Reporter)

Comment 47

6 years ago
Unfortunately I lost my Gingerbread Android installation while trying to flash B2G (oops) so I can't verify the fix.  But thanks for working on it!
I am unable to open websites in reader mode from the Top Sites tab anymore. Marking the issue as verified on Firefox Mobile 16 and 17.

Verified on:
Aurora 17.0a2 2012-09-26 / Firefox Mobile 16.0b5 build 1
Samsung Galaxy R ( Android 2.3.4)
status-firefox16: fixed → verified
status-firefox17: fixed → verified
You need to log in before you can comment on or make changes to this bug.