660686 - Enable entire row region selection in Preferences

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110530 Firefox/7.0a1 Fennec/7.0a1

The following changeset [1] contains modifications to the Gingerbread theme which landed patches from Bug 659457. 

[1] http://hg.mozilla.org/mozilla-central/rev/113014cf801a

Now that the theme has landed and it more closely resembles the native Android experience, in Preferences I often find myself tapping a specified preferences text label expecting the toggle (checkbox for the most part) to change based on my row selection. Currently the hotspot region is only on the XUL preference type.

To mimic that of the native widget Android preferences, the Preferences in the browser should have entire row region selection.

Comment 1

[Aaron Train [:aaronmt]]

I'd like to work on this in my spare time. Currently looking at the bindings for settings.xml [1], not sure at the moment what changes would be necessary for each specific binding like setting-bool. I wonder if  simply setting a control attribute on the labels would do the trick, although the target would be an anonid (no idea how to reference those).

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/setting.xml#169

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP Patch

This patch has been tested on "desktop" build of Fennec.
I am yet to check this on a mobile.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 555622 [details] [diff] [review]
WIP Patch

Looks good. I wonder if there is a way to default to fulltoggle in mobile.

Adding Mossop on for toolkit review too.

Dave what kind of tests should we look to add here? I noticed a few simple tests in toolkit already. I think we should add the fulltoggle=true" variant, just to make sure it appears valid. We should also try to get at least one test with a fulltoggle="true" and fulltoggle="false", then use mouse events to test the click behavior.

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 555622 [details] [diff] [review]
WIP Patch

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 555622 [details] [diff] [review]
> WIP Patch
> 
> Looks good. I wonder if there is a way to default to fulltoggle in mobile.

This. I don't think we want extension devs to be specifying this, it should just work IMO. I would guess at a pref being the right way to set the default for this unless we can do something truly awesome with media queries selecting a different XBL binding based on screen size?

> Dave what kind of tests should we look to add here? I noticed a few simple
> tests in toolkit already. I think we should add the fulltoggle=true"
> variant, just to make sure it appears valid. We should also try to get at
> least one test with a fulltoggle="true" and fulltoggle="false", then use
> mouse events to test the click behavior.

This sounds good to me.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch with binding

Patch with new bindings for mobile.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 556148 [details] [diff] [review]
Patch with binding

Looks ok to me, except for the "setting-mobile-*" ID. I think we should be mobile-centric. Instead let's try to be more feature-centric. I like "setting-fulltoggle-*" for the IDs better.

Mossop - do you want the "fulltoggle" bindings in the toolkit file, or would you rather we move them to a /mobile/ file?

Sriram - You should start a second patch (or add to this patch) to create a few tests for the fulltoggle bindings.

r+ with the ID change

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch with ID change

The ID has been changed from setting-mobile-* to setting-fulltoggle-* .

I'll update the tests as a second patch soon.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch for bindings moved into mobile

The bindings have been moved into mobile.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch for browser chrome test

Browser chrome test for "full click toggle" of preferences in mobile.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Sending to try server

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0959354c6d6a
http://hg.mozilla.org/integration/mozilla-inbound/rev/c23b1b22041b

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/c23b1b22041b
http://hg.mozilla.org/mozilla-central/rev/0959354c6d6a

Comment 13

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110907 Firefox/9.0a1 Fennec/9.0a1

Comment 14

[Aaron Train [:aaronmt]]

One thing missing from this implementation is the tap-hold selection color status on the entire row region. In the stock browser on Gingerbread, this will be orange, and on the Galaxy SII, it will be blue. This implementation should mimic that functionality. Follow-up patch, or new bug?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

New bug sounds good

Comment 16

[Matt Brubeck (:mbrubeck)]

Backed out because of bug 686379:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e864670fad4e

Comment 17

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch for bindings moved into mobile

This solves the problem of tap-hold scroll https://bugzilla.mozilla.org/show_bug.cgi?id=686379

Comment 18

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch for browser chrome test

Tests now changed to handle "TapSingle".

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72d43f339cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a5de4a7413

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/56a5de4a7413
https://hg.mozilla.org/mozilla-central/rev/a72d43f339cd

Comment 21

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1