Sam Foster [:sfoster] (he/him) Reporter
	Description • 11 years ago

It has turned out that we duplicate a lot of logic and content across the "normal" start-ui and the snapped start-ui. It should be possible to removed the entire #snapped-start structure and adapt #start and its views for reuse in the snapped viewstate

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 1 • 11 years ago

Further notes on implementation and justification: 

* Views can observe viewstate changes and make any necessary changes. We will not attempt to do this only in CSS
* No need for distinct grids, the same content can be presented and styled for the available space and orientation
* Fewer queries, fewer bindings, smaller DOM should mean faster startup and better user experience
* TopSites will need to switch its grid items to the normal/non-thumbnail variant (and back again) when viewstate changes
* PanelUI largely but not completely redundant with this stage (and in v1). The console is still in use, but is accessible from the hamburger appbar menu so the switcher (dropdown from panel headings) can go away
* Treating snapped as a named threshold, but making the start-ui responsive lays important groundwork for supporting windows 8.1 and its split panels

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 2 • 11 years ago

Removing snapped view and snapped topsites view. Part 2 will collapse/expand items in snapped view and adjust CSS if needed.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 3 • 11 years ago

Comment on attachment 778704 [details] [diff] [review]
Part 1 of 2 - Remove snapped view.

Ally's patch simplified a couple of things on this patch. I'll post a newer fresher patch later.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 4 • 11 years ago

Incoming bitrot from: https://bugzilla.mozilla.org/show_bug.cgi?id=892632 (should be minor)

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 5 • 11 years ago

Got rid of useThumbs in TopSites and I'm removing the tiletype when snapped to let the CSS do it's thing. Now nothing is rearranging programmatically when snapped, so metro_viewstate_dom_snapped is not needed.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 6 • 11 years ago

Added collapse/expand behavior to snapped sections.

Would be nice to get them to transition open/close.

Removed unnecessary container #start.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 7 • 11 years ago

Comment on attachment 779069 [details] [diff] [review]
Part 2 or 2 - Expand collapse snapped sections

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

Looks good. There are still issues with snapped view interactions, but I think they are not introduced here, and adequately covered by other bugs.

::: browser/metro/theme/browser.css
@@ +765,5 @@
>  
>  #start-container[viewstate="snapped"] .meta-section {
>    margin: 0px;
>    min-width: @grid_double_column_width@;
> +  -moz-box-flex: 1;

This looks like a good base to build off for the snapped-view. Lets put it in front of Yuan/Shorlander and get sign-off/follow ups to refine how this is presented and the interaction details.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 8 • 11 years ago

Comment on attachment 779060 [details] [diff] [review]
Part 1 of 2 - Remove snapped view.

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

Looks great, thanks for taking this on. Looks like we got to lose a lot of code and a layer or 2 of complexity. Here's hoping it stays that way as we address the details.

