Closed Bug 688677 Opened 8 years ago Closed 8 years ago

[TABLETUI] Add polish to actionbar / urlbar layout

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 9
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

Attachments

(5 files, 6 obsolete files)

No description provided.
Blocks: 655762
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Attached patch Patch v1.1 (obsolete) — Splinter Review
Some extra tweaks
Attachment #562189 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #562202 - Attachment is obsolete: true
Attachment #562571 - Flags: review?(mbrubeck)
Attached image screenshot with patch (obsolete) —
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 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).
Attachment #562571 - Flags: review?(mbrubeck) → review+
This patch doesn't make me sad. But the "0px" do get my OCD going.
> 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.
Attached patch Patch v2 (obsolete) — Splinter Review
Updated to use a 24x24 favicon in the urlbar. I'll post a screenshot in a sec.
Attachment #562571 - Attachment is obsolete: true
Attached image 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.
Attachment #562574 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed comments. I like this better without -moz-crisp-edges, but it may be that I just happen to visit sites where it works...
Attachment #562595 - Attachment is obsolete: true
Attachment #562604 - Flags: review?(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.
Attachment #562604 - Flags: review?(mbrubeck) → review-
(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?
(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...
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 :)
Also, let's please not change any font sizes yet, they're pretty bang on right now
Attached patch Patch v2.1Splinter Review
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?
Attachment #562604 - Attachment is obsolete: true
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.
Attachment #562614 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ca14a168605b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.