Closed Bug 660686 Opened 8 years ago Closed 8 years ago

Enable entire row region selection in Preferences

Categories

(Firefox for Android Graveyard :: General, enhancement)

All
Android
enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: aaronmt, Assigned: sriram)

References

Details

(Keywords: polish)

Attachments

(2 files, 5 obsolete files)

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.
Whiteboard: [good first bug][mentor=mfinkle]
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
Assignee: nobody → sramasubramanian
Attached patch WIP Patch (obsolete) — Splinter Review
This patch has been tested on "desktop" build of Fennec.
I am yet to check this on a mobile.
Attachment #555622 - Flags: review?(mark.finkle)
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.
Attachment #555622 - Flags: review?(mark.finkle)
Attachment #555622 - Flags: review?(dtownsend)
Attachment #555622 - Flags: review+
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.
Attachment #555622 - Flags: review?(dtownsend) → review-
Attached patch Patch with binding (obsolete) — Splinter Review
Patch with new bindings for mobile.
Attachment #555622 - Attachment is obsolete: true
Attachment #556148 - Flags: review?(mark.finkle)
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
Attachment #556148 - Flags: review?(mark.finkle)
Attachment #556148 - Flags: review?(dtownsend)
Attachment #556148 - Flags: review+
Attached patch Patch with ID change (obsolete) — Splinter Review
The ID has been changed from setting-mobile-* to setting-fulltoggle-* .

I'll update the tests as a second patch soon.
Attachment #556148 - Attachment is obsolete: true
Attachment #556148 - Flags: review?(dtownsend)
Attachment #557634 - Flags: review?(mark.finkle)
Attachment #557634 - Flags: review?(dtownsend)
The bindings have been moved into mobile.
Attachment #557634 - Attachment is obsolete: true
Attachment #557634 - Flags: review?(mark.finkle)
Attachment #557634 - Flags: review?(dtownsend)
Attachment #557998 - Flags: review?(mark.finkle)
Attached patch Patch for browser chrome test (obsolete) — Splinter Review
Browser chrome test for "full click toggle" of preferences in mobile.
Attachment #558000 - Flags: review?(mark.finkle)
Attachment #557998 - Flags: review?(mark.finkle) → review+
Attachment #558000 - Flags: review?(mark.finkle) → review+
Sending to try server
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110907 Firefox/9.0a1 Fennec/9.0a1
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?
New bug sounds good
Depends on: 685147
Depends on: 686379
Backed out because of bug 686379:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e864670fad4e
Status: RESOLVED → REOPENED
Keywords: polish
Hardware: ARM → All
Resolution: FIXED → ---
Whiteboard: [good first bug][mentor=mfinkle]
Target Milestone: Firefox 9 → ---
This solves the problem of tap-hold scroll https://bugzilla.mozilla.org/show_bug.cgi?id=686379
Attachment #557998 - Attachment is obsolete: true
Attachment #560428 - Flags: review?(mark.finkle)
Tests now changed to handle "TapSingle".
Attachment #558000 - Attachment is obsolete: true
Attachment #560444 - Flags: review?(mark.finkle)
Attachment #560428 - Flags: review?(mark.finkle) → review+
Attachment #560444 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/56a5de4a7413
https://hg.mozilla.org/mozilla-central/rev/a72d43f339cd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Depends on: 687298
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
Depends on: 688640
You need to log in before you can comment on or make changes to this bug.