[ouya] Make row items on about:home clickable using the gamepad action button

RESOLVED FIXED in Firefox 22

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 22
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 729719 [details] [diff] [review]
Part 1 - Make the item rows respond to the gamepad action button

Clicking the "O" button on the gamepad while focus is on the row items (e.g. featured addons or restored tabs) does nothing. This patch activates the item. The patch depends on the GamepadUtils which is added in a patch on bug 854971 (pending review).
Attachment #729719 - Flags: review?(sriram)
Attachment #729719 - Attachment description: Patch → Part 1 - Make the item rows respond to the gamepad action button
Created attachment 729743 [details] [diff] [review]
Part 2 - Make the more text links focusable and clickable

I'm not sure if it's a good idea to reuse the action_bar_button drawable xml file for this; I can copy it into a new xml file if you prefer.
Attachment #729743 - Flags: review?(sriram)
Created attachment 729745 [details] [diff] [review]
Part 3 - Make the promo box focusable

Since I lack the requisite photoshop skillz, I filed a follow-up for getting a proper focused-highlight promo box background, and just re-used the pressed background for focus as well. It already responds to click events so I didn't have do anything there (I guess overriding onClick is better than setting an onClickListener because onClick gets run with the gamepad action button already).
Attachment #729745 - Flags: review?(sriram)
Comment on attachment 729745 [details] [diff] [review]
Part 3 - Make the promo box focusable

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

Looks good to me.
Attachment #729745 - Flags: review?(sriram) → review+
Comment on attachment 729719 [details] [diff] [review]
Part 1 - Make the item rows respond to the gamepad action button

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

LGTM.
Attachment #729719 - Flags: review?(sriram) → review+
The "part 2" patch makes the entire "more text" row highlightable. See action shots at [1] and [2]. I discussed this with sriram on IRC and he suggested I get feedback from ibarlow about this behaviour - it may be that the link there is supposed to emulate a normal web link, in which case the focus highlight should be a smaller rectangle around the text. Note also that my patch means that when the link is clicked on a touch device (e.g. a regular phone) the entire row will change color as it enters the "pressed" state. This may also be undesirable; sriram suggested that we might want to just change the text color instead.

Personally I feel that trying to emulate a web link is going to be hard because we'll want the focus highlight to look like the spatial navigation highlight. The styles for that will change independently and it will be annoying to always keep these in sync. More importantly, the active click area is the entire row, not just the text (even on a phone with current nightly, you click activate the link by clicking the rightmost edge of the row) and I think the focus/press highlight should reflect that.

[1] http://people.mozilla.com/~kgupta/tmp/highlight-addon.png
[2] http://people.mozilla.com/~kgupta/tmp/highlight-more.png
Flags: needinfo?(ibarlow)
Talked to Ian about this today, and he also wanted the link itself to be focused, as opposed to the entire row. I'll update the patch.
Flags: needinfo?(ibarlow)
Created attachment 730156 [details] [diff] [review]
Part 2 - Make the more text links focusable and clickable (v2)

Update part 2 so that only the text piece has a background. I couldn't do this just in styles since the LinkTextView is the size of the whole row but I could do it in code by applying/removing a span on the text. This also keeps the behaviour on touch devices the same as it was previously - tapping on the more text links doesn't show any focus/pressed states.
Attachment #729743 - Attachment is obsolete: true
Attachment #729743 - Flags: review?(sriram)
Attachment #730156 - Flags: review?(sriram)
Created attachment 730303 [details] [diff] [review]
Part 1 - Make the item rows respond to the gamepad action button

Small adjustment to part 1 to also check that event.getAction() == KeyEvent.ACTION_DOWN because the onKey method gets called on key up as well. Carrying r+
Attachment #729719 - Attachment is obsolete: true
Attachment #730303 - Flags: review+
Comment on attachment 730156 [details] [diff] [review]
Part 2 - Make the more text links focusable and clickable (v2)

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

This looks good to me. If there's a way to make it "wrap_content", with a "center" gravity, and have the focus in XML, I would be more happy.
But then this UI is changing. Let's not do it now :D
Attachment #730156 - Flags: review?(sriram) → review+
Component: General → Theme and Visual Design
https://hg.mozilla.org/mozilla-central/rev/321ce1200ede
https://hg.mozilla.org/mozilla-central/rev/513cf5f34495
https://hg.mozilla.org/mozilla-central/rev/d96d3de29187
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.