828088 - Work - Tiles (on Firefox Start and the autocomplete screen) should be organized vertically (in columns)

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Discussed in Win8 sync-up mtg on Jan 7.
We should make the tiles listed in a vertical sequence.

http://cl.ly/image/2e342d0T1P0v/o

Comment 1

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

I'll use this ticket for the initial integration of Frank's tile group column-fill prototype: https://bug831915.bugzilla.mozilla.org/attachment.cgi?id=707266

Comment 2

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

Attachment: WIP extending prototype for start grid tiles

I've got a couple of HTML/JS/CSS mockups underway to bring the columns goodness from fryn's prototype into our richgrid binding. I'm going to keep to HTML for the item's anonymous content, but there's some detail needed to be added to support the different icons and states we need. 

This WIP is just a couple of steps away from the original prototype. Not really of interest yet; mostly just a snapshot for my reference. The labels are inlined not CSS generated content. Somewhere in there I broke the "bend" transition...

Comment 3

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

Attachment: WIP extending prototype for start grid tiles

Updated mockup. I found and partly-fixed the bend on click/touch behavior. It works, but the preventDefault() when handling a touchstart event doesn't prevent the mousedown event being fired, so the handler runs twice. 

I made a stab at the colored bar on the tiles, as shown in https://bug836387.bugzilla.mozilla.org/attachment.cgi?id=726419

Next: 
* I need to work up the tile variations to make sure this markup will be adequate for all cases. 
* New arrangeItems method, which sets the right height for desired number of rows, and truncates the collection to fit into the given columns
* New tilegrid binding that can live alongside richgrid during integration and while we get the kinks out one instance at a time
* CSS will need more work to live alongside the rules already present in the (chrome) document

Comment 4

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

Attachment: WIP updated richgrid binding for column-based tile grid

Snapshot of progress on the new tile grid. I'm mid-stream on tracking down an issue where a ~60px gap at the bottom of the topsites grid is throwing off the column flow and alignment of tiles.

Comment 5

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

Attachment: 751711: WIP updated richgrid binding for column-based tile grid

Updated snapshot:
* simplified rearrange logic (no rescheduling)
* the bound element may flex, but the grid itself does not - it expands horizontally to create as many columns as necessary/directed in the available height
* fixed background image/colors
* removed debug/measuring gunk

still to do: 
* hook up collapsed transition
* run through unit tests to find/fix any api changes
* troubleshoot column flow for bookmarks grid (it says its creating 7 rows but wrapping after 3) 
* fix grid height to exclude the appbar (?)

Comment 6

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

Attachment: WIP updated richgrid binding for column-based tile grid

Pretty much functional, I added a arrangeItems test and still need to track down a test failure around grid overflow. 

In browser/metro/base/content/browser-ui.js you'll see I defer the section inits, this is because the contentareaobserver which was init beforehand is also async. 

The grid binding has new markup (css is all moved out to tiles.css), an _isBound guard added to many methods and the new sizing/rearrange logic. All the scheduleRearrange stuff is done (yay, turns out this was firing for all the hidden grids, good riddance). 

Tile animate-in and touch transitions TODO still.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 752228 [details] [diff] [review]
WIP updated richgrid binding for column-based tile grid

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

Looks good at first glance.  I didn't do a 100% thorough review of everything.

::: browser/metro/base/content/bindings/grid.xml
@@ +7,5 @@
> +#define tile_spacing_px 12
> +#define tile_unit_width_px 119
> +#define tile_unit_height_px 74
> +#define tile_double_width_px 250
> +#define tile_double_height_px 160

Can you #include the "theme/defines.inc" file here instead?

@@ +351,4 @@
>        <field name="_columnCount">0</field>
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>  
> +      <method name="_getContainerBox">

naming nit: Can you call this _getContainerRect or _getContainerSize?

("box" tends to mean an element in XUL, while "rect" means a set of dimensions)

::: browser/metro/base/content/browser.css
@@ +105,5 @@
>    -moz-binding: url("chrome://browser/content/bindings/grid.xml#richgrid");
>  }
>  
> +richgriditem,
> +div.tile {

I'm not sure why we support both <richgriditem> and <div class="tile"> -- are we ultimately going to standard on just one of these?

::: browser/metro/base/jar.mn
@@ +16,4 @@
>    content/bindings/tabs.xml                    (content/bindings/tabs.xml)
>    content/bindings/toggleswitch.xml            (content/bindings/toggleswitch.xml)
>    content/bindings/browser.xml                 (content/bindings/browser.xml)
> +*  content/bindings/browser.js                  (content/bindings/browser.js)

nit: alignment :)

::: browser/metro/theme/defines.inc
@@ +39,5 @@
> +%define tile_spacing_px 12
> +%define tile_unit_width_px 119
> +%define tile_unit_height_px 74
> +%define tile_double_width_px 250
> +%define tile_double_height_px 160

We could also use calc() for the "double" versions, if that is any more simple/robust.

