Closed Bug 896766 Opened 11 years ago Closed 11 years ago

Defect - Disable tile selecting and context appbar in snapped view

Categories

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

Product:
Firefox for Metro Graveyard
Component:
Firefox Start
Platform:
x86
Windows 8.1
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 25

People

(Reporter: rsilveira, Assigned: sfoster)

References

Details

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

Attachments

(2 files, 2 obsolete files)

SnappedContextBar.png
30.39 KB, image/png
Details
Toggle the suppressonselect attribute on grids in snapped view
11.90 KB, patch
rsilveira
: review+
Details | Diff | Splinter Review
Attached image SnappedContextBar.png —
We can barely fit 3 items on the context app bar, see screenshot. I don't think it's worth redesigning, better to make snapped view read-only for grid items.
Blocks: metrov1defect&change
Summary: Disable tile selecting and context appbar in snapped view → Defect - Disable tile selecting and context appbar in snapped view
Whiteboard: feature=defect c=tbd u=tbd p=0
I'll take this. I think we have have a flag on the grid that views can toggle as we move in/out of snapped viewstate p=1 I think
Assignee: nobody → sfoster
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=1
Priority: -- → P2
We already had a suppressonselect attribute / property, so I'm using that here, with the appropriate guards to ensure selection doesnt take place if its truthy. I put the metro_viewstate_changed observer in the shared View prototype, but left the addObserver/removeObserver owned by each of the views. Note that I don't disable the CrossSlide handling entirely, just ignore the select event, so you can still drag tiles in snapped view. And note that they drag in the wrong direction (should be side-side in a vertically-oriented container - that's a separate bug)
Attachment #781971 - Flags: review?(rsilveira)
Attachment #781971 - Attachment is patch: true
Attachment #781971 - Attachment mime type: message/rfc822 → text/plain
Comment on attachment 781971 [details] [diff] [review] Toggle the suppressonselect attribute on grids in snapped view Review of attachment 781971 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/content/bindings/grid.xml @@ +111,5 @@ > <parameter name="aItem"/> > <parameter name="aEvent"/> > <body> > <![CDATA[ > + if(!this.isBound || (this.suppressOnSelect || this._suppressOnSelect)) Not really part of this patch, but having both _suppressOnSelect and suppressOnSelect seems unnecessary. _suppressOnSelect field doesn't seem to be used anywhere. ::: browser/metro/base/content/bookmarks.js @@ +91,4 @@ > window.addEventListener('MozAppbarDismissing', this, false); > window.addEventListener('BookmarksNeedsRefresh', this, false); > > + nit: empty line ::: browser/metro/modules/View.jsm @@ +33,5 @@ > + switch(aTopic) { > + case "metro_viewstate_changed": > + if (this._set) { > + this._set.setAttribute("suppressonselect", (aState == "snapped")); > + } I find it confusing to have an observe here without any addOberver calls. We're coupling both View and the descendants to viewstate. There are a few options: - Have the logic only in View. That will require more OO joy like adding a way to access _set from here, call View constructor from the descendant's constructors and removing the observer on destruct. - Rename this to something like onViewstateChange, keep the switch on the views and call this shared method. This way the observer listening logic is kept separate from the action. - Move the entire logic to grid.xml - probably the simplest, just adds a "grid is a metro component" and cares about viewstate coupling. What do you think?
Attachment #781971 - Flags: review?(rsilveira) → review-
Agreed on _suppressOnSelect, we can probably lose this legacy property as it seems to serve no function currently. I'd rather do it in another bug though. I'm a bit reluctant to add the observer to the richgrid binding - in that context, the rule that snapped view gets non-selectable seems arbitrary. Its a business rule that seems to belong in StartUI or the view itself. So I'm for a onViewStateChange method, which is more in the spirit of the shared prototype of common functionality we're using View for today (vs. a superclass and all that implies).
Sound good.
onViewStateChange implemented on the View.prototype, just expects the state name as single arg.
Attachment #781971 - Attachment is obsolete: true
Attachment #782046 - Flags: review?(rsilveira)
Comment on attachment 782046 [details] [diff] [review] Toggle the suppressonselect attribute on grids in snapped view Missed a couple of things, sorry.
Attachment #782046 - Flags: review?(rsilveira)
* I got rid of _suppressOnSelect, I don't think that's going to be an unexpected surprise for anyone. * Think I got the whitespace nits. * onViewStateChange implemented on View.prototype, observe implemented/augmented in the views to call it with the state name.
Attachment #782046 - Attachment is obsolete: true
Attachment #782048 - Flags: review?(rsilveira)
Blocks: metrov1it12
No longer blocks: metrov1it11
Comment on attachment 782048 [details] [diff] [review] Toggle the suppressonselect attribute on grids in snapped view Review of attachment 782048 [details] [diff] [review]: ----------------------------------------------------------------- Look great, thanks for the update!
Attachment #782048 - Flags: review?(rsilveira) → review+
https://hg.mozilla.org/mozilla-central/rev/7944ca804692
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Build ID: 20130815030203 Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc WFM Tested on windows 8 using latest nightly for iteration-12. Tile selecting and context appbar are disabled in snapped view.
Went through the following "Defect" for iteration #13 without any issues. Used the following build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-09-01-03-02-18-mozilla-central/ - Went through the original issue from comment #0 without any issues - Went through "Top Sites", "Bookmarks" & "History" and ensured you cannot select tiles while in snapped view (both on the right & left side of the screen) - Went through "Top Sites", "Bookmarks" & "History" and ensured that you can still drag the tiles while in snapped view (both on the right & left side of the screen) - Ensured that tapping on tiles in "Top Sites", "Bookmarks" & "History" works without any issues - Ensured that you can still select tiles while in full & filled views
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: