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

RESOLVED FIXED in Firefox 26

Status

Firefox for Metro
Theme
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwilde, Assigned: rsilveira)

Tracking

unspecified
Firefox 26
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview] feature=work)

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Comment 1

5 years ago
Created attachment 789389 [details] [diff] [review]
part 1 - fix spacing on not-snap-view
Attachment #789389 - Flags: review?(sfoster)
(Reporter)

Comment 2

5 years ago
Created attachment 789393 [details] [diff] [review]
part 2 - ensure spacing is decent on snap view
Attachment #789393 - Flags: review?(sfoster)
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

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

Updated

5 years ago
Whiteboard: [preview-triage]
(Reporter)

Comment 4

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

Updated

5 years ago
Attachment #789389 - Flags: review?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #789393 - Flags: review?(sfoster)
(Reporter)

Updated

5 years ago
Blocks: 895520
(Reporter)

Updated

5 years ago
Depends on: 904864

Updated

5 years ago
Whiteboard: [preview-triage] → [preview-triage] feature=work

Updated

5 years ago
Whiteboard: [preview-triage] feature=work → [preview] feature=work
(Reporter)

Comment 5

5 years ago
Created attachment 793784 [details] [diff] [review]
part 1 - v2 - fix spacing on not-snap-view
Attachment #789389 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
Created attachment 793785 [details] [diff] [review]
part 2 - v2 - fix spacing on snapped view
Attachment #789393 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #793784 - Attachment description: part 1 - fix spacing on not-snap-view → part 1 - v2 - fix spacing on not-snap-view
(Reporter)

Updated

5 years ago
Attachment #793785 - Attachment description: part 1 - v2 - fix spacing on snapped view → part 2 - v2 - fix spacing on snapped view
(Reporter)

Updated

5 years ago
Attachment #793784 - Flags: review?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #793785 - Flags: review?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #793784 - Flags: review?(sfoster) → feedback?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #793784 - Flags: feedback?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #793784 - Flags: feedback?(sfoster)
(Reporter)

Updated

5 years ago
Attachment #793785 - Flags: review?(sfoster) → feedback?(sfoster)
(Reporter)

Comment 7

5 years ago
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 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
Attachment #793784 - Flags: feedback?(sfoster) → feedback+
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
Attachment #793785 - Flags: feedback?(sfoster) → feedback+
(Reporter)

Updated

5 years ago
Attachment #793784 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #793785 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
Created attachment 798000 [details] [diff] [review]
part1 - move shared styles out into platform
Attachment #798000 - Flags: review?(sfoster)
(Reporter)

Comment 11

5 years ago
Created attachment 798001 [details] [diff] [review]
part2 - initial alignments
Attachment #798001 - Flags: review?(sfoster)
(Reporter)

Comment 12

5 years ago
Created attachment 798003 [details] [diff] [review]
part3 - update font sizes
Attachment #798003 - Flags: review?(sfoster)
(Reporter)

Comment 13

5 years ago
Created attachment 798004 [details] [diff] [review]
part4 - style autocomplete
Attachment #798004 - Flags: review?(sfoster)
(Reporter)

Comment 14

5 years ago
Created attachment 798006 [details] [diff] [review]
part5 - style snapped and portrait views
Attachment #798006 - Flags: review?(sfoster)
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...
Attachment #798000 - Flags: review?(sfoster) → review+
Comment on attachment 798001 [details] [diff] [review]
part2 - initial alignments

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

Looks good, works well
Attachment #798001 - Flags: review?(sfoster) → review+
Comment on attachment 798003 [details] [diff] [review]
part3 - update font sizes

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

Yep. Looks right to me.
Attachment #798003 - Flags: review?(sfoster) → review+
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.
Attachment #798004 - Flags: review?(sfoster) → review+

Updated

5 years ago
Attachment #798006 - Flags: review?(sfoster) → review+
(Reporter)

Updated

5 years ago
Attachment #798000 - Flags: feedback?(rsilveira)
(Reporter)

Updated

5 years ago
Attachment #798001 - Flags: feedback?(rsilveira)
(Reporter)

Updated

5 years ago
Attachment #798003 - Flags: feedback?(rsilveira)
(Reporter)

Updated

5 years ago
Attachment #798004 - Flags: feedback?(rsilveira)
(Reporter)

Updated

5 years ago
Attachment #798006 - Flags: feedback?(rsilveira)
(Reporter)

Comment 19

5 years ago
Per #windev disucssion, flagging rsilveira to look at the patches, particularly on margins/paddings on different viewstates.
(Assignee)

Updated

5 years ago
Attachment #798000 - Flags: feedback?(rsilveira) → feedback+
(Assignee)

Updated

5 years ago
Attachment #798001 - Flags: feedback?(rsilveira) → feedback+
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.
Attachment #798003 - Flags: feedback?(rsilveira) → feedback+
(Assignee)

Updated

5 years ago
Attachment #798004 - Flags: feedback?(rsilveira) → feedback+
(Assignee)

Updated

5 years ago
Attachment #798006 - Flags: feedback?(rsilveira) → feedback+
It looks great! Applied and tested it and didn't find any issues. Thanks for taking this.
(Reporter)

Comment 22

5 years ago
(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!
(Reporter)

Comment 23

5 years ago
:( 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.
Assignee: jwilde → nobody
Status: ASSIGNED → NEW
Created attachment 803467 [details] [diff] [review]
904417.patch

Unbitrotted and folded patch. Carrying over sfoster's r+. Will do some verification tests and check it in.
Assignee: nobody → rsilveira
Attachment #798000 - Attachment is obsolete: true
Attachment #798001 - Attachment is obsolete: true
Attachment #798003 - Attachment is obsolete: true
Attachment #798004 - Attachment is obsolete: true
Attachment #798006 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Comment 25

5 years ago
Thank you! :D
No problem, thanks for all the summer work! :)

https://hg.mozilla.org/integration/fx-team/rev/32f916dd9329
https://hg.mozilla.org/mozilla-central/rev/32f916dd9329
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.