Closed Bug 873251 Opened 7 years ago Closed 7 years ago

Work - Update buttons and backgrounds of Firefox app bar

Categories

(Firefox for Metro Graveyard :: Theme, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fryn, Assigned: fryn)

References

Details

Attachments

(2 files, 2 obsolete files)

Stephen just provided me with a bunch of updated icons (including some HiDPI icons) for the Firefox app bar. Using these assets, we can update the buttons and backgrounds of the Firefox app bar and clean up some of the code as a side effect of doing that.

This gets us halfway done with bug 811390 and also halfway done with bug 816814.

I'm beginning work on this right now, and I estimate that this is a p=2.
Marco, if you think this fits better under another story, feel free to move it.
Component: Metro Operations → Theme
Product: Tracking → Firefox for Metro
Version: --- → Trunk
Blocks: 816814, 811390
No longer blocks: metrov1it7
Duplicate of this bug: 881874
Blocks: metrov1it9
No longer blocks: metrov1it9
Blocks: 880986
Duplicate of this bug: 882942
Blocks: 845152
Attached patch patch (obsolete) — Splinter Review
The resolution cut-off should really be 1.4dppx or 135px, not 130px, but we need to fix bug 883405 first for that to work.

There are many files and lines of CSS changing here, so could you test this locally too?
Attachment #762995 - Flags: review?(jwilde)
No longer blocks: 845152
Comment on attachment 762995 [details] [diff] [review]
patch

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

Some of my patches bitrotted this patch. >.<
I unbitrotted it locally and tested it out and ran into an issue with the menu button noted below.

::: browser/metro/base/content/appbar.js
@@ +7,5 @@
>    get consoleButton() { return document.getElementById('console-button'); },
>    get jsShellButton() { return document.getElementById('jsshell-button'); },
>    get starButton()    { return document.getElementById('star-button'); },
>    get pinButton()     { return document.getElementById('pin-button'); },
> +  get menuButton()    { return document.getElementById('menu-button'); },

After applying this patch, the menu button's click handler no longer functions. moreButton is referenced down below and needs to be updated in order for the menu to appear and be positioned correctly:

https://hg.mozilla.org/mozilla-central/file/36da3cb92193/browser/metro/base/content/appbar.js#l127

Looking at that code, over the long term, we should probably consider adding proper support for XUL menu buttons to simplify future UI work.

::: browser/metro/theme/browser.css
@@ +266,5 @@
>  
> +@media (min-resolution: 130dpi) {
> +  #toolbar > #back-button {
> +    list-style-image: url(chrome://browser/skin/images/appbar-back@1.4x.png);
> +  }  

Nit: extra whitespace.

@@ +422,3 @@
>  /* Application-Specific */
>  #download-button {
>    -moz-image-region: rect(0px, 40px, 40px, 0px) !important;

We're going to need new download button assets at some point. I assume that's going to land separately as a part of the new download work in bug 831942?
Attachment #762995 - Flags: review?(jwilde)
Attached patch patch (unbitrotted) (obsolete) — Splinter Review
Attachment #763698 - Attachment is patch: true
Attachment #763698 - Attachment mime type: text/x-patch → text/plain
Attached patch patch v2Splinter Review
Thanks for the review! :)

