688677 - [TABLETUI] Add polish to actionbar / urlbar layout

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: screenshots with comments

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: more tightly cropped urlbar images

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This fixes up... everything I think. I can't actually check this as thomas reconfiscated his galaxy tab cable from me. I'll have to grab another on the way home tonight. But there's a build at:

http://people.mozilla.com/~wjohnston/fennec-keyhole.apk

I'm holding off on review until I've actually seen this no device.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

Some extra tweaks

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Trying to keep mfinkle from exploding, thought I'd send this to you Matt.

This is pretty close to mockups. Like, I laid them over the top of each other and its really pretty close. I think the mockups are using a smaller favicon size than we actually use in Fennec so that is off, but the rest is... close.

I had a lot of the 1px inset highlight working for the back and forward buttons, but then noticed that have a 2px border on the back button and using a mask was causing some strange rendering artifacts to appear. At that point I just put that piece of this aside.

Had to increase the size of the back button (by removing some padding around its edges). Also decreased the urlbar font-size a bit too. And a lot of padding/margin tweaks.

Also decreased the urlbar size. Its not 5.25mozmm tall which comes out to be around 33px at 160dpi. Most of the icons we're showing there are 32px tall. When I add a two pixel border to the bottom, they all get squished. I cut out a bit more to make a few icons 31x33 where I just couldn't get things to work.

Maybe a better solution is to make the urlbar taller, but try to ensure it doesn't shift up or down when I do so (right now its using align=center). I've been playing with leaving the urlbar at 6.35mozmm tall and using margin-bottom, padding-top, but that means the selected state of things like the forward button not stretches to the top of the backbutton, which is another pain to fix.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot with patch

Ian, do we really want favicons to sit right up against the urlbar bottom border like this?  It feels like it would be better with at least one pixel of padding between the border and the favicon.

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 562571 [details] [diff] [review]
Patch v1

>+@media (@orientation@: landscape) {
>+  #toolbar-main[tablet] {
>+    -moz-padding-start: @padding_xxnormal@;
>+  }
>+}

This adds padding to the left of the curvy tab background if you hide the sidebar in landscape mode.  I think you can add a [tablet_sidebar] to the selector to fix this.  r=mbrubeck with that fix.

I'm also curious about the answer to my question in comment 5, but I think we can land now and fix that later (if it's something to be fixed).

Comment 7

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

This patch doesn't make me sad. But the "0px" do get my OCD going.

Comment 8

[Ian Barlow (:ibarlow)]

> Ian, do we really want favicons to sit right up against the urlbar bottom
> border like this?  It feels like it would be better with at least one pixel
> of padding between the border and the favicon.

I agree, a little bit of breathing room would be nice. Even a pixel or two.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated to use a 24x24 favicon in the urlbar. I'll post a screenshot in a sec.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Screenshot. Just realized I was moving to fix the tablet_sidebar comment and forgot! I'm playing with turning off the crisp edges stuff anyway, so I'll post another patch and a second screenshot for fun.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Fixed comments. I like this better without -moz-crisp-edges, but it may be that I just happen to visit sites where it works...

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Screenshot without crisp-edges

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 562604 [details] [diff] [review]
Patch v2

>+#toolbar-main[tablet][tablet_sidebar] {
>+  -moz-padding-start: @padding_xxnormal@;
>+}

This still needs a "landscape" media query like in your previous patch; otherwise you can get padding applied in portrait mode.

The 24px icons look much better to me than the 32px ones, though opinions may vary.  (They are slightly blurry, rather than very pixelated.)

If we make this change, I think we should also change the Honeycomb awesomescreen icons to 24px; right now they look huge compared to the urlbar.  The awesomescreen font should probably also change to stay in sync with the urlbar.  Some or all of this could be in a followup bug though.

Comment 14

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

(In reply to Wesley Johnston (:wesj) from comment #9)
> Created attachment 562595 [details] [diff] [review] [diff] [details] [review]
> Patch v2
> 
> Updated to use a 24x24 favicon in the urlbar. I'll post a screenshot in a
> sec.

One reason we used 32 was the nice multiple with 16, thinking it would make the scaling faster and cleaner. Maybe that doesn't matter anymore. What about all the other places where we use 32px images?

Comment 15

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

(In reply to Matt Brubeck (:mbrubeck) from comment #13)

> If we make this change, I think we should also change the Honeycomb
> awesomescreen icons to 24px; right now they look huge compared to the
> urlbar.  The awesomescreen font should probably also change to stay in sync
> with the urlbar.  Some or all of this could be in a followup bug though.

This slope is getting slippery...

Comment 16

[Ian Barlow (:ibarlow)]

I like the 24px icon in the header, very nice.

Could we leave the awesomebar / tab bar favicons at 32px for the time being? I'd like to see how it feels with two sizes before we change them all. My sense is that using bigger favicons in the list menus will make them easier to scan. 

To your point, Matt, we can keep tweaking all this in followup bugs :)

Comment 17

[Ian Barlow (:ibarlow)]

Also, let's please not change any font sizes yet, they're pretty bang on right now

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Whoops. I was just seeing that on desktop and trying to figure out what I did wrong. Sorry :(

So in portrait mode "tablet_sidebar" is true, even though there is (and will never be) a sidebar?

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 562614 [details] [diff] [review]
Patch v2.1

Yeah, we keep "tablet_sidebar" set as you rotate to portrait, so that it will still have the correct state when you rotate back to landscape.  But we use media queries to make sure we only pay attention to it in landscape mode.

Comment 20

[Matt Brubeck (:mbrubeck)]

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

Comment 21

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110927
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA A500
OS: Android 3.1