If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 27

Status

Firefox for Metro
Firefox Start
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Yuan, Assigned: sfoster)

Tracking

Trunk
Firefox 27
x86
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work [preview])

Attachments

(2 attachments, 6 obsolete attachments)

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

Updated

4 years ago
Blocks: 859003
Whiteboard: feature=change c=tbd u=tbd p=0
(Assignee)

Comment 1

4 years ago
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)

Updated

4 years ago
Priority: -- → P2

Updated

4 years ago
Blocks: 831910
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=navigation_app_bar_autocomplete u=metro_firefox_user p=0

Updated

4 years ago
No longer blocks: 859003
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

Updated

4 years ago
Component: General → Firefox Start
Whiteboard: feature=work → feature=work [preview]
Assignee: nobody → ally
Status: NEW → ASSIGNED
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)
(Assignee)

Comment 3

4 years ago
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)
Created attachment 793703 [details] [diff] [review]
suppressSelectAutocomplete
Attachment #793703 - Flags: review?(sfoster)
(Assignee)

Comment 5

4 years ago
Comment on attachment 793703 [details] [diff] [review]
suppressSelectAutocomplete

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

Just so.
Attachment #793703 - Flags: review?(sfoster) → review+
It helps to remember to land one work! >.<
https://hg.mozilla.org/integration/fx-team/rev/eb46f1ed8c8c
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
(Assignee)

Comment 8

4 years ago
Created attachment 801019 [details] [diff] [review]
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.
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.
Created attachment 801922 [details] [diff] [review]
v0 nocontext attribute

based on Sam's WIP. Run through mochitest-metro without failures.
Attachment #801019 - Attachment is obsolete: true
Attachment #801922 - Flags: review?(sfoster)
(Assignee)

Comment 11

4 years ago
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-
(Assignee)

Comment 12

4 years ago
Created attachment 802690 [details] [diff] [review]
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 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

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
Created attachment 803411 [details] [diff] [review]
patches folded together, see comments for details
Assignee: ally → sfoster
(Assignee)

Comment 16

4 years ago
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...
(Assignee)

Comment 17

4 years ago
Created attachment 818780 [details] [diff] [review]
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.
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)
(Assignee)

Comment 18

4 years ago
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 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.
Attachment #818780 - Flags: review?(rsilveira) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 820038 [details] [diff] [review]
selectNone

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)
(Assignee)

Comment 21

4 years ago
Created attachment 820039 [details] [diff] [review]
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.
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+
(Assignee)

Comment 22

4 years ago
On fx-team: https://hg.mozilla.org/integration/fx-team/rev/e0598dfe8de1
https://hg.mozilla.org/mozilla-central/rev/e0598dfe8de1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.