Closed
Bug 904417
Opened 11 years ago
Closed 11 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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: jwilde, Assigned: rsilveira)
References
Details
(Whiteboard: [preview] feature=work)
Attachments
(1 file, 9 obsolete files)
14.73 KB,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #789389 -
Flags: review?(sfoster)
Reporter | ||
Comment 2•11 years ago
|
||
Attachment #789393 -
Flags: review?(sfoster)
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•11 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•11 years ago
|
Whiteboard: [preview-triage]
Reporter | ||
Comment 4•11 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•11 years ago
|
Attachment #789389 -
Flags: review?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #789393 -
Flags: review?(sfoster)
Updated•11 years ago
|
Whiteboard: [preview-triage] → [preview-triage] feature=work
Updated•11 years ago
|
Whiteboard: [preview-triage] feature=work → [preview] feature=work
Reporter | ||
Comment 5•11 years ago
|
||
Attachment #789389 -
Attachment is obsolete: true
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #789393 -
Attachment is obsolete: true
Reporter | ||
Updated•11 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•11 years ago
|
Attachment #793785 -
Attachment description: part 1 - v2 - fix spacing on snapped view → part 2 - v2 - fix spacing on snapped view
Reporter | ||
Updated•11 years ago
|
Attachment #793784 -
Flags: review?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #793785 -
Flags: review?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #793784 -
Flags: review?(sfoster) → feedback?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #793784 -
Flags: feedback?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #793784 -
Flags: feedback?(sfoster)
Reporter | ||
Updated•11 years ago
|
Attachment #793785 -
Flags: review?(sfoster) → feedback?(sfoster)
Reporter | ||
Comment 7•11 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 8•11 years ago
|
||
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 9•11 years ago
|
||
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•11 years ago
|
Attachment #793784 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #793785 -
Attachment is obsolete: true
Reporter | ||
Comment 10•11 years ago
|
||
Attachment #798000 -
Flags: review?(sfoster)
Reporter | ||
Comment 11•11 years ago
|
||
Attachment #798001 -
Flags: review?(sfoster)
Reporter | ||
Comment 12•11 years ago
|
||
Attachment #798003 -
Flags: review?(sfoster)
Reporter | ||
Comment 13•11 years ago
|
||
Attachment #798004 -
Flags: review?(sfoster)
Reporter | ||
Comment 14•11 years ago
|
||
Attachment #798006 -
Flags: review?(sfoster)
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
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 17•11 years ago
|
||
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 18•11 years ago
|
||
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•11 years ago
|
Attachment #798006 -
Flags: review?(sfoster) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #798000 -
Flags: feedback?(rsilveira)
Reporter | ||
Updated•11 years ago
|
Attachment #798001 -
Flags: feedback?(rsilveira)
Reporter | ||
Updated•11 years ago
|
Attachment #798003 -
Flags: feedback?(rsilveira)
Reporter | ||
Updated•11 years ago
|
Attachment #798004 -
Flags: feedback?(rsilveira)
Reporter | ||
Updated•11 years ago
|
Attachment #798006 -
Flags: feedback?(rsilveira)
Reporter | ||
Comment 19•11 years ago
|
||
Per #windev disucssion, flagging rsilveira to look at the patches, particularly on margins/paddings on different viewstates.
Assignee | ||
Updated•11 years ago
|
Attachment #798000 -
Flags: feedback?(rsilveira) → feedback+
Assignee | ||
Updated•11 years ago
|
Attachment #798001 -
Flags: feedback?(rsilveira) → feedback+
Assignee | ||
Comment 20•11 years ago
|
||
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•11 years ago
|
Attachment #798004 -
Flags: feedback?(rsilveira) → feedback+
Assignee | ||
Updated•11 years ago
|
Attachment #798006 -
Flags: feedback?(rsilveira) → feedback+
Assignee | ||
Comment 21•11 years ago
|
||
It looks great! Applied and tested it and didn't find any issues. Thanks for taking this.
Reporter | ||
Comment 22•11 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•11 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
Assignee | ||
Comment 24•11 years ago
|
||
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•11 years ago
|
||
Thank you! :D
Assignee | ||
Comment 26•11 years ago
|
||
No problem, thanks for all the summer work! :)
https://hg.mozilla.org/integration/fx-team/rev/32f916dd9329
Comment 27•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•