Comment 8

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

Attachment: WIP updated richgrid binding for column-based tile grid

I've reworked a few things here - the item sizes are "parsed" via the CSSOM out of the tile.css stylesheet - that allows a single source of truth (the defines.inc) and removes the need for the item-measurement race condition we suffered before. 

In arrangeItems, the number of rows that will fit is calculated, which gives us the number of columns and width necessary for the grid. This is better than it was, but it still relies on the container height measurement (which is appropriate I think)

For the snapped view, I'm using a media query to override some rules and use the more compact grid row height and item style. That looks to be doing the right thing, but in immersive+snapped the items are too wide currently- I've not tracked down why yet. 

For autocomplete, whatever the source of the various issues we've had here is still in effect. With the new grid the effect is mis-measurment of the container, so we get 1 column instead of 2. I've got a seperate patch that mitigates it somewhat but the core issue remains. 

I think I'd like to get more eyes on it. I believe as-is, this patch is a net improvement, but there will be follow-on bugs to address these and probably other regressions. Try it out, let me know what you think

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 758856 [details] [diff] [review]
WIP updated richgrid binding for column-based tile grid

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

::: browser/metro/base/content/bindings/grid.xml
@@ +346,4 @@
>        <field name="_columnCount">0</field>
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>  
> +      <property name="containerNode">

Please make this "private" (add an _underscore), just to make it clear this is an implementation detail.

Perhaps it should be named something like _containerSizeParent to make it clear why we care about this element.

