Closed Bug 897113 Opened 10 years ago Closed 9 years ago

Work - Tiles on auto-complete screen should not be selectable


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

Windows 8.1


(Not tracked)

Firefox 27


(Reporter: ywang, Assigned: sfoster)



(Whiteboard: feature=work [preview])


(2 files, 6 obsolete files)

On auto-complete screen, there is no need of customization. We should remove the ability of right-clicking or swiping to select tiles.
Whiteboard: feature=change c=tbd u=tbd p=0
We have a similar requirement for the snapped-view tiles. I'm thinking a selectable attribute on the grid's bound node, and a check for its truthiness in the handlers before we select/toggleSelection (bearing in mind that when seltype="single", select means default action)
Priority: -- → P2
Blocks: 831910
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=navigation_app_bar_autocomplete u=metro_firefox_user p=0
No longer blocks: metrov1defect&change
Summary: Change - Tiles on auto-complete screen should not be selectable → Work - Tiles on auto-complete screen should not be selectable
Whiteboard: feature=change c=navigation_app_bar_autocomplete u=metro_firefox_user p=0 → feature=work
Component: General → Firefox Start
Whiteboard: feature=work → feature=work [preview]
Assignee: nobody → ally
Assignee: ally → nobody
Assignee: nobody → ally
So if I understand the problem correctly, the following is true:

1)The end goal is that these grids not be selectable and the swiping to select & the right click menu that Yuan mentions are possible because those grids are selectable.

2) The suggested implementation is to add another value for seltype, such as seltype="noselect" that the selection/toggle code understands to mean it should not allow selection on grids with that attribute?

What about stripping the seltype attribute from those grids in the case of the autocomplete or snapped? Is there an assumption baked in that we have to have that attribute present?
Flags: needinfo?(sfoster)
The richgrid binding has a suppressOnSelect property that is checked by any of the entry points that might trigger selection. It simply looks to the presence of a "suppressonselect" attribute, so toggling the behaviour is a simple as adding that attribute to the bound node. This is how we toggle selection behavior as we go in/out of snapped view. For the autocomplete results grids you should be able to go ahead and put the attribute in there in urlbar.xml and it will do the right thing. 

The seltype attribute should remain as "single". As indicated in the richgrid comments, the notion of "selection" is a bit overloaded and means different things depending on what this attribute value is. But single is right for this usage.
Flags: needinfo?(sfoster)
Attached patch suppressSelectAutocomplete (obsolete) — Splinter Review
Attachment #793703 - Flags: review?(sfoster)
Comment on attachment 793703 [details] [diff] [review]

Review of attachment 793703 [details] [diff] [review]:

