732104 - Make sure BACK always takes you up a folder level in the bookmarks UI (and make that the only way to go up a folder level)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. go to the awesome page after syncing
2. click on bookmarks
3. click in a folder
4. click on the history tab
5. click on the bookmarks tab
6. click the back button on the android device

Expectation : go up a folder?
Actual: exits the awesome page to content page... though this is designed behavior.

Note: 
1. looking at the folder name, it's not apparent to click on that to go up a folder level; an icon could be useful in order to signify to go up a folder level
2. my ux issue is that : It's not apparent to click on the folder name to go up the folder level after clicking on another awesomebar tab.
3. Galaxy Captivate, Nightly, 3/1/2012

Comment 1

[:Margaret Leibovic]

Oh, I was confused by our discussion on IRC and didn't realize exactly what you were experiencing. If you're in a sub-folder, clicking the back button should always take you up a folder level, not exit the awesome screen, so this isn't working as designed. You shouldn't ever need to click on the header to go up a folder level - that's just a secret bonus feature from when I was trying to copy XUL fennec.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Oh!  Well, I do like the secret bonus feature on top of that. 

I'm not sure though if we should be able just to go up a folder there, or exit... still would like ux input in regards to this in what they think.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

assigning to ian to get a UX design

Comment 4

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

Ian - We might need some styling to make it easier to see that we can tap the element to go back up the bookmark hierarchy. If you don't think this blocks, let us know.

Comment 5

[Madhava Enros [:madhava]]

Back button should take you up a level.

Comment 6

[Ian Barlow (:ibarlow)]

I think Mark's right, there should be a clearer indicator that, in addition to the back button, tapping that heading should bring you up a level in the tree. 

I'll post something asap

Comment 7

[:Margaret Leibovic]

Oof, mid-aired with Ian, but before he commented, I was going to say...

I think the bug summary needs to change here, unless we want to actually add a "go up a level" indicator to the header as proposed. Tapping the header to go up a level wasn't part of the UI spec, it was just leftover from my initial attempt to copy XUL fennec, and it seemed like there was no harm to leave it in (although I guess there is harm if it leads to confusion).

The main problem is that after switching tabs in the awesomescreen UI, the bookmark folder level seems to get lost, so using the back button fails in that case. This, I need to look into fixing, so if we're not going to change the bug summary here, maybe we should file another bug for that.

... so I guess I'll file another bug?

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It sounds like there needs to be another bug.  One for the back button and one for the ui indication?

Comment 9

[Ian Barlow (:ibarlow)]

Ok, based on our IRC conversation, let's

1. Remove the ability to tap a folder heading to go back
2. Improve the look of the folder heading (coming soon from me!)
3. The Back button should always navigate up the folder tree in the Bookmarks tab, EVEN WHEN you've switched to a different tab and come back.

Comment 10

[:Margaret Leibovic]

Updating the summary to reflect what's going on here.

Comment 11

[Ian Barlow (:ibarlow)]

Attachment: mockup of header tweaks

Changes:
- Match font size to tab font size
- Make heading bold
- Make heading row 20% shorter
- Make rule below heading 3x thicker

Comment 12

[Ian Barlow (:ibarlow)]

Attachment: mockup of header tweaks

oops, uploaded the PSD first, which was not what I wanted to do

Comment 13

[:Margaret Leibovic]

Attachment: (1/3) Make sure BACK always takes you up a folder level

What's happening is that right now we're only handling the case when BACK is pressed when the edit text is focused:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#163

I'm not sure why the edit text doesn't lose focus when you first tap on one of the tabs, but it loses focus after you switch to History then back to Bookmarks. Overriding onBackPressed like this does what we want (this doesn't get called in the edit text focused case).

Comment 14

[:Margaret Leibovic]

Attachment: (2/3) Remove the ability to tap a folder heading to go back

Comment 15

[:Margaret Leibovic]

Attachment: (3/3) Style header row

I copied these styles from awesomebar_header_row.xml so that they'll match the history section headers. I was hoping to just use that file instead of aweseomebar_folder_header_row.xml, but we need the LinearLayout wrapped around the TextView in order to totally collapse the header in the root folder view (I remember having issues trying to figure this out at one point).

Comment 16

[:Margaret Leibovic]

Attachment: screenshot

Comment 17

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

Comment on attachment 606009 [details] [diff] [review]
(1/3) Make sure BACK always takes you up a folder level

Just a note to myself. We are leaving the textbox BACK handling in place so the fullscreen keyboard situation is still handled. Otherwise, we could remove the code here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java#177

Comment 18

[Ian Barlow (:ibarlow)]

Looks great, Margaret!

Comment 19

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> Comment on attachment 606009 [details] [diff] [review]
> (1/3) Make sure BACK always takes you up a folder level
> 
> Just a note to myself. We are leaving the textbox BACK handling in place so
> the fullscreen keyboard situation is still handled. Otherwise, we could
> remove the code here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.
> java#177

Yeah, we need to keep that. That code path was introduced in the first place because we wanted to leave the awesomescreen with one tap (see bug 700951), and now in this folder case, we want to go up a folder level.

Comment 20

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab87931b7ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/59517bc6052c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae04da5efc98

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/eab87931b7ac
https://hg.mozilla.org/mozilla-central/rev/59517bc6052c
https://hg.mozilla.org/mozilla-central/rev/ae04da5efc98

Comment 22

[:Margaret Leibovic]

Comment on attachment 606009 [details] [diff] [review]
(1/3) Make sure BACK always takes you up a folder level

[Approval Request Comment]
Mobile-only. Blocking fennec-1.0.

Comment 23

[:Margaret Leibovic]

Comment on attachment 606010 [details] [diff] [review]
(2/3) Remove the ability to tap a folder heading to go back

[Approval Request Comment]
Mobile-only. Blocking fennec-1.0.

Comment 24

[:Margaret Leibovic]

Comment on attachment 606011 [details] [diff] [review]
(3/3) Style header row

[Approval Request Comment]
Mobile-only. Blocking fennec-1.0.

Comment 25

[Aaron Train [:aaronmt]]

SoftVision, please update any tests with this change.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 606009 [details] [diff] [review]
(1/3) Make sure BACK always takes you up a folder level

[Triage Comment]
Mobile only - approved for Aurora 13.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/4015ada30b5b
https://hg.mozilla.org/releases/mozilla-aurora/rev/4994cb705e9a
https://hg.mozilla.org/releases/mozilla-aurora/rev/4994cb705e9a

Comment 28

[Paul Feher]

Verified/fixed on:

Aurora Fennec 13.0a2 (2012-03-26)
Nightly Fennec 14.0a1 (2012-03-26)
Device: Samsung Nexus S
OS: Android 2.3.6

Testcase added in Litmus
https://litmus.mozilla.org/show_test.cgi?id=53247