904417 - Work - NewUI - Adjust spacing and font sizes on start screen and autocomplete popup to more closely match comps.

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Theme, defect)

Description

[Jonathan Wilde [:jwilde]]

Specifically, these comps:
https://bug895519.bugzilla.mozilla.org/attachment.cgi?id=783388
http://people.mozilla.com/~shorlander/files/design-specs-metro/images-design-spec-sheets/Windows8-i03-DesignSpec-%28NavBar%29-i02.jpg

Comment 1

[Jonathan Wilde [:jwilde]]

Attachment: part 1 - fix spacing on not-snap-view

Comment 2

[Jonathan Wilde [:jwilde]]

Attachment: part 2 - ensure spacing is decent on snap view

Comment 3

[Jonathan Wilde [:jwilde]]

Hmmmmm. It looks like the current richgrid implementation may not fully account for the space that it'll need when the user is dragging a tile in the bottom row down to touch select it.

Therefore, at certain resolutions, the richgrid may clip the bottommost tiles and temporarily display a vertical scrollbar on release.

The above patches make it so that we're actually going to hit against this issue at the relatively common HiDPI display resolution of 1920 x 1080. I suspect that we should file a bug on richgrid and make that size calculation change directly (or find a way to elevate dragged tiles so that we don't see them get clipped) rather than trying to patch the clipping/scrollbar behavior in the startUI code indirectly.

Comment 4

[Jonathan Wilde [:jwilde]]

Because of these issues, I'm going to drop review flags for now and file a bug on richgrid (also with preview-triage), but I'd love to see this sort of patch land. It really makes everything look a lot more cohesive.

Comment 5

[Jonathan Wilde [:jwilde]]

Attachment: part 1 - v2 - fix spacing on not-snap-view

Comment 6

[Jonathan Wilde [:jwilde]]

Attachment: part 2 - v2 - fix spacing on snapped view

Comment 7

[Jonathan Wilde [:jwilde]]

Sorry, throwing these up for feedback instead of review. There's still some more work to do with regard to bug 904864 that this hits into.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 793784 [details] [diff] [review]
part 1 - v2 - fix spacing on not-snap-view

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

Looks good so far by my testing. I wondered if the larger left/right margins on the grids/meta-sections needed to be display size/density dependent - great when you've the space for it, but wastes needed space on a smaller display, but actually it seems ok so far.

::: browser/metro/base/content/bindings/grid.xml
@@ +362,5 @@
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>        <property name="_containerSize">
>          <getter><![CDATA[
>              // return the rect that represents our bounding box
>              let containerNode = this.hasAttribute("flex") ? this : this.parentNode;

Amen to that

::: browser/metro/base/content/bindings/urlbar.xml
@@ +490,3 @@
>          <xul:label class="meta-section-title"
>                     value="&autocompleteResultsHeader.label;"/>
> +        <richgrid anonid="results" rows="3" flex="1"

Incoming bitrot from bug 897113

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 793785 [details] [diff] [review]
part 2 - v2 - fix spacing on snapped view

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

Yeah seems to work well.

::: browser/metro/theme/browser.css
@@ +204,5 @@
>  
>  #content-viewport[startpage] browser {
>    padding-bottom: @toolbar_height@;
>  }
>  

Oh I see, this moved to platform.css

Comment 10

[Jonathan Wilde [:jwilde]]

Attachment: part1 - move shared styles out into platform

Comment 11

[Jonathan Wilde [:jwilde]]

Attachment: part2 - initial alignments

Comment 12

[Jonathan Wilde [:jwilde]]

Attachment: part3 - update font sizes

Comment 13

[Jonathan Wilde [:jwilde]]

Attachment: part4 - style autocomplete

Comment 14

[Jonathan Wilde [:jwilde]]

Attachment: part5 - style snapped and portrait views

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 798000 [details] [diff] [review]
part1 - move shared styles out into platform

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

Merged cleanly (!) and looks good. I'm working through the rest of the patches now...

Comment 16

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 798001 [details] [diff] [review]
part2 - initial alignments

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

Looks good, works well

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 798003 [details] [diff] [review]
part3 - update font sizes

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

Yep. Looks right to me.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 798004 [details] [diff] [review]
part4 - style autocomplete

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

Looks good.

::: browser/metro/base/content/bindings/grid.xml
@@ +362,5 @@
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>        <property name="_containerSize">
>          <getter><![CDATA[
>              // return the rect that represents our bounding box
>              let containerNode = this.hasAttribute("flex") ? this : this.parentNode;

Very nice to get rid of this special-case.

::: browser/metro/base/content/bindings/urlbar.xml
@@ +490,3 @@
>          <xul:label class="meta-section-title"
>                     value="&autocompleteResultsHeader.label;"/>
> +        <richgrid anonid="results" rows="3" flex="1"

yes, we use the anonid now so the id isn't necessary.

Comment 19

[Jonathan Wilde [:jwilde]]

Per #windev disucssion, flagging rsilveira to look at the patches, particularly on margins/paddings on different viewstates.

Comment 20

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 798003 [details] [diff] [review]
part3 - update font sizes

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

::: browser/metro/theme/browser.css
@@ -805,4 @@
>    }
>  }
>  
>  #panel-view-switcher {

Not related to your change, but this id is not being used anywhere, it's safe to remove.

::: browser/metro/theme/platform.css
@@ +639,5 @@
>  }
>  
> +.meta-section-title.wide-title {
> +  font-size: @metro_font_xlarge@;
> +  margin-bottom: 34px; /* 40px - @tile_side_margin@; */

You can use calc() to get self-documenting code.

Comment 21

[Rodrigo Silveira [:rsilveira] [:rodms]]

It looks great! Applied and tested it and didn't find any issues. Thanks for taking this.

Comment 22

[Jonathan Wilde [:jwilde]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #20)
> Comment on attachment 798003 [details] [diff] [review]
> part3 - update font sizes
> 
> Review of attachment 798003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/theme/browser.css
> @@ -805,4 @@
> >    }
> >  }
> >  
> >  #panel-view-switcher {
> 
> Not related to your change, but this id is not being used anywhere, it's
> safe to remove.

Will fix before pushing.

> ::: browser/metro/theme/platform.css
> @@ +639,5 @@
> >  }
> >  
> > +.meta-section-title.wide-title {
> > +  font-size: @metro_font_xlarge@;
> > +  margin-bottom: 34px; /* 40px - @tile_side_margin@; */
> 
> You can use calc() to get self-documenting code.

I was wondering about that. Will fix before pushing.

Thanks!

Comment 23

[Jonathan Wilde [:jwilde]]

:( All of these patches got bitrotted. I'll need to switch back to Windows 8.0 before I can really test the unbitrotting, which appears potentially slightly hairy.

Unassigning so others can work on this. Will reassign if I can get the downgrade to work in a reasonable time period and can land this.

Comment 24

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: 904417.patch

Unbitrotted and folded patch. Carrying over sfoster's r+. Will do some verification tests and check it in.

Comment 25

[Jonathan Wilde [:jwilde]]

Thank you! :D

Comment 26

[Rodrigo Silveira [:rsilveira] [:rodms]]

No problem, thanks for all the summer work! :)

https://hg.mozilla.org/integration/fx-team/rev/32f916dd9329

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/32f916dd9329