Yuan Wang(:Yuan) – Mobile Firefox Design Lead Reporter
	Description • 11 years ago

Steps to reproduce:
1. In full view or half view, open a site from Firefox Start Page
2. Snap Metro FX into snap view
3. Hit back button

Expected result:
Show Firefox Start Page in the snap view version

Actual result:
Firefox Start Page was shown as the full view version.

	Jim Mathies [:jimm]
	Comment 1 • 11 years ago

This should have fixed the problem. The debug out confirms that on load the broadcaster is set right. Yet, a new tab displays top site tiles. I don't quite understand what's going on here.

	Jim Mathies [:jimm]
	Comment 2 • 11 years ago

Moving the _adjustDOMforViewState call below all the show calls doesn't help either.

	Jim Mathies [:jimm]
	Comment 3 • 11 years ago

Adding a red debug border around start-container shows the viewstate css is taking effect. But the top site thumbs aren't going away.

	Jim Mathies [:jimm]
	Comment 4 • 11 years ago

Tracked this down. TopSitesView needs to know current state as well.

	Jim Mathies [:jimm]
	Comment 6 • 11 years ago

Comment on attachment 789529 [details] [diff] [review]
fix

Note these are built on top of some css changes (removal of the scrollbox) that are currently on fx-team.

	Jim Mathies [:jimm]
	Comment 8 • 11 years ago

Comment on attachment 789529 [details] [diff] [review]
fix

wrong patch.

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

We need to call this._view.onViewStateChange(viewstate); on init for the other views too (bookmarks, history and remote tabs), to set the suppress selection properly.

	Jim Mathies [:jimm]
	Comment 11 • 11 years ago

(In reply to Rodrigo Silveira [:rsilveira] from comment #10)
> We need to call this._view.onViewStateChange(viewstate); on init for the
> other views too (bookmarks, history and remote tabs), to set the suppress
> selection properly.

Hmm, sounds like some of what's in _adjustDOMforViewState can go into View. I'll take a look.

	Jim Mathies [:jimm]
	Comment 12 • 11 years ago

updated per rsilveira's comments.

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

Comment on attachment 789692 [details] [diff] [review]
fix

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

Updated your patch, coming right up.

::: browser/metro/base/content/startui/BookmarksView.js
@@ +274,5 @@
>      // Send refresh event so all view are in sync.
>      this._sendNeedsRefresh();
>    },
>  
> +  _adjustDOMforViewState: function _adjustDOMforViewState(aState) {

This seems a bit inside out? We want views to be able to adjust DOM for viewstate - at initalization and as a consequence of viewstate changing. I'm going to attach a counter-proposal.

::: browser/metro/modules/View.jsm
@@ +33,5 @@
>        this._set.setAttribute("suppressonselect", (aState == "snapped"));
>      }
>    },
>  
> +  queryAndUpdateViewState: function () {

We've done this work in StartUI already to set the viewstate broadcaster. I'm thinking we could have some markup in the view observe that viewstate attribute.

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

Revision of jimm's patch which gives View a noop _adjustDOMforViewState which is implemented/overridden in TopSitesView where we need to update the richgrid children. I think this is functionally equivalent?

	Jim Mathies [:jimm]
	Comment 15 • 11 years ago

Comment on attachment 790479 [details] [diff] [review]
observe viewstate in view markup and call a usually-noop _adjustDOMforViewState from onViewStateChange

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

From the perspective of the original top sites bug, this looks great. The bookmarks and history changes just execute the no-op code though, or am I missing something? rsilveira was concerned about something there (see comment 10).

	Jim Mathies [:jimm]
	Comment 16 • 11 years ago

I think those update dom calls need to be queryAndUpdateViewState calls?

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

Builds and refactors Jimm's 'fix' patch. The suppressonselect attribute is applied in the View's _adjustDOMforViewState. This can be called at any time - we call in the constructor and in onViewStateChange. 
TopSites also needs to update the tiletype attributes on its children, so I wrap the inherited _adjustDOMforViewState. 
This seems to work out. A couple of times I've caught the topsites not updating/reflowing in snapped view so its possible some race condition persists? But I've been unable to reproduce the last several times I've tested so it might have been a blip.

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

Comment on attachment 791452 [details] [diff] [review]
observe viewstate in view markup, call _adjustDOMforViewState at init and from onViewStateChange

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

LGTM and works great.

::: browser/metro/base/content/startui/TopSitesView.js
@@ +239,5 @@
> +    for (let item of this._set.children) {
> +      if (tileType) {
> +        item.setAttribute("tiletype", tileType);
> +      } else {
> +        item.removeAttribute("tiletype");

Wonder if we can update our CSS and code to have the tiletype on the grid only. I don't think we'd ever want to have mixed tile types on the same grid. Should be on a separate bug though.

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

On fx-team: https://hg.mozilla.org/integration/fx-team/rev/dd9d2871a6f2

I've gone back and forth about having this tiletype attribute and logic, vs. doing it all in CSS. And we certainly dont support having different tile types in a grid currently (although its not a huge stretch to imagine it becoming a requirement). We could look at it, I just want to be sure we dont create more strange magic from afar - which CSS can be very prone to.

	Phil Ringnalda (:philor)
	Comment 20 • 11 years ago

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

	Manuela Muntean [Away]
	Comment 21 • 11 years ago

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130827030201

WFM
Tested on both Windows 8 32bit and 64bit using latest Nightly for iteration #13 and following the STR from comment 0.