Closed Bug 895519 Opened 11 years ago Closed 11 years ago

Change - Firefox Start tile theme changes based on latest comps

Categories

(Firefox for Metro Graveyard :: Theme, defect, P1)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jimm, Assigned: sfoster)

References

()

Details

(Whiteboard: feature=change c=firefox_start u=metro_firefox_user p=1)

Attachments

(4 files, 3 obsolete files)

Attached image comp_topsites.png
From the latest spec shorlander has up, our Top Site tiles need some tweaks.
Summary: Firefox Start Top Site tile theme changes based on latest comps → Change - Firefox Start Top Site tile theme changes based on latest comps
Whiteboard: feature=change c=tbd u=tbd p=0
Hi Stephan, looking for your input to confirm.
Assignee: nobody → shorlander
Priority: -- → P1
Blocks: 831923
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=firefox_start u=metro_firefox_user p=0
Assignee: shorlander → sfoster
Blocks: metrov1it12
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=change c=firefox_start u=metro_firefox_user p=0 → feature=change c=firefox_start u=metro_firefox_user p=1
Will follow up with some styling, still tweaking it a bit.
Attached image WIP screenshot (obsolete) —
I've got most of this looking pretty good, see screenshot. Your thoughts Stephen?

Notably absent/incomplete: 
* Handle large favicons: the .ico file can contain multiple image sizes. Currently we're just using the url PlacesUtils.favicons.getFaviconURLForPage gives us. Bug 419588 gave us access to these via media-fragments but I've not yet had success putting it to use. Also, we really need to know programmatically what sizes are available, as our treatment (markup/css) differs for small and large icons. This may end up in a follow up bug. 

* Opaque icon box color: We get the dominant color from the favicon via colorUtils and ImageAnalyzer. I can easily make this a tint by setting opacity - or in this case adding the alpha value. But this leaves that box translucent rather than opaque. To get the opaque color value I'll need to convert to HSL/HSV and set an appropriate level. I think it will make sense to add that stuff to our colorUtils.jsm, in a separate bug and follow up.
Attachment #784649 - Flags: feedback?(shorlander)
Depends on: 902246
Attached image WIP screenshot (obsolete) —
Update, with the icon surround tint color implemented by compositing the color we get from getForegroundAndBackgroundIconColors, set at 0.1 alpha against a white matte.
Attachment #784649 - Attachment is obsolete: true
Attachment #784649 - Flags: feedback?(shorlander)
Attached image WIP screenshot
Just dialed back the tint a little as it was looking too heavy.
Attachment #787191 - Attachment is obsolete: true
(Builds on patch on bug 902246)
Hooks up the favicon-based color tints and new styling for both the topsites/thumbnail tiles and the others. Note that in the comps most of the tiles use a larger favicon. To-date we've just been scaling what we get to 24px. It should be possible to address the larger icon in the .ico if one exists, but I'll file a follow-up for that. Meantime, all but the fallback icon get the 16px treatment.
Attachment #787845 - Flags: review?(rsilveira)
Filed bug 903210 as follow-up for getting us the large favicon size when possible.
Summary: Change - Firefox Start Top Site tile theme changes based on latest comps → Change - Firefox Start tile theme changes based on latest comps
I applied the patch, it's looking great! :) I noticed that the label background for stackoverflow.com is transparent and it doesn't get a border in snapped. Also topsite tiles in snapped are keeping their background colors. I'll review the code tomorrow.
Ah, yeah I'm sure I broke that full->snapped, thumbnail->normal transition now that you mention it. I'll revise the patch and get a new version up in the morning. Not sure what's going on with the label background, I'll take a look.
Comment on attachment 787845 [details] [diff] [review]
Rework tiles favicon treatment + tweaks to meet revised design

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

Looks good so far, I'll wait for issues in comment 9.

::: browser/metro/base/content/bindings/grid.xml
@@ +799,5 @@
>    <binding id="richgrid-item">
>      <content>
>        <html:div anonid="anon-tile" class="tile-content" xbl:inherits="customImage">
>          <html:div class="tile-start-container" xbl:inherits="customImage">
> +          <html:div class="tile-icon-box" anonid="anon-tile-icon-box"><xul:image anonid="anon-tile-icon" xbl:inherits="src=iconURI"/></html:div>

Super nit: Move the anonid as the first attribute.

@@ +877,5 @@
>            if (val) {
>              this.setAttribute("customColor", val);
>              this._contentBox.style.backgroundColor = val;
> +            if("thumbnail" == this.getAttribute("tiletype")) {
> +              this._label.style.backgroundColor = val.replace(/rgb\(([^\)]+)\)/, 'rgba($1, 0.8)');

Setting the style programatically here will make it stick when we change the tiletype in snapped view. We could overwrite this with an !important property.

I think that having a different hbox for the label background and using css opacity will make it simpler.
Ok, got that snapped-view issue and also a couple other tweaks encountered will troubleshooting missing customColors/tintColor for some favicons (which will need to be its own bug once I figured out how to reliably reproduce)
Attachment #787845 - Attachment is obsolete: true
Attachment #787845 - Flags: review?(rsilveira)
Attachment #788417 - Flags: review?(rsilveira)
Comment on attachment 788417 [details] [diff] [review]
Rework tiles favicon treatment + tweaks to meet revised design

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

Looks pretty nice.

::: browser/metro/base/content/bindings/grid.xml
@@ +878,5 @@
>              this.setAttribute("customColor", val);
>              this._contentBox.style.backgroundColor = val;
> +
> +            // overridden in tiles.css for non-thumbnail types
> +            this._label.style.backgroundColor = val.replace(/rgb\(([^\)]+)\)/, 'rgba($1, 0.8)');

You can use opacity if you split the background strip and the text of the label into different elements. This way the text is not transparent as well.
Attachment #788417 - Flags: review?(rsilveira) → review+
Maybe the extra element is the lesser of the 2 evils. That would get the opacity into CSS and do a lot to simplify the binding. Will do.
Second thoughts, the extra element would have to be a decorative sibling to carry the background-color, with the label positioned on top. An inner span or similar for the label text would still have the same opacity issues the s/rgb/rgba/ hack is addressing. I've landed as-is on fx-team, but am happy to pick this and any other issues back up in a follow-up bug. 

https://hg.mozilla.org/integration/fx-team/rev/09b366c13cb0
https://hg.mozilla.org/mozilla-central/rev/09b366c13cb0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
This looks incredible!
What should I test for this?
Flags: needinfo?(jmathies)
(In reply to Samvedana (:Samvedana) from comment #18)
> What should I test for this?

You can compare design comps to actual product if you like, but that's going to be a bit precarious because changes get accepted while development takes place, so comps usually don't 100% reflect the end product.
Flags: needinfo?(jmathies)
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. Metro Firefox start screen matches with https://bug895519.bugzilla.mozilla.org/attachment.cgi?id=787215
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: