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

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: fryn, Assigned: jwilde)

Tracking

Trunk
x86
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(6 attachments, 5 obsolete attachments)

77.75 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
16.29 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
2.68 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
26.86 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
10.20 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
7.38 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Implement the styles/states defined in bug 811390.

I estimate this to be a p=3.
(Reporter)

Updated

6 years ago
Blocks: 881837
(Reporter)

Updated

5 years ago
Assignee: fryn → jwilde
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 776742 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #776742 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 3

5 years ago
Created attachment 776748 [details] [diff] [review]
part2 - simplifying some of the markup for the browser

Simplifying some of the markup in the nav bar.
Attachment #776748 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 4

5 years ago
Created attachment 776750 [details] [diff] [review]
part3 - update toolbar buttons, add 1.8x versions
Attachment #776750 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 5

5 years ago
Created attachment 776828 [details] [diff] [review]
part3 - update existing toolbar buttons with 1.4x modes, add 1.8x versions
Attachment #776750 - Attachment is obsolete: true
Attachment #776750 - Flags: feedback?(mbrubeck)
Attachment #776828 - Flags: review?(mbrubeck)
(Assignee)

Comment 6

5 years ago
Created attachment 776829 [details] [diff] [review]
part4 - implement updated stop/reload/go button
Attachment #776829 - Flags: review?(mbrubeck)
(Assignee)

Comment 7

5 years ago
Created attachment 776845 [details] [diff] [review]
part5 - fix back button in and cleanup panelUI
Attachment #776845 - Flags: review?(mbrubeck)
(Assignee)

Comment 8

5 years ago
Created attachment 776846 [details] [diff] [review]
part6 - proper disabled states
Attachment #776846 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #776846 - Attachment is patch: true
Attachment #776846 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #776828 - Flags: review?(mbrubeck) → review+
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.  :)
Attachment #776742 - Flags: feedback?(mbrubeck) → feedback+
(Assignee)

Comment 11

5 years ago
(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 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.
Attachment #776748 - Flags: feedback?(mbrubeck) → feedback+
Comment on attachment 776829 [details] [diff] [review]
part4 - implement updated stop/reload/go button

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

Nice!
Attachment #776829 - Flags: review?(mbrubeck) → review+
Attachment #776845 - Flags: review?(mbrubeck) → review+
Attachment #776846 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 14

5 years ago
(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. :/
(Assignee)

Comment 15

5 years ago
Created attachment 777391 [details] [diff] [review]
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.
Attachment #777391 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #776742 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #777391 - Attachment is patch: true
Attachment #777391 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 16

5 years ago
Created attachment 777393 [details] [diff] [review]
part2 v1.1 - simplifying some of the markup for the browser

Addressing feedback, dropping helperapp-target here.
Attachment #777393 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #776748 - Attachment is obsolete: true
Attachment #777391 - Flags: review?(mbrubeck) → review+
Attachment #777393 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 20

5 years ago
Created attachment 778232 [details] [diff] [review]
part2 v1.2 - simplify markup in browser.xul, do some cleanup elsewhere
Attachment #777393 - Attachment is obsolete: true
Attachment #778232 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #778232 - Attachment description: browser-simplify-dom-v1.2.patch → part1 v1.2 - simplify markup in browser.xul, do some cleanup elsewhere
(Assignee)

Updated

5 years ago
Attachment #778232 - Attachment description: part1 v1.2 - simplify markup in browser.xul, do some cleanup elsewhere → part2 v1.2 - simplify markup in browser.xul, do some cleanup elsewhere
(Assignee)

Comment 21

5 years ago
Created attachment 778237 [details] [diff] [review]
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.
Attachment #776845 - Attachment is obsolete: true
Attachment #778237 - Flags: review?(mbrubeck)
Attachment #778237 - Flags: review?(mbrubeck) → review+
Attachment #778232 - Flags: review?(mbrubeck) → review+
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.