[MP] Defect - Start Page didn't snap when users hit back in snap view

VERIFIED FIXED in Firefox 26

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: ywang, Assigned: sfoster)

Tracking

Trunk
Firefox 26
All
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview] feature=defect c=tbd u=tbd p=1)

Attachments

(1 attachment, 5 obsolete attachments)

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.
OS: All → Windows 8 Metro
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=0
Blocks: 904168
Posted patch fix (obsolete) — Splinter Review
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.
Moving the _adjustDOMforViewState call below all the show calls doesn't help either.
Summary: Defect - Start Page didn't snap when users hit back in snap view → [MP] Defect - Start Page didn't snap when users hit back in snap view
Adding a red debug border around start-container shows the viewstate css is taking effect. But the top site thumbs aren't going away.
Tracked this down. TopSitesView needs to know current state as well.
Posted patch fixes (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #789543 - Attachment is obsolete: true
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.
Attachment #789529 - Flags: review?(sfoster)
Duplicate of this bug: 904168
Comment on attachment 789529 [details] [diff] [review]
fix

wrong patch.
Attachment #789529 - Flags: review?(sfoster)
Posted patch fix (obsolete) — Splinter Review
Attachment #789529 - Attachment is obsolete: true
Attachment #789573 - Flags: review?(sfoster)
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.
(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.
Posted patch fix (obsolete) — Splinter Review
updated per rsilveira's comments.
Attachment #789573 - Attachment is obsolete: true
Attachment #789573 - Flags: review?(sfoster)
Attachment #789692 - Flags: review?(sfoster)
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=1
Blocks: metrov1it13
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
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.
Attachment #789692 - Flags: review?(sfoster) → feedback+
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?
Attachment #790479 - Flags: review?(jmathies)
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).
Attachment #790479 - Flags: review?(jmathies) → review+
Assignee: jmathies → sfoster
I think those update dom calls need to be queryAndUpdateViewState calls?
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.
Attachment #789692 - Attachment is obsolete: true
Attachment #790479 - Attachment is obsolete: true
Attachment #791452 - Flags: review?(rsilveira)
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.
Attachment #791452 - Flags: review?(rsilveira) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/dd9d2871a6f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.