::: browser/metro/base/content/TopSites.js
@@ +307,5 @@
>  
>    updateTile: function(aTileNode, aSite, aArrangeGrid) {
>      this._updateFavicon(aTileNode, Util.makeURI(aSite.url));
>  
> +    Task.spawn(function() {

Can this get called in snapped view? As we don't use the thumbnail there it seems a waste to go looking for it on the filesystem

@@ +378,5 @@
>      switch(aTopic) {
>        case "Metro:RefreshTopsiteThumbnail":
>          this.forceReloadOfThumbnail(aState);
>          break;
> +      case "metro_viewstate_changed":

I think this works today as we simply override columns-count in snapped view, so item dimensions have no effect. We will probably need to arrangeItems here if what and how we display items changes between viewstates though.

::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +29,3 @@
>        The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' -->
>  <!ENTITY snappedRemoteTabsHeader.label    "Remote Tabs >">
> +<!ENTITY snappedBookmarksHeader.label     "Bookmarks >">

We still need to get those > out of there before we lock strings

::: browser/metro/theme/browser.css
@@ +743,5 @@
>    /* allot space for 3 tile columns for the topsites grid */
>    min-width: calc(3 * @grid_double_column_width@);
>  }
>  
> +#start-scrollbox {

This gets us part of bug 892041. I'll leave further comments on next steps there.

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 9 • 11 years ago

(In reply to Sam Foster [:sfoster] from comment #7)
> Comment on attachment 779069 [details] [diff] [review]
> Part 2 or 2 - Expand collapse snapped sections
> 
> Review of attachment 779069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. There are still issues with snapped view interactions, but I
> think they are not introduced here, and adequately covered by other bugs.
> 

Thanks for the review! I filed bug 896766 for one of the issues we discussed on IRC.

> ::: browser/metro/theme/browser.css
> @@ +765,5 @@
> >  
> >  #start-container[viewstate="snapped"] .meta-section {
> >    margin: 0px;
> >    min-width: @grid_double_column_width@;
> > +  -moz-box-flex: 1;
> 
> This looks like a good base to build off for the snapped-view. Lets put it
> in front of Yuan/Shorlander and get sign-off/follow ups to refine how this
> is presented and the interaction details.

Absolutely, I'm sure there will be plenty to work on after they look at it!

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 10 • 11 years ago

(In reply to Sam Foster [:sfoster] from comment #8)
> Comment on attachment 779060 [details] [diff] [review]
> Part 1 of 2 - Remove snapped view.
> 
> Review of attachment 779060 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, thanks for taking this on. Looks like we got to lose a lot of
> code and a layer or 2 of complexity. Here's hoping it stays that way as we
> address the details.
> 
> ::: browser/metro/base/content/TopSites.js
> @@ +307,5 @@
> >  
> >    updateTile: function(aTileNode, aSite, aArrangeGrid) {
> >      this._updateFavicon(aTileNode, Util.makeURI(aSite.url));
> >  
> > +    Task.spawn(function() {
> 
> Can this get called in snapped view? As we don't use the thumbnail there it
> seems a waste to go looking for it on the filesystem
> 

If I understand correctly, since we're not capturing new thumbnails in snapped mode[1] and we're not reacting to nsINavHistoryObserver events this shouldn't be called unnecessarily.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#877

> @@ +378,5 @@
> >      switch(aTopic) {
> >        case "Metro:RefreshTopsiteThumbnail":
> >          this.forceReloadOfThumbnail(aState);
> >          break;
> > +      case "metro_viewstate_changed":
> 
> I think this works today as we simply override columns-count in snapped
> view, so item dimensions have no effect. We will probably need to
> arrangeItems here if what and how we display items changes between
> viewstates though.
> 

Sure! The media queries we have seem to be handling snapped incredibly well[2].

[2] https://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/tiles.css#246

> ::: browser/metro/locales/en-US/chrome/browser.dtd
> @@ +29,3 @@
> >        The '>' character is not part of the name, but is an indicator of more content. Please do not localize the '>' -->
> >  <!ENTITY snappedRemoteTabsHeader.label    "Remote Tabs >">
> > +<!ENTITY snappedBookmarksHeader.label     "Bookmarks >">
> 
> We still need to get those > out of there before we lock strings
> 

Yeah, opened bug 896757 for that.

> ::: browser/metro/theme/browser.css
> @@ +743,5 @@
> >    /* allot space for 3 tile columns for the topsites grid */
> >    min-width: calc(3 * @grid_double_column_width@);
> >  }
> >  
> > +#start-scrollbox {
> 
> This gets us part of bug 892041. I'll leave further comments on next steps
> there.

Great, thanks!

	Rodrigo Silveira [:rsilveira] [:rodms] Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d444d04fa6c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2da1603c04

	Ed Morley [:emorley]
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/d444d04fa6c3
https://hg.mozilla.org/mozilla-central/rev/4b2da1603c04