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

Status: VERIFIED FIXED

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

Description

[Cameron McCormack (:heycam)]

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.)

Comment 1

[Lucas Rocha (:lucasr)]

(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?

Comment 2

[Cameron McCormack (:heycam)]

(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.

Comment 3

[Cameron McCormack (:heycam)]

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.

Comment 4

[Lucas Rocha (:lucasr)]

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?

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Lucas Rocha (:lucasr)]

(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?

Comment 7

[Cameron McCormack (:heycam)]

(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.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

[Alex Keybl [:akeybl]]

Sending this your way Lucas, since you seem to be investigating.

Comment 10

[Lucas Rocha (:lucasr)]

(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.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 12

[Lucas Rocha (:lucasr)]

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.

Comment 13

[Lucas Rocha (:lucasr)]

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.

Comment 14

[Cameron McCormack (:heycam)]

Great, looking forward to getting my local news site back in its original style. :)

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: 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.

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Always open original page in awesomebar search

We decided to always go to normal page when tapping on awesomebar results.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Move getReaderMode() method to AwesomeBar

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: 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.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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".

Comment 20

[Ian Barlow (:ibarlow)]

Lucas, would you mind posting a build I can try?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

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 22

[Richard Newman [:rnewman]]

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?

Comment 23

[Lucas Rocha (:lucasr)]

(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?

Comment 24

[Lucas Rocha (:lucasr)]

(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

Comment 25

[Ian Barlow (:ibarlow)]

(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.

Comment 26

[Richard Newman [:rnewman]]

(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

[:Margaret Leibovic]

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.

Comment 29

[Lucas Rocha (:lucasr)]

(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.

Comment 30

[Lucas Rocha (:lucasr)]

Attachment: Add "Open in Reader Mode" menu item to be shown on reading list items

Comment 31

[Lucas Rocha (:lucasr)]

(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 32

[Mark Finkle (:mfinkle) (use needinfo?)]

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)

Comment 33

[Lucas Rocha (:lucasr)]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4be8144c6149
https://hg.mozilla.org/mozilla-central/rev/4425347713b7
https://hg.mozilla.org/mozilla-central/rev/6c25268999b5
https://hg.mozilla.org/mozilla-central/rev/d72e44652418

Comment 35

[Lucas Rocha (:lucasr)]

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.

Comment 36

[Lucas Rocha (:lucasr)]

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.

Comment 37

[Lucas Rocha (:lucasr)]

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.

Comment 38

[Lucas Rocha (:lucasr)]

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).

Comment 39

[Cristian Nicolae (:xti)]

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

Comment 40

[Geoff Brown [:gbrown] (pto Apr 11-18)]

(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: [e-mail]
To: [e-mail]
Subject: Talos Regression Robocop Database Benchmark increase 3.15%
        on        Android 2.2 (Native) mobile
Message-ID:
        <[e-mail]>
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

Comment 41

[Lucas Rocha (:lucasr)]

(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.

Comment 42

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d685e216d684
https://hg.mozilla.org/releases/mozilla-aurora/rev/3db4e076dd4a
https://hg.mozilla.org/releases/mozilla-aurora/rev/6269d2eae30c

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/6692541f6fc2
https://hg.mozilla.org/releases/mozilla-beta/rev/09f3c1a34043
https://hg.mozilla.org/releases/mozilla-beta/rev/6add9eca358f

Comment 43

[Ed Morley [:emorley]]

(In reply to Lucas Rocha (:lucasr) from comment #42)
> Pushed to beta:
> https://hg.mozilla.org/releases/mozilla-beta/rev/6692541f6fc2
> https://hg.mozilla.org/releases/mozilla-beta/rev/09f3c1a34043
> https://hg.mozilla.org/releases/mozilla-beta/rev/6add9eca358f

This is burning beta.

Presume because this commit:
https://hg.mozilla.org/releases/mozilla-beta/rev/09f3c1a34043
...is empty :-)

Comment 44

[Lucas Rocha (:lucasr)]

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

Will fix the empty patch and push again.

Comment 45

[Lucas Rocha (:lucasr)]

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/8bd0ba02c7c2
https://hg.mozilla.org/releases/mozilla-beta/rev/51c0d35d7449
https://hg.mozilla.org/releases/mozilla-beta/rev/65843725023

Comment 46

[Alex Keybl [:akeybl]]

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.

Comment 47

[Cameron McCormack (:heycam)]

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!

Comment 48

[Adrian Tamas (:AdrianT)]

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)