835999 - Work - add a topsites grid to the snapped view

Status: RESOLVED FIXED

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

Description

[Allison Naaktgeboren :ally]

including coloring the tiles, similiar to bookmark tiles on main awesome screen. May require refactoring the tile coloring code out of bookmarks.js

mocks attached to bug 801000

Comment 1

[Allison Naaktgeboren :ally]

Attachment: wip 0: grid, sized for snapped, not fullscreen

does this look reasonable for the grid? I'd like to make sure that's solid before I start messing with the colorizing

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 708421 [details] [diff] [review]
wip 0: grid, sized for snapped, not fullscreen

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

This looks like the right approach to me.

::: browser/metro/base/content/TopSites.js
@@ +90,3 @@
>  
>    show: function show() {
> +    this._grid.arrangeItems(9, 1);

When I apply this patch, the grid still has one row instead of 9; I haven't investigated the reasons.

::: browser/metro/theme/browser.css
@@ +690,5 @@
> +
> +/* DEBUG this doesnt appear to be working */
> +#snapped-topsites .richgrid-item-content {
> +  -moz-box-orient: horizontal;
> +}

This part is actually working fine in my build.

Comment 3

[Allison Naaktgeboren :ally]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Comment on attachment 708421 [details] [diff] [review]
> wip 0: grid, sized for snapped, not fullscreen
> 
> Review of attachment 708421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like the right approach to me.
> 
> ::: browser/metro/base/content/TopSites.js
> @@ +90,3 @@
> >  
> >    show: function show() {
> > +    this._grid.arrangeItems(9, 1);
> 
> When I apply this patch, the grid still has one row instead of 9; I haven't
> investigated the reasons.
> 

Did you open the start screen and then snap it immediately? There's a naughty bug (probably in richgrd) that this does not lay out vertically unless you've {already opened, had opened when you snapped} a webpage in snapped view.

Comment 4

[Matt Brubeck (:mbrubeck)]

Marco, this part of the snapped Firefox Start design hasn't been implemented yet and is currently attached to closed story bug 831894.  Should we reopen that bug, or file a change bug or something for this work item?

Marking this dependent on bug 828088 because some of the implementation details might be affected if we make any significant changes there to our tile widgetry.

Comment 5

[Marco Mucci [:MarcoM]]

Thanks Matt.  I'm going to reopen the story and move it 'to be planned'

Can you have a look at it and estimate the point value for full completion.

Thanks.

Comment 6

[Allison Naaktgeboren :ally]

Attachment: wip functional, hacky

- icons currently bleed into box, I don't like the way this looks.
- left debug code in for those who want to play with it
-- and theres fugly stuff from me tinkering my way through the problem
- proper layout requires calling arrangeitems() when the screen is snapped (when the AweSnap screen is inited, there are no item and no dimensions, so the column/row setting code is never reached initally)
- I'm going to see about a cleaner way to do this

Comment 7

[Allison Naaktgeboren :ally]

Attachment: wip, still hacky

questions are in the comments inline. I'm particularly annoyed by whatever anon element is messing up my child selector

Comment 8

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

Comment on attachment 734908 [details] [diff] [review]
wip, still hacky

I poked around in the DOM inspector, ran into presumably the same confusions you did and discovered: 

#snapped-topsites-grid > richgriditem > .richgrid-item-content {
  -moz-box-orient: horizontal;
}

...for reasons I don't understand, according to the inspector, it should be:

#snapped-topsites-grid > .richgrid-grid > richgriditem > .richgrid-item-content

.. but that div.richgrid-grid breaks the selector, even though it is a parent to richgriditems and https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Selectors_and_Scopes says that it should be treated as such for the purpose of selectors. 

Anyhow, looks good. We will eventually need to use a class I think, rather than a id for each grid, as I assume that each grid will need the same rules from the point of view of rendering in the narrow vertical space.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 734908 [details] [diff] [review]
wip, still hacky

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

Looks good!

::: browser/metro/base/content/bindings/grid.xml
@@ +339,5 @@
>            <![CDATA[
> +            let Cu = Components.utils;
> +            //Cu.reportError("inside arrange items. itemcount="+ this.itemCount);
> +            this._rowCount = this.getAttribute("rows");
> +            this._columnCount = this.getAttribute("columns");

Let's move these assignments down to where the values are first used.

::: browser/metro/base/content/browser-ui.js
@@ +533,5 @@
>          this._adjustDOMforViewState();
> +        if (aData == "snapped") {
> +          //Cu.reportError("snapped event handler, expect grid to re-arrange");
> +          //anaaktge replace with less bleeding of implementation
> +          //details

We could clean this up by making TopSitesSnappedView observe "metro_viewstate_changed" notifications and call arrangeItems on its grid.

::: browser/metro/theme/browser.css
@@ +815,5 @@
> +/*layout tile for snapped view */
> +#snapped-topsites .richgrid-item-content {
> +/* TODO figure out what #$%^ing anon element is in the middle
> + *  messing up my child selectors
> + * #snapped-topsites > richgriditem > .richgrid-item-content {*/

It looks like sfoster found a solution/workaround for this one?

Comment 10

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

To clarify:
#snapped-topsites-grid > richgriditem > .richgrid-item-content

.. does work. I just don't understand why it works.

Comment 11

[Allison Naaktgeboren :ally]

Attachment: double observer version part 1/1

post irc discussions & feedback

Comment 12

[Allison Naaktgeboren :ally]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fff11ed20a04

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fff11ed20a04