Closed Bug 1120004 Opened 5 years ago Closed 5 years ago

Update Reader View controls

Categories

(Firefox for Android :: Reader View, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified
fennec 38+ ---

People

(Reporter: antlam, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

I think that our Reader mode controls could be improved. It seems like a good time to take a look at this now with all the other work going into Reader Mode. The navigational conventions don't resurface anywhere else in our UI or the systems and it seems they're sometimes even overlooked/ not noticeable. E.g. Right now it's not terribly obvious that you can/have to "Add to Reading list" to keep the article in your panel (referred to in bug 1103232)

Also, we could take advantage of Android L (http://www.google.com/design/spec/components/buttons.html#buttons-floating-action-button).

I think an update this to with the goal of offering better UX (much of it doesn't really change, I've only moved the "Add to Reading list" and "Share" button inside), and an updated UI would be really great. My hope is that this makes our awesome controls more obvious, gives the user more of the article, and also aligns better with the rest of our UI. 

Currently molding this design for Tablet as well. The first part would be to unify, and then see how we can offer unique improvements on that type of device.

WIP previews attached. :)
Attached image prev_controlsupdate_mob2.png (obsolete) —
Whoops - thought I attached it. Still working on this
Attached image prev_controlsupdate_mob4.png (obsolete) —
Addressing some feedback, I decided to go with something less drastic for this update. 

I think we should also loop in bug 998031 at the same time to help with issues of discover-ability.

Mocks for Tablet to follow.
Attachment #8547688 - Attachment is obsolete: true
Attached image prev_controlsupdate_tab4.png (obsolete) —
Attaching Tablet mock. Not looking to change too much here. 

Uses the same menu background image (and therefore shadow) but maybe not the triangle. I'd like to try and move away from using that arrow (and I will mock something similar up for bug 1088220 as well) because it seems almost unnecessary now, and currently aligns weirdly in our UI.

Nothing else should change about our current implementation of the controls and which device & orientation combination gets what.
As part of this (or maybe as its own bug), I think we should also update the share icon to be a paper airplane icon, which will match what's in the desktop mock-ups.
Attached image prev_controlsupdate_mob5.png (obsolete) —
Just realized I forgot the "Aa" button's active state. Also thinking about moving back to +/- that just change the type by a multiple so this design would handle that (taking into consideration iOS too).
Attachment #8548411 - Attachment is obsolete: true
Blocks: 1132054
Attached image prev_controlsupdate_mob6.png (obsolete) —
Updates for Clear Sans, and the "Add to Reading list" icon. Desktop has a + inside a circle but after talking to Maslaney, there are lots of nuances involved in unifying this icon. So, this updated icon at least brings us a step closer (+ is more obvious) for the time being.

Will attach assets later.
Attachment #8556555 - Attachment is obsolete: true
I'm going to try to get around to this bug this week. It might be hard to land something this week, but if this ends up being mostly CSS changes, it should be upliftable. The main risky bit would be changing the font size controls to +/-, especially if that makes us diverge from the destkop controls, since right now we actually share the same logic for these controls.
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → ?
(In reply to :Margaret Leibovic from comment #7)
> The main risky bit would be changing the font size
> controls to +/-, especially if that makes us diverge from the destkop
> controls, since right now we actually share the same logic for these
> controls.

Definitely, I'm going to talk to the rest of the team this week to get us on the same page about this.
Status: NEW → ASSIGNED
Hardware: x86 → All
Summary: Update Reader mode controls on Mobile → Update Reader View controls
Spoke with the team and we're going to go with + / - across platforms for V1. Mocks to follow.
Attachment #8549049 - Attachment is obsolete: true
Attachment #8564203 - Attachment is obsolete: true
A few questions:
* What's that circle icon at the left of the controls?
* Can we update our share button to be the same as desktop's plane icon?
* You're okay with Serif/Sans-Serif instead of the font names? I thought you had veto-ed that before.
* Is there a desktop bug for updating these controls to the +/- as well?
Flags: needinfo?(alam)
Hey Margaret! sorry about the brevity, wanted to file that before the end of the day and just signed off right after.

(In reply to :Margaret Leibovic from comment #11)
> A few questions:
> * What's that circle icon at the left of the controls?

This is a "Mark as read/unread" button that our iOS side has in their controls and I'm hoping to get some consistency by including this. Especially if we're adding indicators, it'd be nice to give users control over this. I'm thinking we would also leverage our toast's to provide user feedback too.

> * Can we update our share button to be the same as desktop's plane icon?

I know I wanted to get this done as well. But, there seems to be a variety of places in which we use this across all our different platforms. I'm going to start a thread and include the relevant parties to sort this out. In the meantime, we should stick to Android conventions for "share" (3 node icon) and keep using the plane for "Send tab to Firefox on my other devices" as we're doing in the Share overlay. 

The plane is also being used for sending and receiving tabs on the iOS side.

> * You're okay with Serif/Sans-Serif instead of the font names? I thought you
> had veto-ed that before.

I did and I still prefer the personal touch of the names. 

But, (and we had a long conversation about this with the iOS and Desktop side) since shipping with same fonts brings up a host of other issues, this keeps things a bit more consistent in the controls and causes less confusion in the mean time. 

This of course might be different as users study the typefaces but we feel like that's less of a concern than provided a unified Reading List / Reader View experience.

> * Is there a desktop bug for updating these controls to the +/- as well?

I didn't file one, maybe Mike did?
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #12)
> Hey Margaret! sorry about the brevity, wanted to file that before the end of
> the day and just signed off right after.
> 
> (In reply to :Margaret Leibovic from comment #11)
> > A few questions:
> > * What's that circle icon at the left of the controls?
> 
> This is a "Mark as read/unread" button that our iOS side has in their
> controls and I'm hoping to get some consistency by including this.
> Especially if we're adding indicators, it'd be nice to give users control
> over this. I'm thinking we would also leverage our toast's to provide user
> feedback too.

Let's split this into a different bug, since this will require adding a new button and some new logic. Is this button going to exist on desktop as well?

> > * Can we update our share button to be the same as desktop's plane icon?
> 
> I know I wanted to get this done as well. But, there seems to be a variety
> of places in which we use this across all our different platforms. I'm going
> to start a thread and include the relevant parties to sort this out. In the
> meantime, we should stick to Android conventions for "share" (3 node icon)
> and keep using the plane for "Send tab to Firefox on my other devices" as
> we're doing in the Share overlay. 
> 
> The plane is also being used for sending and receiving tabs on the iOS side.

Okay, I'll leave the icon alone for Android, then.

> > * You're okay with Serif/Sans-Serif instead of the font names? I thought you
> > had veto-ed that before.
> 
> I did and I still prefer the personal touch of the names. 

I'm confused, since your mock-up has Serif/Sans-serif, not the names. Should I implement what's in this mock-up? Or keep the names?

> But, (and we had a long conversation about this with the iOS and Desktop
> side) since shipping with same fonts brings up a host of other issues, this
> keeps things a bit more consistent in the controls and causes less confusion
> in the mean time. 
> 
> This of course might be different as users study the typefaces but we feel
> like that's less of a concern than provided a unified Reading List / Reader
> View experience.
> 
> > * Is there a desktop bug for updating these controls to the +/- as well?
> 
> I didn't file one, maybe Mike did?

Okay, I'll be on the lookout for that.
Depends on: 1134441
(In reply to :Margaret Leibovic from comment #13)

> Let's split this into a different bug, since this will require adding a new
> button and some new logic.

FWIW, the backend for this will be done when Bug 1130461 lands. Please mark it as a dep of the new bug!
(In reply to :Margaret Leibovic from comment #13)
> (In reply to Anthony Lam (:antlam) from comment #12)
> > Hey Margaret! sorry about the brevity, wanted to file that before the end of
> > the day and just signed off right after.
> > 
> > (In reply to :Margaret Leibovic from comment #11)
> > > A few questions:
> > > * What's that circle icon at the left of the controls?
> > 
> > This is a "Mark as read/unread" button that our iOS side has in their
> > controls and I'm hoping to get some consistency by including this.
> > Especially if we're adding indicators, it'd be nice to give users control
> > over this. I'm thinking we would also leverage our toast's to provide user
> > feedback too.
> 
> Let's split this into a different bug, since this will require adding a new
> button and some new logic. Is this button going to exist on desktop as well?
> 
> > > * Can we update our share button to be the same as desktop's plane icon?
> > 
> > I know I wanted to get this done as well. But, there seems to be a variety
> > of places in which we use this across all our different platforms. I'm going
> > to start a thread and include the relevant parties to sort this out. In the
> > meantime, we should stick to Android conventions for "share" (3 node icon)
> > and keep using the plane for "Send tab to Firefox on my other devices" as
> > we're doing in the Share overlay. 
> > 
> > The plane is also being used for sending and receiving tabs on the iOS side.
> 
> Okay, I'll leave the icon alone for Android, then.
> 
> > > * You're okay with Serif/Sans-Serif instead of the font names? I thought you
> > > had veto-ed that before.
> > 
> > I did and I still prefer the personal touch of the names. 
> 
> I'm confused, since your mock-up has Serif/Sans-serif, not the names. Should
> I implement what's in this mock-up? Or keep the names?

Serif and Sans-serif please!

> > But, (and we had a long conversation about this with the iOS and Desktop
> > side) since shipping with same fonts brings up a host of other issues, this
> > keeps things a bit more consistent in the controls and causes less confusion
> > in the mean time. 
> > 
> > This of course might be different as users study the typefaces but we feel
> > like that's less of a concern than provided a unified Reading List / Reader
> > View experience.
> > 
> > > * Is there a desktop bug for updating these controls to the +/- as well?
> > 
> > I didn't file one, maybe Mike did?
> 
> Okay, I'll be on the lookout for that.

NI-ing Maslaney for the Desktop related notes
Flags: needinfo?(mmaslaney)
tracking-fennec: ? → 38+
FWIW we're going with the standard share icon for iOS, not the shareplane.
I'm going to update the font size controls for both desktop and mobile in bug 1134441, since that will affect the markup/logic of the controls.

As for updating the style for mobile, I'm going to need an orange "Aa" image for the selected state in the controls.

Also, can you specify what the pressed state should look like for these toolbar buttons? Right now a background color is just applied to the rectangular hit area for the buttons.

I also noticed that the serif/sans-serif font sample styles seem to have regressed on Android... I'll have to look into that.
Flags: needinfo?(alam)
Depends on: 1135234
Attached file icon_RView_SVG.zip (obsolete) —
Here are icons in SVG
 - Aa
 - Aa Active
 - Read
 - Unread
 - Add to RL
 - Trash (from RL)
 - Share
Flags: needinfo?(alam)
As for the pressed state, we can just keep it the same as our menu items right now where the background changes to #D7D7DC. I have ideas for something more polished but it needs more work I think (on my end).
Attached file icon_RView.zip
Non- SVG versions for the controls
QA Contact: teodora.vermesan
Color specs:
---

Menu background matte: #EBEBF0
Menu item pressed state: #D7D7DC
Above-menu divider color: #D7D9DB

Typeface active: #222222
Typeface inactive: #AFB1B3
Typeface font-size: 24px ? (converted from 24sp)

Dark-theme button matte: #222222
Dark-theme text: Roboto, regular, 14px? (converted from sp) #EEEEEE
Dark-theme border: 4px rounded corners, 1px solid #BFBFBF

Auto button matte: #EBEBF0
Auto button text: Roboto, regular, 14px? (converted from 14sp) #222222
Auto button border: 4px rounded corners, 1px solid #BFBFBF 

Light-theme button matte: #FFFFFF
Light-theme button text: Roboto, regular, 14px? (converted from 14sp) #222222
Light-theme button border: 4px rounded corners, 1px solid #BFBFBF 

Dark-theme/ Auto/ Light-theme button active: Border 2px #FF9500
Blocks: 1136245
I tried updating the toolbar button icons, but they're all different sizes here, which is causing issues. Currently we use 30x24dpi for those icons. Could you update your new icons to match that size?
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #23)

> Dark-theme button matte: #222222

Currently our dark theme uses #000000. Should I update it to use #222222 instead?
/r/4467 - Bug 1120004 - Update styles of Reader View controls on mobile. r=bnicholson

Pull down this commit:

hg pull review -r 2776c8636dd4fc9e772d87a596f5c2a7acc6a393
Attachment #8570803 - Flags: review?(bnicholson)
Comment on attachment 8570803 [details]
MozReview Request: bz://1120004/margaret

This is still a WIP. Things I need to fix:

* Toolbar icons are improperly scaled
* Try to add "Aa" between font size buttons
* Tablet support!

Here are screenshots so far:
http://cl.ly/image/3m2I1n2J2128
http://cl.ly/image/2j2Y3a280C3z

This switch to using proper buttons also gives us keyboard navigation for free on desktop, which is pretty sweet. And it fixes bug 1136245.
Attachment #8570803 - Flags: review?(bnicholson) → feedback?(bnicholson)
(In reply to :Margaret Leibovic from comment #25)
> (In reply to Anthony Lam (:antlam) from comment #23)
> 
> > Dark-theme button matte: #222222
> 
> Currently our dark theme uses #000000. Should I update it to use #222222
> instead?

Yes please! That was a conscious decision we made in our hack week and I think it was spec'd for Desktop too.
(In reply to :Margaret Leibovic from comment #24)
> I tried updating the toolbar button icons, but they're all different sizes
> here, which is causing issues. Currently we use 30x24dpi for those icons.
> Could you update your new icons to match that size?

Ah, I know why! I forgot to mention, my design makes the controls along the bottom 56dp tall (in accordance with Android L guidelines) rather than 48 dp (our current toolbar height). It was a conscious decision at the time since I felt our controls were a bit small and harder to notice (especially on larger phones).

After playing with this for a while now, this does increase tab-ability for us although it strays from our current 48 dp height. Can we try the new 56 dp height? 

I mocked this up in framer and tested it on my N5, seemed quite comfortable. (56dp = 168px tall, then plus the divider).
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
No longer blocks: 1136245
Duplicate of this bug: 1136245
No longer blocks: 1103232
Flags: needinfo?(margaret.leibovic)
Depends on: 998031
Duplicate of this bug: 805569
Comment on attachment 8570803 [details]
MozReview Request: bz://1120004/margaret

Looks fine to me so far.
Attachment #8570803 - Flags: feedback?(bnicholson) → feedback+
(In reply to Anthony Lam (:antlam) from comment #29)
> (In reply to :Margaret Leibovic from comment #24)
> > I tried updating the toolbar button icons, but they're all different sizes
> > here, which is causing issues. Currently we use 30x24dpi for those icons.
> > Could you update your new icons to match that size?
> 
> Ah, I know why! I forgot to mention, my design makes the controls along the
> bottom 56dp tall (in accordance with Android L guidelines) rather than 48 dp
> (our current toolbar height). It was a conscious decision at the time since
> I felt our controls were a bit small and harder to notice (especially on
> larger phones).
> 
> After playing with this for a while now, this does increase tab-ability for
> us although it strays from our current 48 dp height. Can we try the new 56
> dp height? 
> 
> I mocked this up in framer and tested it on my N5, seemed quite comfortable.
> (56dp = 168px tall, then plus the divider).

Okay, I can try this out, but could you make the icons all the same dimensions?

The way that we do hdpi/xhdpi with CSS is that we explicitly set the pixel size of the image to the mdpi value, then include the higher resolution icons for other resolutions, and it's easier if I can just set a generic width/height for all of the icons.

Currently, all the icons in the toolbar are 30x24dp, and we can increase that, but it would make my life a lot easier if we increased them all consistently.
Flags: needinfo?(alam)
Attached file icon_Padded.zip
Try these! Every icon (MDPI) is 30dp x 28dp. I've included the others just in case.
Attachment #8568837 - Attachment is obsolete: true
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
Attachment #8570803 - Flags: feedback+ → review?(bnicholson)
Comment on attachment 8570803 [details]
MozReview Request: bz://1120004/margaret

/r/4467 - Bug 1120004 - Update styles of Reader View controls on mobile. r=bnicholson

Pull down this commit:

hg pull review -r fb5a79de7324b1766decccd3c216e02ad3eab3e3
FYI, I pngcrush-ed all the new icons.

Here are screenshots:
http://cl.ly/image/1t0m2D0g2q0c
http://cl.ly/image/1I1S2a1Q1y2K
http://cl.ly/image/1m2c382e3V0Q
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #36)

> http://cl.ly/image/1I1S2a1Q1y2K

(This tablet one is a bit out of date, and the +/- buttons are closer to the center in the final version of my patch)
Comment on attachment 8570803 [details]
MozReview Request: bz://1120004/margaret

https://reviewboard.mozilla.org/r/4465/#review3755

Ship It!
Attachment #8570803 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/d819aa9d47f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Tested using:
Build: Firefox for Android 39.0a1 (2015-03-05)
Phone: LG Nexus 4 (Android 4.4)
Tablet: Sony Zperia Z2 (Android 4.4.4) 
Reader view controls for tablet:
-portrait: http://i.imgur.com/iH82HM6.png
-landscape: http://i.imgur.com/h1vnNNd.png
Reader view controls for phone:
-portrait: http://i.imgur.com/CVhXyHi.png
-landscape: http://i.imgur.com/lVoidyX.png
Tested using:
Build: Firefox for Android 38.0a2 (2015-03-06)
Phone: Alcatel One Touch (Android 4.1.2)
Tablet: Sony Zperia Z2 (Android 4.4.4) 
Reader view controls for tablet:
-portrait: http://i.imgur.com/efJNtpX.png
-landscape: http://i.imgur.com/HJaKbY0.png
Reader view controls for phone:
-portrait: http://i.imgur.com/vfcv0Sg.png
-landscape: http://i.imgur.com/q5QKrN0.png
Depends on: 1141317
Depends on: 1142528
Based on the screenshots from comment 41 and comment 43, I will mark status-firefox38 and 39 as verified. I will leave the overall status as resolved fixed due to bug blocks and dependencies.
Attachment #8570803 - Attachment is obsolete: true
Attachment #8619098 - Flags: review+
You need to log in before you can comment on or make changes to this bug.