885242 - Work - Implement complete button styles/states for Firefox app bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P1)

Description

[Frank Yan (:fryn)]

Implement the styles/states defined in bug 811390.

I estimate this to be a p=3.

Comment 1

[Frank Yan (:fryn)]

Note to Jonathan:
The specs for this are located in bug 811390.
Yuan will also have more specifics for things not explicitly in the specs.

Comment 2

[Jonathan Wilde [:jwilde]]

Attachment: part 1 - reorder browser.css to roughly match corresponding blocks in browser.xul

Right now, there's no real order to what's in browser.css. I was thinking that it might be easier to find things if everything was roughly in the same order as the corresponding markup in browser.xul.

Comment 3

[Jonathan Wilde [:jwilde]]

Attachment: part2 - simplifying some of the markup for the browser

Simplifying some of the markup in the nav bar.

Comment 4

[Jonathan Wilde [:jwilde]]

Attachment: part3 - update toolbar buttons, add 1.8x versions

Comment 5

[Jonathan Wilde [:jwilde]]

Attachment: part3 - update existing toolbar buttons with 1.4x modes, add 1.8x versions

Comment 6

[Jonathan Wilde [:jwilde]]

Attachment: part4 - implement updated stop/reload/go button

Comment 7

[Jonathan Wilde [:jwilde]]

Attachment: part5 - fix back button in and cleanup panelUI

Comment 8

[Jonathan Wilde [:jwilde]]

Attachment: part6 - proper disabled states

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776828 [details] [diff] [review]
part3 - update existing toolbar buttons with 1.4x modes, add 1.8x versions

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

r=mbrubeck with one suggested change:

::: browser/metro/theme/defines.inc
@@ +99,5 @@
> +% minimum resolution cutoffs for displaying 1.4x and 1.8x versions of icons
> +% XXX as discussed in bug 812529, calculated DPIs currently come out low,
> +% so we have to use excessively low cutoff values here
> +%define min_res_140pc 130dpi
> +%define min_res_180pc 170dpi
\ No newline at end of file

Since 1in == 96px in CSS units, 1dppx == 96dpi.

This means that 1.4dppx == 134.4dpi, and 1.8dppx == 172.8dpi.

I think "min-resolution: 1.4dppx" and "min-resolution: 1.8dppx" is the most straightforward way to write these media queries.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776742 [details] [diff] [review]
part 1 - reorder browser.css to roughly match corresponding blocks in browser.xul

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

I'm fine with this.  I'm taking it on faith that this patch is just moving code around; I haven't verified that by hand.  :)

Comment 11

[Jonathan Wilde [:jwilde]]

(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> Comment on attachment 776742 [details] [diff] [review]
> part 1 - reorder browser.css to roughly match corresponding blocks in
> browser.xul
> 
> Review of attachment 776742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with this.  I'm taking it on faith that this patch is just moving
> code around; I haven't verified that by hand.  :)

There's one code drop:
#helperapp-target, since that ID doesn't exist anywhere anymore.

http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=helperapp-target&redirect=true

Perhaps that'd be better to move into the part2 patch.

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776748 [details] [diff] [review]
part2 - simplifying some of the markup for the browser

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

::: browser/metro/base/content/browser.xul
@@ +352,5 @@
>      </vbox>
>  
>      <!-- Find bar -->
> +    <appbar id="findbar" class="window-width findbar-box" pack="start">
> +      <textbox id="findbar-textbox" class="findbar-item" type="search"

Let's get rid of the unused "findbar-box" and "findbar-item" classes while we're here.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776829 [details] [diff] [review]
part4 - implement updated stop/reload/go button

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

Nice!

Comment 14

[Jonathan Wilde [:jwilde]]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> Comment on attachment 776828 [details] [diff] [review]
> part3 - update existing toolbar buttons with 1.4x modes, add 1.8x versions
> 
> Review of attachment 776828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=mbrubeck with one suggested change:
> 
> ::: browser/metro/theme/defines.inc
> @@ +99,5 @@
> > +% minimum resolution cutoffs for displaying 1.4x and 1.8x versions of icons
> > +% XXX as discussed in bug 812529, calculated DPIs currently come out low,
> > +% so we have to use excessively low cutoff values here
> > +%define min_res_140pc 130dpi
> > +%define min_res_180pc 170dpi
> \ No newline at end of file
> 
> Since 1in == 96px in CSS units, 1dppx == 96dpi.
> 
> This means that 1.4dppx == 134.4dpi, and 1.8dppx == 172.8dpi.
> 
> I think "min-resolution: 1.4dppx" and "min-resolution: 1.8dppx" is the most
> straightforward way to write these media queries.

Agreed. However, weirdly, when I use 1.4dppx and 1.8dppx for the min-resolution, the 1.4x versions of images don't apply until the devPixelsPerPx pref is at least 1.412. When I use 1.39dppx for the min-resolution, it works just fine. The 1.8x versions work just fine.

I'd normally think that dppx should just pull from the pref, but this sounds very similar to what's going on in bug 812529. :/

Comment 15

[Jonathan Wilde [:jwilde]]

Attachment: part1 v1.1 - reorder browser.css to roughly match corresponding blocks in browser.xul

Moved removal of helperapp-target to the part 2 v1.1 patch.

Comment 16

[Jonathan Wilde [:jwilde]]

Attachment: part2 v1.1 - simplifying some of the markup for the browser

Addressing feedback, dropping helperapp-target here.

Comment 17

[Jonathan Wilde [:jwilde]]

As discussed in the channel, we'll be landing part2 with the min_res_140_pc def set to 1.39dppx because of weird rounding issues with the dppx unit. I've filed the Core :: Layout bug 895277 with a testcase demonstrating this issue.

Comment 18

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b27487d6814
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb45f94edb1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9563a9ea0ce2
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ae0e6a6fec
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e4c716ea9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3297faeb61

Comment 19

[Ed Morley [:emorley]]

Backed out for metro-chrome suite failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25420920&tree=Mozilla-Inbound

Did you run the testsuite locally before pushing? :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c81600630212
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1324bb6e19
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/acb70848a575
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f97cf639d081
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6e68dbef7b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/089c92108cef

Comment 20

[Jonathan Wilde [:jwilde]]

Attachment: part2 v1.2 - simplify markup in browser.xul, do some cleanup elsewhere

Comment 21

[Jonathan Wilde [:jwilde]]

Attachment: part5 v1.1 - fix back button in and cleanup panelUI

I had to do some pretty significant hand un-bitrotting, so I'm going to chuck this through review again just to be safe.

Comment 22

[Jonathan Wilde [:jwilde]]

Now that I have a Windows 8 device (in addition to 8.1) rolling again, minor unbitrotting and relanding. And yes, all of the tests (including perf), work now. :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/28e87c73927d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e70887b953f
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4581ec9681
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f1a0de07d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1f8cbdcc8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c64f2402e12e

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28e87c73927d
https://hg.mozilla.org/mozilla-central/rev/4e70887b953f
https://hg.mozilla.org/mozilla-central/rev/de4581ec9681
https://hg.mozilla.org/mozilla-central/rev/08f1a0de07d8
https://hg.mozilla.org/mozilla-central/rev/3f1f8cbdcc8c
https://hg.mozilla.org/mozilla-central/rev/c64f2402e12e