Closed Bug 837815 Opened 9 years ago Closed 8 years ago

Reading list button opens Top Sites, not Reading List

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox18 unaffected, firefox19- wontfix, firefox20+ verified, firefox21+ fixed, firefox22 verified, relnote-firefox -, fennec20+)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox18 --- unaffected
firefox19 - wontfix
firefox20 + verified
firefox21 + fixed
firefox22 --- verified
relnote-firefox --- -
fennec 20+ ---

People

(Reporter: deb, Assigned: capella)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 1 obsolete file)

Summary pretty much covers it -- in today's nightly (and yesterday's) the Reading List button in Readability Mode opens the Top Sites list, not the Reading List.
Looks like this regression happened some time ago and not recently.
tracking-fennec: --- → ?
Flags: in-moztrap?(fennec)
Hardware: Other → ARM
Attached patch Patch (v1) (obsolete) — Splinter Review
Not sure how this might have originally worked ...

Setting mAwesomeTabs.setCurrentTabByTag() in AwesomeBar.java doesn't appear to have an effect, where setting the mViewPager.setCurrentItem(1) seems to produce the correct response.

(How would setting current tab of TabHost have selected current item of ViewPager?)

fyi log:
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 083 bookmarksTab.getTag(): bookmarks
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 084 mAwesomeTabs.getCurrentTab(): -1
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 085 mAwesomeTabs.getCurrentTab(): -1
   D/XYZZY   (22595): AwesomeBarTabs.java - XYZZY() *2* - mViewPager.getCurrentItem(): 0
   D/XYZZY   (22595): AwesomeBarTabs.java - XYZZY() *2* - mViewPager.getCurrentItem(): 1
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 085 mAwesomeTabs.getCurrentTab(): -1
Attachment #710474 - Flags: feedback?(mark.finkle)
There is already a TC in Moztrap for this: https://moztrap.mozilla.org/manage/case/1267/ 

Should there be created one for negative behaviour? "Make sure the reader List button goes to Bookmarks -> Reading list not to Top Sites nor History"

I think the existing one is ok.
Flags: in-moztrap?(fennec) → in-moztrap+
Comment on attachment 710474 [details] [diff] [review]
Patch (v1)

