919516 - [Tablet] The previous bookmark in the list is opened when opening bookmarks from the Bookmarks list

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 27.0a1 2013-09-23
Asus Transformer TF101

Steps to reproduce:
1) Open the Bookmark tab
2) Tap on the first bookmark
3) Tap on the 2nd bookmark

Expected results:
The correct bookmark is opened every time

Actual results:
At step 2 nothing happens
At step 3 the 1st bookmark is opened
When opening a bookmark the previous bookmark in the list is opened

Comment 1

[Adrian Tamas (:AdrianT)]

Long tap seems to work correctly

Comment 2

[Sriram Ramasubramanian [:sriram]]

This has to do with the header views. But already have checks for those. :(

Comment 3

[Lucas Rocha (:lucasr)]

Sriram, could you have a look at this bug?

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: No need to account for header views in Bookmarks page

Took this bug as it's kinda or urgent (it blocks 2 other tracking bugs for Fx26).

It seems the 'position' argument for onItemClick() already accounts for the header views for us. Reading list has the same top divider and doesn't account for header view in its onItemClick handler either.

To be honest, this is a bit surprising given that we do have to account for header views in the TopSitesPage's onItemClick handler.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 813044 [details] [diff] [review]
No need to account for header views in Bookmarks page

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

This is strange. If it works, I'm fine with this patch.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Comment on attachment 813044 [details] [diff] [review]
> No need to account for header views in Bookmarks page
> 
> Review of attachment 813044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is strange. If it works, I'm fine with this patch.

Yeah, it works fine.

Comment 8

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

Comment on attachment 813044 [details] [diff] [review]
No need to account for header views in Bookmarks page

Any ideas for how to test for this kind of issue?

Comment 9

[Adrian Tamas (:AdrianT)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 813044 [details] [diff] [review]
> No need to account for header views in Bookmarks page
> 
> Any ideas for how to test for this kind of issue?

This is covered in testBookmark and testBookmarksTab Robocop tests. Unfortunately I didn't manage to re-write and re-enable the tests yet with all the changes to about:home. I'll test the patch for testBookmark tomorrow and will start the re-write of the patch in testBookmarksTab to account for the changes done in the new-new-about-home test

Comment 10

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/ad37f9c6cc95

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ad37f9c6cc95

Comment 12

[Aaron Train [:aaronmt]]

Lucas, this is needed on Aurora

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 813044 [details] [diff] [review]
No need to account for header views in Bookmarks page

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: Wrong URL is loaded when tapping on bookmarks (on tablets)
Testing completed (on m-c, etc.): In m-c for a week or so, no issues. 
Risk to taking this patch (and alternatives if risky): Low risk, just removing some unnecessary code that was causing problem.
String or IDL/UUID changes made by this patch: n/a

Comment 14

[Chenxia Liu [:liuche]]

This is the cause of the permaorange rc1 on Android 4.0 on Aurora, so that should clear up once this gets approval and gets landed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a088f42b312b

Comment 16

[u421692]

Verified as fixed on Firefox 26 Beta 6, on Samsung Galaxy Tab(Android 4.0.4)