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

Status: RESOLVED FIXED

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

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

On auto-complete screen, there is no need of customization. We should remove the ability of right-clicking or swiping to select tiles.

Comment 1

[Sam Foster [:sfoster] (he/him)]

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)

Comment 2

[Allison Naaktgeboren :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?

Comment 3

[Sam Foster [:sfoster] (he/him)]

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.

Comment 4

[Allison Naaktgeboren :ally]

Attachment: suppressSelectAutocomplete

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 793703 [details] [diff] [review]
suppressSelectAutocomplete

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

Just so.

Comment 6

[Allison Naaktgeboren :ally]

It helps to remember to land one work! >.<
https://hg.mozilla.org/integration/fx-team/rev/eb46f1ed8c8c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-mc failures.
https://hg.mozilla.org/integration/fx-team/rev/d65841c24ab2

https://tbpl.mozilla.org/php/getParsedLog.php?id=27501727&tree=Fx-Team

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP add new noContext flag on richgrids to block any contextmenu/long-tap/cross-slide behavior

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.

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

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.

Comment 10

[Allison Naaktgeboren :ally]

Attachment: v0 nocontext attribute

based on Sam's WIP. Run through mochitest-metro without failures.

Comment 11

[Sam Foster [:sfoster] (he/him)]

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.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Test snapped view selection behavior, correct viewstate change handling

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 13

[Rodrigo Silveira [:rsilveira] [:rodms]]

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.

Comment 14

[Allison Naaktgeboren :ally]

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

Notes:
  - 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

Comment 15

[Allison Naaktgeboren :ally]

Attachment: patches folded together, see comments for details

Comment 16

[Sam Foster [:sfoster] (he/him)]

Working on a patch to improve the stability of some of these tests. So far I've got this: https://tbpl.mozilla.org/?tree=Try&rev=4cbc168ebe35 .. 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...

Comment 17

[Sam Foster [:sfoster] (he/him)]

Attachment: Add noContext/contextmenu toggle for richgrid, disable selection for snapped view and awesomebar results

Unbitrotted this, fixed a busted test and crossing fingers with this try-push:
https://tbpl.mozilla.org/?tree=Try&rev=8153f7976b2e 

Note that you may need these patches in your queue to apply it cleanly: 
https://hg.mozilla.org/integration/fx-team/rev/dc42a14663fe
https://bug927938.bugzilla.mozilla.org/attachment.cgi?id=818610

Its all green and clean locally.

Comment 18

[Sam Foster [:sfoster] (he/him)]

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 19

[Rodrigo Silveira [:rsilveira] [:rodms]]

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 https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/grid.xml#35

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.

Comment 20

[Sam Foster [:sfoster] (he/him)]

Attachment: selectNone

I made a selectNone method, which does clearSelection and fires a selectionchange event as necessary. I'll also update the noContext patch...

Comment 21

[Sam Foster [:sfoster] (he/him)]

Attachment: Add noContext/contextmenu toggle for richgrid, disable selection for snapped view and awesomebar results

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.

Comment 22

[Sam Foster [:sfoster] (he/him)]

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

Comment 23

[Carsten Book [:Tomcat]]

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