Sending to Wes since he has more knowledge about the viewpager changes
Attachment #710474 - Flags: feedback?(mark.finkle) → feedback?(wjohnston)
Attached patch Patch (v2)Splinter Review
Ok, and the previous comment of mine wasn't very clear, I was wondering if this is really what we need ...
Assignee: nobody → markcapella
Attachment #710474 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #710474 - Flags: feedback?(wjohnston)
Attachment #711415 - Flags: review?(wjohnston)
(In reply to Mark Capella [:capella] from comment #5)
> Created attachment 711415 [details] [diff] [review]
> Patch (v2)
> 
> Ok, and the previous comment of mine wasn't very clear, I was wondering if
> this is really what we need ...

Drive-by: according to Android's source code, this is exactly what TabHost's setCurrentTabByTag() does. I would be surprised if overriding this method with with the exact same code would make any difference. Does the patch fix the bug for you?
yes
somethings not the same ... see comment 2 ... mAwesomeTabs.getCurrentTab(): returns -1 prior to and after the call to the un-overridden method
Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

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

I have no idea why this patch would work bug 750167 removes this filter("") call which might help somewhat... but really AwesomeBarTabs.java should just die. Its a useless abstraction for us at this point.
Attachment #711415 - Flags: review?(wjohnston) → review-
tracking-fennec: ? → 20+
Our final build of FF19 is tomorrow - we can't risk regression here. We'll release note instead.
Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

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

Oh wait. I didn't realize what was going on here. So we used to actually use this TabHost, and I think maybe I had to keep it around in order to have the TabWidget (TabWidget MUST be inside a TabHost or it throws). But you're right, it has no connection to the ViewPager really. Sorry for the confusion.
Attachment #711415 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/64df2203966e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Verified fixed on:
-build: Firefox for Android 21.0a1 (2013-02-17)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.1
Let's get this uplifted to FF 20 on Beta, if low risk.
Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

[Approval Request Comment] - This was lost somewhere along the way due to related code changes and overlooked.
Bug caused by (feature/regressing bug #): Not identified ... just corrected.
User impact if declined: Less friendly screen navigation.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None - low level UI change ... easy to backout / repair.
String or UUID changes made by this patch: None.
Attachment #711415 - Flags: approval-mozilla-beta?
Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

low risk well baked patch which helps with expected user behavior on screen navigation. Approving for uplift.
Attachment #711415 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
QA Contact: aaron.train
Works now, thanks Capella; verified (03/01) m-c.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 847475
Kevin got to our attention Bug 847475 will be affected, if we take the patch in this bug in Fx20 beta 3(going to build today).We are requesting a backout asap of this patch here to avoid the issue.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #721337 - Flags: approval-mozilla-beta?
Attachment #721337 - Flags: approval-mozilla-aurora?
Attachment #721337 - Flags: approval-mozilla-beta?
Attachment #721337 - Flags: approval-mozilla-beta+
Attachment #721337 - Flags: approval-mozilla-aurora?
Attachment #721337 - Flags: approval-mozilla-aurora+
Comment on attachment 721337 [details] [diff] [review]
Backout for bug847475 regression

marking this patch as obsolete, as sriram already helped with the backout !
Attachment #721337 - Flags: approval-mozilla-beta-
Attachment #721337 - Flags: approval-mozilla-beta+
Attachment #721337 - Flags: approval-mozilla-aurora-
Attachment #721337 - Flags: approval-mozilla-aurora+
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(In reply to Mark Capella [:capella] from comment #26)
> And finally backout from inbound
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a09c6489a9fd

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a09c6489a9fd
Any new attempts to fix this issue (that do not cause a regression like in bug 847475) will have to be tested & nominated before Monday March 10th so that we can put it in beta 4 and get more bake time/testing in the wild prior to release.
Thanks ... I've got a more direct patch I'm privately testing / baking currently ...
Attached patch Patch (v3)Splinter Review
The original patch contained correct code to open the reading list, but over-rode the classes setCurrentTabByTag() method which accidentally impacted AwesomeBarTabs.java's filter() method.

This solution isolates the new code to the specific condition where it's invoked by the user pressing the reader list item on the readermode screen.
Attachment #722348 - Flags: review?(wjohnston)
Blocks: 848267
Build above with the patch works for me - (LG Nexus 4, Android 4.2.2).
Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

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

::: mobile/android/base/AwesomeBarTabs.java
@@ +20,5 @@
>  import android.widget.TabHost;
>  import android.widget.TabWidget;
>  
>  public class AwesomeBarTabs extends TabHost
> +                            implements LightweightTheme.OnChangeListener {

There are some whitespace changes in here that aren't really realted.
Attachment #722348 - Flags: review?(wjohnston) → review+
Let's get the uplift nomination ready for this new version of the patch so when it's verified on m-c we can get it uplifted in time for the Beta going to build today.
Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Undetermined regression.
User impact if declined: Degraded user experience.
Testing completed (on m-c, etc.): Tested locally by developer, posted build tested independently, previous regression caused by this patch corrected / verified.
Risk to taking this patch (and alternatives if risky): Should be extremely minimal, critical backout amounts to one-line change.
String or UUID changes made by this patch: None.
Attachment #722348 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/87e22489d39d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Verified on Firefox for Android mozilla-central tinderbox build https://hg.mozilla.org/mozilla-central/rev/79b8e0a0bdb7
Attachment #722348 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/a2a4b7086c98

Does this need to go on Aurora too?
Target Milestone: Firefox 21 → Firefox 22
Duplicate of this bug: 814667
(In reply to Mark Capella [:capella] from comment #35)
> And on to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/87e22489d39d

Can you please nominate the patch for uplift on aurora as well ? Thanks
Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]: Should have been moved along with prior move to beta ... testing / verifications  look good.

Bug caused by (feature/regressing bug #): Undetermined regression.
User impact if declined: Degraded user experience.
Testing completed (on m-c, etc.): Tested locally by developer, posted build tested independently, previous regression caused by this patch corrected / verified. (See previous comments).

Risk to taking this patch (and alternatives if risky): Should be extremely minimal, critical backout amounts to one-line change.
String or UUID changes made by this patch: None.
Attachment #722348 - Flags: approval-mozilla-aurora?
Attachment #722348 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fix on Firefox Mobile 20 beta 5 build 1 on the Samsung Galaxy Tab 2 7.0 (Android 4.1.1)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.