Just so.
Attachment #793703 - Flags: review?(sfoster) → review+
This patch adds a new nocontext attribute and noContext property on richgrid. We check its truthiness before handling contextmenu/long-tap events (and don't even setup CrossSlide if its false.)
This un-overloads seltype="single" a bit. So you'll want to check any assumptions being made in the tests about the behaviour of seltype="single" vs. multiple richgrids.
Attachment #793703 - Attachment is obsolete: true
Comment on attachment 801019 [details] [diff] [review]
WIP add new noContext flag on richgrids to block any contextmenu/long-tap/cross-slide behavior

Review of attachment 801019 [details] [diff] [review]:

We'll need to change view.jsm too.
Attached patch v0 nocontext attribute (obsolete) — Splinter Review
based on Sam's WIP. Run through mochitest-metro without failures.
Attachment #801019 - Attachment is obsolete: true
Attachment #801922 - Flags: review?(sfoster)
Comment on attachment 801922 [details] [diff] [review]
v0 nocontext attribute

Review of attachment 801922 [details] [diff] [review]:

You are missing corresponding changes to View.jsm, however, we are missing a test that would have caught that. So I'm going to r- for now and attach a patch with that test.

::: browser/metro/theme/tiles.css
@@ +206,5 @@
>    box-shadow: 0px 0px 2px 2px rgba(0, 0, 0, 0.05), 0px 2px 0px rgba(0, 0, 0, 0.1);
>  }
>  /* selected tile indicator */
> +richgrid:not([nocontext])  richgriditem[selected] > .tile-content::after {

In the snapped-view case, this not([nocontext]) qualifier shouldn't be necessary - if an item *is* selected that's the symptom we should treat. But I gather we do need it for auto-complete as otherwise the item appears selected briefly before navigating away.
So this is good.
Attachment #801922 - Flags: review?(sfoster) → review-
Builds on the 'v0 nocontext attribute' patch. 
It turns out we had a bug in the viewstate handling in View.jsm. We should explicitly clearSelection there for snapped view. I've reworked that method to add that, as well as a mochitest for the start grid selection behavior as we move between landscape and snapped view. 
*I think* that gets us back on track. Ally, could you look it over, fold the patches and give it back to rsilveira for review?
Comment on attachment 802690 [details] [diff] [review]
Test snapped view selection behavior, correct viewstate change handling

Review of attachment 802690 [details] [diff] [review]:

::: browser/metro/base/tests/mochitest/browser_snappedState.js
@@ +206,5 @@
> +
> +    is(bookmarksGrid.selectedItems.length, 0, "no grid item selections possible in snapped view");
> +  },
> +  tearDown: function() {
> +    BookmarksTestHelper.restore();

Need a HistoryTestHelper.restore() too.
Rolled patches together, and got some really weird test behavior, broken only when running the whole suite(making it extra painful for testing). Either pulling Sam's test or the browser_url.js

Patch changes:
  - m-c rejected all of the changes to Views.jsm, so I had to rebase that
  - Test tear down function needs a HistoryTestHelper.restore(); call
  - At all points all tests pass individually, errors only occurred when the full suite is run

  - if you rename files to move them around, note you may need to restart your terminal (I lost a lot of time to a 'phantom'(removed) test that would not go away and messed with my results.)
  - changes to build system (removing, add, renaming a test file) need a rebuild of browser/metro for the build system to notice there has been a configuration change.
  - mochitest goes through tests alphabetically. If your test run isn't what you think it should be, check the test code that was built & run at objdir/_tests/testing/mochitest/metro/browser/metro/base/tests/mochitest/

Initial Errors:
 2:38.81 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_urlbar_highlightURLs.js | runTests: Task failed - Error: transitionend event timeout at waitForEvent@chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/head.js:329
 2:38.81 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_urlbar_highlightURLs.js | Left over tab after test: 'about:blank'

After moving browser_urlbar_highlightURLs.js to browser_a.js:
 2:37.58 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_urlbar_trimURLs.js | runTests: Task failed - Error: transitionend event timeout at waitForEvent@chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/head.js:329
 2:37.58 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_urlbar_trimURLs.js | Left over tab after test: 'about:blank'

After removing browser_urlbar.js:
 No errors, all tests pass

Reinsterting browser_url.js and removing Sam's new test in browser_snappedState.js:
 No errors, all tests pass
Assignee: ally → sfoster
Working on a patch to improve the stability of some of these tests. So far I've got this: .. that context_ui test failure fails 100% of the time for me locally. So although its unrelated its hard to land new tests without fixing these first. Probably this work should move to a new bug that blocks this one...
Unbitrotted this, fixed a busted test and crossing fingers with this try-push: 

Note that you may need these patches in your queue to apply it cleanly:

Its all green and clean locally.
Attachment #801922 - Attachment is obsolete: true
Attachment #802690 - Attachment is obsolete: true
Attachment #803411 - Attachment is obsolete: true
Attachment #818780 - Flags: review?(rsilveira)
Attachment #818780 - Flags: review?(ally)
Try test failure was the same that backed out rev dc42a14663fe (bug 927226) - i.e unrelated. But, I'm testing a fix for that and will update here when I have news.
Comment on attachment 818780 [details] [diff] [review]
Add noContext/contextmenu toggle for richgrid, disable selection for snapped view and awesomebar results

Review of attachment 818780 [details] [diff] [review]:

As discussed on IRC, making selections then snapping with a mouse will keep the context bar. We need to fire a "selectionchange" event in clearSelection() at

That looks good and works fine. r+ with the above and border shifting tile content addressed.

::: browser/metro/base/content/bindings/grid.xml
@@ +608,5 @@
>                    onget="return this.getAttribute('suppressonselect') == 'true';"
>                    onset="this.setAttribute('suppressonselect', val);"/>
> +      <property name="noContext"
> +                  onget="return this.getAttribute('nocontext') == 'true';"
> +                  onset="this.setAttribute('nocontext', val);"/>

In most places we remove the attribute when val is falsy. Since the setter is not being used I'd suggest removing it or fixing it so that it can be used in view.jsm. I prefer the latter since it better isolates attribute jiggling logic.

::: browser/metro/theme/tiles.css
@@ +222,5 @@
>    background-size: 35px 35px;
>    border: @metro_border_xthick@ solid @selected_color@;
>  }
> +richgrid[nocontext]  richgriditem[selected] > .tile-content {
> +  border: @metro_border_xthick@ solid @selected_color@;

The border is shifting the content of the tiles in autocomplete. We can try using an inset box-shadow like:
box-shadow: 0 0 0 @metro_border_xthick@ @selected_color@ inset;
Or regular (non-inset) border or use the same ::after border trick as above.
Attachment #818780 - Flags: review?(rsilveira) → review+
Attached patch selectNoneSplinter Review
I made a selectNone method, which does clearSelection and fires a selectionchange event as necessary. I'll also update the noContext patch...
Attachment #820038 - Flags: review?(rsilveira)
Not too much new here, just the selectNone usage (implementation/test in its own patch). Add after selectNone in your queue. By my testing the mouse-driven snapped view issue is fixed.
Attachment #818780 - Attachment is obsolete: true
Attachment #818780 - Flags: review?(ally)
Attachment #820039 - Flags: review?(rsilveira)
Attachment #820038 - Flags: review?(rsilveira) → review+
Attachment #820039 - Flags: review?(rsilveira) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.