Toolbar refinement - Change entry asset and tweak padding

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

26.58 KB, image/png
Details
7.61 KB, application/zip
Details
117.62 KB, image/png
Details
20.56 KB, image/png
Details
76.76 KB, image/png
Details
5.60 MB, video/quicktime
Details
39.27 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
93.32 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
No description provided.
Current toolbar (bottom) compared to the refined toolbar (top). 

Mockup is to depict overall positioning and layout of icons, tab curve, and input field changes (border radius, padding, border  etc...)
Posted file urlbar_dot9.zip (obsolete) —
Attaching .9 files for the URL bar in both active and default states
Posted image URL bar 4.png
Attaching specs for spacing in dp units
Posted file urlbar_dot9.zip (obsolete) —
Updated to fix .9 file bug
Posted file urlbar_dot9.zip (obsolete) —
Border-radius refinements
Attachment #8423111 - Attachment is obsolete: true
Posted file urlbar_dot9.zip
moar radius!
Attachment #8423120 - Attachment is obsolete: true
Posted image prev_toolbar_states2.png (obsolete) —
Just updating the preview of the changes as per our discussions, Lucas :) (x was too contrasty, etc)
Attachment #8423042 - Attachment is obsolete: true
Played with static images on a Nexus 5 and the uneven spacing didn't come across as well as it did on a laptop. Attaching updated measurements :) maybe we can give these a try.
Attachment #8447497 - Attachment is obsolete: true
Posted image spec_toolbar_x.png
Figured this might help too.
this is REALLLLLY close! (attaching a visual comparison)

On the curve: I seem to have lost the values we're using for the cubic-bezier but if you let me know I can tune it again for you. I think the bottom half of the curve needs to stick out (to the right) just a bit more...

On the input bar: the size is just a bit too big, I think this contributes to making it feel too cramped.
Posted video v3_compare.mov
Also, not sure if this helps Lucas, but I've overlay-ed the current build on top of the design file and just switched back and forth to compare the differences. (here's a MOV file!)
Assignee

Updated

5 years ago
Attachment #8471534 - Flags: review?(michael.l.comella)
Assignee

Updated

5 years ago
Attachment #8471539 - Flags: review?(michael.l.comella)
Comment on attachment 8471534 [details] [diff] [review]
Update icon for the edit cancel button (r=mcomella)

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

lgtm w/ nits considered.

::: mobile/android/base/resources/drawable/close_edit_mode_selector.xml
@@ +9,5 @@
>      <item gecko:state_dark="true"
>            android:drawable="@drawable/close_edit_mode_dark"/>
>  
> +    <item gecko:state_private="true"
> +          android:drawable="@drawable/close_edit_mode_light"/>

"_light" refers to the background color, i.e. this drawable is darker than close_edit_mode_dark. Since private mode has a dark background, are you sure that this should be light?

Semi-relatedly, I'm not sure if this convention makes sense (it took me a lot longer to grow the intuition for it just now than if it was reversed, and I wrote it!) and bring up the option of changing it.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1381,3 @@
>          urlEditLayout.setPrivateMode(isPrivate);
>  
> +

nit: excess ws.
Attachment #8471534 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8471539 [details] [diff] [review]
Change appearance of input entry in toolbar (r=mcomella)

Perhaps I'm missing a patch from an unmarked dependent bug, but it appears that the curve hasn't been changed and, with the reduction of the tabs counter's padding, it looks pretty strange.

However, I trust that you know what's going on here and I don't want to waste more cycles when I'm confident you know what's going on here, so r+.
Attachment #8471539 - Flags: review?(michael.l.comella) → review+
Assignee

Comment 17

5 years ago
(In reply to Michael Comella (:mcomella) from comment #15)
> Comment on attachment 8471534 [details] [diff] [review]
> Update icon for the edit cancel button (r=mcomella)
> 
> Review of attachment 8471534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm w/ nits considered.
> 
> ::: mobile/android/base/resources/drawable/close_edit_mode_selector.xml
> @@ +9,5 @@
> >      <item gecko:state_dark="true"
> >            android:drawable="@drawable/close_edit_mode_dark"/>
> >  
> > +    <item gecko:state_private="true"
> > +          android:drawable="@drawable/close_edit_mode_light"/>
> 
> "_light" refers to the background color, i.e. this drawable is darker than
> close_edit_mode_dark. Since private mode has a dark background, are you sure
> that this should be light?

Yep, close_edit_mode_dark looks a bit too constrasty. We'll only use close_edit_mode_dark when the theme is dark to ensure it's visible with arbitrary background images.

> Semi-relatedly, I'm not sure if this convention makes sense (it took me a
> lot longer to grow the intuition for it just now than if it was reversed,
> and I wrote it!) and bring up the option of changing it.

I agree it's confusing. Our styles desperately need some major cleanups... 

> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +1381,3 @@
> >          urlEditLayout.setPrivateMode(isPrivate);
> >  
> > +
> 
> nit: excess ws.

Fixed.
Assignee

Comment 18

5 years ago
(In reply to Michael Comella (:mcomella) from comment #16)
> Comment on attachment 8471539 [details] [diff] [review]
> Change appearance of input entry in toolbar (r=mcomella)
> 
> Perhaps I'm missing a patch from an unmarked dependent bug, but it appears
> that the curve hasn't been changed and, with the reduction of the tabs
> counter's padding, it looks pretty strange.

Yeah, lots of interdependent changes here, apologies. It will be alright in the end, promise :-)
Backed out along with the rest of the patch stack for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/d18d3e14ac3e
Assignee

Comment 21

5 years ago
The string changes in bug 1010741 caused the failures. Forgot to update the tests accordingly. Try build looks nice and green now, pushed:

https://hg.mozilla.org/integration/fx-team/rev/0a0c695b4576
https://hg.mozilla.org/integration/fx-team/rev/53ad0481f772
https://hg.mozilla.org/mozilla-central/rev/0a0c695b4576
https://hg.mozilla.org/mozilla-central/rev/53ad0481f772
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.