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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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).

Comment 3

[Sriram Ramasubramanian [: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.

Comment 4

[Sriram Ramasubramanian [:sriram]]

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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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+

Comment 9

[Sriram Ramasubramanian [:sriram]]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/321ce1200ede
https://hg.mozilla.org/integration/mozilla-inbound/rev/513cf5f34495
https://hg.mozilla.org/integration/mozilla-inbound/rev/d96d3de29187

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/321ce1200ede
https://hg.mozilla.org/mozilla-central/rev/513cf5f34495
https://hg.mozilla.org/mozilla-central/rev/d96d3de29187