856252 - Regression: Arrowing down to content from BrowserToolBar does not work

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eitan Isaacson [:eeejay]]

Focus seems to be stuck in the toolbar. I think this is a regression from the dynamic toolbar patches. Having the LayerView top be above the BrowserToolBar bottom messes with Android's focus finding algorithm.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Explicitly set next focused view when arrowing down from BrowserToolBar

Comment 2

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

Comment on attachment 731400 [details] [diff] [review]
Explicitly set next focused view when arrowing down from BrowserToolBar

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

Fine by me, but sriram should review this too.

Comment 4

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

Comment on attachment 731400 [details] [diff] [review]
Explicitly set next focused view when arrowing down from BrowserToolBar

Do we needed to explicitly call each child.setNextFocusDownId in BrowserToolbar.setNextFocusDownId? Can't we loop over children or something? Do we need and @Override ?

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 731400 [details] [diff] [review]
> Explicitly set next focused view when arrowing down from BrowserToolBar
> 
> Do we needed to explicitly call each child.setNextFocusDownId in
> BrowserToolbar.setNextFocusDownId?

You would think that setting the focus order on the container would be enough, but nope. You need to set it directly on focusable items.

> Can't we loop over children or something?

Well, recursively. Since the views may be nested. We could set any focusable ancestor. That might be worthwhile so that when widgets are added to the toolbar we don't need to remember to add it to this list? We have a similar problem with horizontal focus order. I think that the patch I introduced for that has the potential of rotting as more widgets are added.

> Do we need and @Override ?

BrowserToolBar is not a subclass of View, so no.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 731400 [details] [diff] [review]
Explicitly set next focused view when arrowing down from BrowserToolBar

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

Sounds good to me. Could you shorten "nextFocusDownId" parameter to "id" or "nextId"? That's a bit super long given that it's repeating in 10 statements. The method name says what the parameter is going to be. So, "id" sounds enough for the parameter name.

Comment 7

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/819dfbb15b29

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/819dfbb15b29

Comment 9

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 23.0a1 2013-04-03.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 731400 [details] [diff] [review]
Explicitly set next focused view when arrowing down from BrowserToolBar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716403
User impact if declined: Blind keyboard users on Gingerbread devices will no longer be able to reach web content from the awesome bar and thus not be able to browse web pages.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): No real alternatives, risk is minimal. This just sets a bunch of keyboard navigation attributes for controls.
String or IDL/UUID changes made by this patch:None.

Comment 11

[Marco Zehe (:MarcoZ)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b615cb59220