(In reply to Jonathan Wilde [:jwilde] from comment #4)
> We're going to need new download button assets at some point. I assume
> that's going to land separately as a part of the new download work in bug
> 831942?

Yes. Even if it's not bug 831942, the download button assets were not among the images that Stephen provided me, so it's out of scope for this bug.
Attachment #762995 - Attachment is obsolete: true
Attachment #763698 - Attachment is obsolete: true
Attachment #764478 - Flags: review?(jwilde)
Comment on attachment 764478 [details] [diff] [review]
patch v2

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

:D
Attachment #764478 - Flags: review?(jwilde) → review+
This broke browser_selection_urlbar.js - https://tbpl.mozilla.org/php/getParsedLog.php?id=24316318&tree=Mozilla-Inbound. Probably need to adjust the magic numbers to get it selecting over where the urlbar is, instead of where it was.
Depends on: 884677
(In reply to Phil Ringnalda (:philor) from comment #9)
> This broke browser_selection_urlbar.js -
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24316318&tree=Mozilla-
> Inbound. Probably need to adjust the magic numbers to get it selecting over
> where the urlbar is, instead of where it was.

Why didn't the patch here get backed out on inbound?
(In reply to Jim Mathies [:jimm] from comment #10)
> Why didn't the patch here get backed out on inbound?

Because one of jwilde's patches that also landed depends on the patch here, because metro-chrome failures aren't considered top-tier oranges, and perhaps also because the test is written in an awfully fragile way.
(In reply to Frank Yan (:fryn) from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > Why didn't the patch here get backed out on inbound?
> 
> Because one of jwilde's patches that also landed depends on the patch here,
> because metro-chrome failures aren't considered top-tier oranges, and

The whole point of being visible on tbpl is that we are considered top-tier. I'm not sure how this decision got made but we need to reverse it and make sure tree watchers know.
 
> perhaps also because the test is written in an awfully fragile way.

Then back out the patch, fix the patch or fix the test or both. But busted tests on inbound shouldn't be ignored, and patches that land there that break things should be backed out. But you already knew this. ;)
(In reply to Jim Mathies [:jimm] from comment #12)
> The whole point of being visible on tbpl is that we are considered top-tier.
> I'm not sure how this decision got made but we need to reverse it and make
> sure tree watchers know.
>  
> > perhaps also because the test is written in an awfully fragile way.
> 
> Then back out the patch, fix the patch or fix the test or both. But busted
> tests on inbound shouldn't be ignored, and patches that land there that
> break things should be backed out. But you already knew this. ;)

If it's visible it requires backout [1]. The only thing I can think of is that philor (our superstar volunteer sheriff for PST/evening PST) didn't have a tree available at the time (eg he was at his non-mozilla job) or else he thought the test tweak would be easy enough that it could land straight away. 

Anyway this is backed out now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59679baba21b
(As is bug 811392 since comment 11 implies it relies on the patch backed out)


[1] https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#2.29_Breakage_is_expected_to_be_followed_by_tree_closure_or_backout
Duplicate of this bug: 884712
No, actually I had no idea it was visible (I just watch &showall=1 for all trees and see everything everywhere), since there's absolutely no way it should be since it doesn't meet the most basic visibility requirement. If you ask releng for "on inbound and central" you are saying "I just want to run this hidden and for my own purposes." If you want to be visible, what you want is "on all trunk trees" or "on all trees that feed mozilla-central."

Looks like the only reason it's visible is because the name changed, which made it a new job as far as tbpl was concerned. It's now hidden under the new name; once it's running everywhere, feel free to file a webtools::tinderboxpushlog bug about unhiding it (though if its primary failure still consists of printing that unstarrable FAIL-SHOULD-RETRY without automation that knows to retry based on it, it's not going to meet point 6, either).
Ah I didn't realise it wasn't running on the other trees, thank you for spotting that. Yeah it shouldn't be visible in the default view then.
mc tests should be running on all trees that feed mc mc opt visible to catch bustage. I was under the impression it was.
(In reply to Ed Morley [:edmorley UTC+1] from comment #18)
> eg:
> https://tbpl.mozilla.org/?tree=Services-Central&showall=1&jobname=metro

metro tests will never run on services central, they are browser specific.

I've reopened bug 847442 to finish this work.
(In reply to Jim Mathies [:jimm] from comment #19)
> metro tests will never run on services central, they are browser specific.

Slightly puzzled, sync & browser changes are made there too
(In reply to Ed Morley [:edmorley UTC+1] from comment #20)
> (In reply to Jim Mathies [:jimm] from comment #19)
> > metro tests will never run on services central, they are browser specific.
> 
> Slightly puzzled, sync & browser changes are made there too

Hmm, maybe you're right. I didn't realize browser-chrome tests run there too.
No longer depends on: 884677
Blocks: 885537
Depends on: 887621
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.