@@ +364,5 @@
> +          dump("richgrid, containerNode setter\n");
> +          if (val) {
> +            this.setAttribute("container", val);
> +          } else {
> +            this.removeAttribute("container");

Is this setter needed?  If not, feel free to make this a read-only property.  (Callers can always use "setAttribute".)

In fact, if it turns out we don't use the container="id" attribute, we can simplify the getter too.

@@ +401,5 @@
> +
> +            if (!this._tileSizes) {
> +              // grab tile/item dimensions
> +              this._tileSizes = this._getTileSizes();
> +            }

I'd put this field-caching stuff into _getTileSizes, in case it's called from other places in the future.

::: browser/metro/base/content/browser-ui.js
@@ +1411,5 @@
>      Elements.startUI.addEventListener("click", this, false);
>  
> +    let sections = this.sections;
> +    // section init deferred as ContentAreaObserver's first update is also deferred
> +     setTimeout(function(){

Can we listen to an event or notification from CAO instead of using a timeout here?

Comment 10

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

Attachment: Rework richgrid and richgrid items for (new design and) down-then-across grids

This patch is the first of a series to implement the new grid designs:
* CSS broken out into its own "tiles.css" file
* Key grid/tile dimensions added to theme/defines.inc
* Item size acquired via CSSOM avoids render-then-measure race
* createItemElement richgrid method made public
* Rework item markup for the columns-style grid, also in anticipation of bug 835984 and bug 835986
* New Tile look&feel came along for the ride, ref: http://people.mozilla.com/~shorlander/files/design-specs-metro/metro-designSpecs-splitView-tiles.html
* Tests updated and pass

Comment 11

[Frank Yan (:fryn)]

Comment on attachment 765458 [details] [diff] [review]
Rework richgrid and richgrid items for (new design and) down-then-across grids

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

I'm definitely going to give this a full review, but I think jwilde's feedback here will be highly useful too, especially his autocomplete panel work is related.

Comment 12

[Frank Yan (:fryn)]

Comment on attachment 765458 [details] [diff] [review]
Rework richgrid and richgrid items for (new design and) down-then-across grids

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

r+ with the "variable" values changed to include their units.

I found the tiles to be more janky on my Surface Pro with this patch than without patch.
Not sure why, but we can debug after landing it.

Any feedback that jwilde provides can be addressed in followups.

::: browser/metro/base/content/bindings/grid.xml
@@ +573,5 @@
> +                  let sizes = typeSizes[type] = {};
> +                  typeSelectors[type] = null;
> +                  delete typeSelectors[type];
> +                  sizes.width =  parseInt(rule.style.getPropertyValue("width"), 10);
> +                  sizes.height = parseInt(rule.style.getPropertyValue("height"), 10);

Nit: the second argument (10) of parseInt is now unnecessary, since parseInt now uses base 10 even when parsing stuff like "013px".

::: browser/metro/theme/defines.inc
@@ +41,2 @@
>  %define tile_border_color #dbdcde
> +%define tile_spacing_px 12

I think it'd be better if we kept the unit as part of each of these "variables", since it doesn't make sense to ever append one kind of unit to it in one block and another unit in another block.

Comment 13

[Jonathan Wilde [:jwilde]]

Comment on attachment 765458 [details] [diff] [review]
Rework richgrid and richgrid items for (new design and) down-then-across grids

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

Concur with Frank's landing comments.

::: browser/metro/base/content/bindings/grid.xml
@@ +20,5 @@
>  
>      <implementation implements="nsIDOMXULSelectControlElement">
>        <property name="_grid" readonly="true" onget="return document.getAnonymousElementByAttribute(this, 'anonid', 'grid');"/>
> +
> +      <property name="isBound" readonly="true" onget="return !!this._grid"/>

This sort of information is extremely helpful all over the place. I wonder if there's some way we could get this moved lower in the stack...

Unlikely, given that contentgenerated got a WONTFIX, even though it's in the XBL spec submitted by Mozilla to the W3C, but still.

@@ +502,5 @@
>        <field name="_xslideHandler"/>
>        <constructor>
>          <![CDATA[
>            if (this.controller && this.controller.gridBoundCallback != undefined)
>              this.controller.gridBoundCallback();

Thing that's been bugging me lately: we currently have two ways to get a callback when grids get bound. There's gridBoundCallback and then there's that contentgenerated event below.

Can we simplify the codebase and just focus on using one?

I think we should land this without making changes there, but something that I've been thinking a lot about with the autocomplete.xml work.

::: browser/metro/theme/browser.css
@@ +732,5 @@
> +  background-color: rgba(0,0,255, 0.5);
> +  padding-bottom: @toolbar_height@;
> +  -moz-box-sizing: border-box;
> +}
> +*/

Nit: this should probably get dropped, especially since upcoming autocomplete work will discontinue using viewable-height and meta-section entirely.

::: browser/metro/theme/defines.inc
@@ +41,2 @@
>  %define tile_border_color #dbdcde
> +%define tile_spacing_px 12

I was just about to write a comment about how we should move the unit into the variable, and then I noticed fryn's comment.

There were discussions very early in the life of metro about the potential need to move away from pixel values in places to avoid HiDPI-related usability issues later on. I'd probably be a good idea to make sure we're ready to deal with unit conversions later in the future, even though it'll complicate the CSSOM unit snarfing.

Comment 14

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

I made the suggested change to put the unit in the preprocessor variables, and landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2af3c33bfb29

Its now e.g. %define grid_column_width 131px. As I parseInt on the property value in the js it actually makes no difference. As jwilde points out, if we use non-px values in the future we'll need to look at this again. 

Also, I didn't really call it out above, but for snapped view grids I'm using a media query. It should mean we're more future proofed for windows 8.1's resizable snapped panes - as its just a threshold beyond which we go to a single column, vertical grid layout.

Comment 15

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9b867a42663c for giving metro-chrome a rather odd variety of failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=24416425&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24410727&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24412757&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24413654&tree=Mozilla-Inbound
etc

Comment 16

[Jim Mathies [:jimm]]

(In reply to Phil Ringnalda (:philor) from comment #15)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/9b867a42663c for
> giving metro-chrome a rather odd variety of failures:
> 
https://tbpl.mozilla.org/php/getParsedLog.php?id=24416425&tree=Mozilla-Inbound

single failure in downloads.

https://tbpl.mozilla.org/php/getParsedLog.php?id=24410727&tree=Mozilla-Inbound

These were the result of bug 885541, not this landing.

Comment 17

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

Attachment: Rework richgrid and richgrid items for (new design and) down-then-across grids

Due to some test failures I went back and hardened a little, adding new tests for clearAll and the empty-grid case and also re-working/clarifying the handling of rows and columns attributes in arrangeItems. 
I also added a isArranging property for (mostly testing) convenience.

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 766167 [details] [diff] [review]
Rework richgrid and richgrid items for (new design and) down-then-across grids

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

::: browser/metro/base/tests/mochitest/head.js
@@ +777,1 @@
>  function stubMethod(aObj, aMethod) {

There is quite a lot of duplicated code between spyOnMethod and stubMethod, but this is in tests/, so I won't worry about it.

Comment 19

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

I am getting intermittent orange when I push to try (bug 885930) but can't see the connection in the code to this patch. So, I've landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9cf2a04252

I suspect the download issue may be revealed rather than caused by this patch. I propose to follow up on that ticket.

Comment 20

[Ed Morley [:emorley]]

(In reply to Sam Foster [:sfoster] from comment #19)
> I am getting intermittent orange when I push to try (bug 885930) but can't
> see the connection in the code to this patch. So, I've landed on inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9cf2a04252
> 
> I suspect the download issue may be revealed rather than caused by this
> patch. I propose to follow up on that ticket.

Bug 885930 is now occurring on ~80%+ of metro test runs, so backing this out for now:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d5fff0c9ebbe

Comment 22

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

I added a bunch of checks and guards around the downloads code and uses of _getLocalFile. I did spot some abiquity in whether a nsIURI or url string was getting assigned to the bound xul item node - I decided on string. Push to try and ran tests 10 times. I got 8 greens, 2 restarts (bug 884247). 

Carried fryn's r+, landed on inbound:  https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9d40dc058a

Comment 23

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

Try run was at: https://tbpl.mozilla.org/?tree=Try&rev=ccf448a613da

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a9d40dc058a