Closed Bug 828088 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ywang, Assigned: sfoster)

References

Details

(Whiteboard: feature=work)

Attachments

(2 files, 6 obsolete files)

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
Product: Firefox → Firefox for Metro
Product: Firefox for Metro → Firefox
Whiteboard: [metro-mvp?]
Product: Firefox → Firefox for Metro
Whiteboard: [metro-mvp?] → [metro-mvp]
Blocks: 831915
Whiteboard: [metro-mvp] → [metro-mvp] feature=work
Summary: Bookmarks, History, Remote tabs on start page should be organized vertically → Work - Bookmarks, History, Remote tabs on start page should be organized vertically
Whiteboard: [metro-mvp] feature=work → feature=work
Assignee: nobody → fyan
Status: NEW → ASSIGNED
Summary: Work - Bookmarks, History, Remote tabs on start page should be organized vertically → Work - Tiles (on Firefox Start and the autocomplete screen) should be organized vertically (in columns)
Blocks: 835999
Component: General → Firefox Start
Assignee: fyan → nobody
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
Assignee: nobody → sfoster
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...
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
Attachment #746500 - Attachment is obsolete: true
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.
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 (?)
Attachment #751711 - Attachment is obsolete: true
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.
Attachment #752087 - Attachment is obsolete: true
Attachment #752228 - Flags: feedback?(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.
Attachment #752228 - Flags: feedback?(mbrubeck) → feedback+
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
Attachment #752228 - Attachment is obsolete: true
Attachment #758856 - Flags: feedback?(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?
Attachment #758856 - Flags: feedback?(mbrubeck) → feedback+
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
Attachment #758856 - Attachment is obsolete: true
Attachment #765458 - Flags: review?(fyan)
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.
Attachment #765458 - Flags: feedback?(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]:
-----------------------------------------------------------------

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.
Attachment #765458 - Flags: review?(fyan) → review+
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.
Attachment #765458 - Flags: feedback?(jwilde) → feedback+
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.
(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.
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.
Attachment #765458 - Attachment is obsolete: true
Attachment #766167 - Flags: review?(fyan)
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.
Attachment #766167 - Flags: review?(fyan) → review+
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.
Depends on: 885930
(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
No longer depends on: 885930
Duplicate of this bug: 885930
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
https://hg.mozilla.org/mozilla-central/rev/6a9d40dc058a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 888981
No longer depends on: 888981
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.