Closed Bug 904417 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Metro Graveyard :: Theme, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jwilde, Assigned: rsilveira)

References

Details

(Whiteboard: [preview] feature=work)

Attachments

(1 file, 9 obsolete files)

Attachment #789389 - Flags: review?(sfoster)
Attachment #789393 - Flags: review?(sfoster)
Status: NEW → ASSIGNED
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.
Whiteboard: [preview-triage]
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.
Attachment #789389 - Flags: review?(sfoster)
Attachment #789393 - Flags: review?(sfoster)
Blocks: 895520
Depends on: 904864
Whiteboard: [preview-triage] → [preview-triage] feature=work
Whiteboard: [preview-triage] feature=work → [preview] feature=work
Attachment #789389 - Attachment is obsolete: true
Attachment #789393 - Attachment is obsolete: true
Attachment #793784 - Attachment description: part 1 - fix spacing on not-snap-view → part 1 - v2 - fix spacing on not-snap-view
Attachment #793785 - Attachment description: part 1 - v2 - fix spacing on snapped view → part 2 - v2 - fix spacing on snapped view
Attachment #793784 - Flags: review?(sfoster)
Attachment #793785 - Flags: review?(sfoster)
Attachment #793784 - Flags: review?(sfoster) → feedback?(sfoster)
Attachment #793784 - Flags: feedback?(sfoster)
Attachment #793784 - Flags: feedback?(sfoster)
Attachment #793785 - Flags: review?(sfoster) → feedback?(sfoster)
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+
Attachment #793784 - Attachment is obsolete: true
Attachment #793785 - Attachment is obsolete: true
Attachment #798000 - Flags: review?(sfoster)
Attached patch part2 - initial alignments (obsolete) — Splinter Review
Attachment #798001 - Flags: review?(sfoster)
Attached patch part3 - update font sizes (obsolete) — Splinter Review
Attachment #798003 - Flags: review?(sfoster)
Attached patch part4 - style autocomplete (obsolete) — Splinter Review
Attachment #798004 - Flags: review?(sfoster)
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+
Attachment #798006 - Flags: review?(sfoster) → review+
Attachment #798000 - Flags: feedback?(rsilveira)
Attachment #798001 - Flags: feedback?(rsilveira)
Attachment #798003 - Flags: feedback?(rsilveira)
Attachment #798004 - Flags: feedback?(rsilveira)
Attachment #798006 - Flags: feedback?(rsilveira)
Per #windev disucssion, flagging rsilveira to look at the patches, particularly on margins/paddings on different viewstates.
Attachment #798000 - Flags: feedback?(rsilveira) → feedback+
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+
Attachment #798004 - Flags: feedback?(rsilveira) → feedback+
Attachment #798006 - Flags: feedback?(rsilveira) → feedback+
It looks great! Applied and tested it and didn't find any issues. Thanks for taking this.
(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!
:( 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
Attached patch 904417.patchSplinter Review
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
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
Closed: 7 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.