Closed Bug 1265433 Opened 3 years ago Closed 3 years ago

Improve main menu after removing "add to reading list"

Categories

(Firefox for Android :: Awesomescreen, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- verified
firefox49 --- verified

People

(Reporter: ahunt, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
130.95 KB, image/png
Details
596.02 KB, image/png
antlam
: feedback-
Details
520.47 KB, image/png
Details
68.28 KB, image/png
Details
508.84 KB, image/png
Details
58 bytes, text/x-review-board-request
Details
93.43 KB, image/png
Details
93.16 KB, image/png
Details
100.07 KB, image/png
Details
154.65 KB, image/png
Details
253.02 KB, image/png
Details
See the mocks and comments in Bug 1234319 - we've removed the "add to reading" list button which makes the main menu look rather odd.

We'll want this in 48, which might require uplifting (but hopefully we can get this done this week).
Assignee: nobody → ahunt
Depends on: 1234319
We only care about API >= 14, so there's no need for the pre-v11
menu.

Review commit: https://reviewboard.mozilla.org/r/47629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47629/
Attachment #8743187 - Flags: review?(s.kaspari)
Attachment #8743188 - Flags: review?(s.kaspari)
We also need to shuffle our icon sizes around: our xhdpi icon size matches
the hdpi icon sizes of the other icons in the top bar (modulo padding, which
we need to add as part of this commit), similarly our xxhdpi icon is shifted to
xhdpi. We don't supply an xxhdpi icon at all for any of the icons in the top bar
of the menu, hence that size is removed (and reused for the xhdpi icon, see above).

Review commit: https://reviewboard.mozilla.org/r/47631/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47631/
Attached image menu_4items.png
Here's a screenshot of the new menu - I'm not sure if we had a firm decision regarding removing the back button, given the telemetry data I'd be wary of removing it?
Blocks: migrate-RL
Status: NEW → ASSIGNED
Attachment #8743187 - Flags: review?(s.kaspari) → review+
Comment on attachment 8743187 [details]
MozReview Request: Bug 1265433 - Pre: remove v11 prefixes from menus r=sebastian

https://reviewboard.mozilla.org/r/47629/#review44533
Comment on attachment 8743188 [details]
MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian

Something doesn't seem to be right with the "Share" menu item. After sharing the quick share button(s) show up but the "Share" item does not go away and the share icon does not appear next to the quick buttons - this seems to be wrong?

I'll upload some screenshots.
Attachment #8743188 - Flags: review?(s.kaspari)
Is this correct? Something is weird about having two share rows.
Attachment #8743403 - Flags: feedback?(alam)
Comment on attachment 8743403 [details]
Screenshot_20160420-194025.png

This should not happen. 

I'll attach a mock in a bit but basically "Share" should be the default state. But once the user presses on some sharing options, we should be over riding that row with our usual app icons.
Attachment #8743403 - Flags: feedback?(alam) → feedback-
Looking closely at the screenshot in comment 6, there seems to be a weird 1 dp transparency around the edges of the menu. It seems to only be there for items below the first row.

Do you see that? we should fix that too, it looks unfinished. :(
Flags: needinfo?(ahunt)
^looks like that work is being tracked in bug 1148552.
(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> Comment on attachment 8743188 [details]
> MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu
> r?sebastian
> 
> Something doesn't seem to be right with the "Share" menu item. After sharing
> the quick share button(s) show up but the "Share" item does not go away and
> the share icon does not appear next to the quick buttons - this seems to be
> wrong?
> 
> I'll upload some screenshots.

Huh, I was testing on an Android 4 device, and I never realised we're doing different things on newer versions (on 4 I get a submenu appearing when I select the share menu item). I'll need to investigate a bit more!
Flags: needinfo?(ahunt)
Attached image prev_overflowmenu4.png
This is what it should look like with 4 items along the top. 

Will attach specs later.
Attached image spec_menuspacing1.png
Here's how the spacing is spec'd out. Icons should be the same and are just centered in the space.

Does this make sense?
Here's the menu before a user "shares" a page. After which, we can cap the quick share as depicted in the earlier screenshot.
Review commit: https://reviewboard.mozilla.org/r/49253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49253/
Attachment #8743187 - Attachment description: MozReview Request: Bug 1265433 - Pre: remove v11 prefixes from menus r?sebastian → MozReview Request: Bug 1265433 - Pre: remove v11 prefixes from menus r=sebastian
Attachment #8743188 - Attachment description: MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian → MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu
Attachment #8743188 - Flags: review?(s.kaspari)
Comment on attachment 8743187 [details]
MozReview Request: Bug 1265433 - Pre: remove v11 prefixes from menus r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47629/diff/1-2/
Comment on attachment 8743188 [details]
MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47631/diff/1-2/
Comment on attachment 8743188 [details]
MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian

I think I've now managed to build the right behaviour, I'm going to try and figure out if we can simplify this code more though. The code seems extremely unobvious (it needed to be more abstract for the previous menu implementation), however I'm not sure how much work simplifying it would actually be.
Attachment #8743188 - Flags: review?(s.kaspari)
Can you share a screenshot in the meantime? :)
Comment on attachment 8746094 [details]
MozReview Request: Bug 1265433 - Pre: remove unnecessary v11 checks r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49253/diff/1-2/
Attachment #8743188 - Attachment description: MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu → MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian
Attachment #8743188 - Flags: review?(s.kaspari)
Comment on attachment 8743187 [details]
MozReview Request: Bug 1265433 - Pre: remove v11 prefixes from menus r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47629/diff/2-3/
Comment on attachment 8743188 [details]
MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47631/diff/2-3/
This is (a screenshot of) what the menu looks like before sharing anything.
This is (a screenshot of) what the menu looks like after sharing once.
This is (a screenshot of) what the menu looks like after sharing 2 or more times (the most recent 2 items are listed, the left one, i.e. center icon, is the most recent share).
Comment on attachment 8743188 [details]
MozReview Request: Bug 1265433 - Move bookmarks star into top bar of menu r?sebastian

https://reviewboard.mozilla.org/r/47631/#review46705

LGTM.

The star looks a bit "frayed" on the screenshots (It looks better on an actual device). Do we need higher quality resources here?
Attachment #8743188 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/1a5b75c9e522
https://hg.mozilla.org/mozilla-central/rev/f164661f36de
https://hg.mozilla.org/mozilla-central/rev/8faf7ba00b31
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 49.0a1 (2015-05-03)
Verified as fixed using:
Devices: Moto X (Android 4.4), Nexus 7 (Android 5.1.1)
Build: Firefox for Android 48.0a2 (2016-06-01)
You need to log in before you can comment on or make changes to this bug.