888497 - Fix focus order of browser toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

I hate how we have to manually update the focus order for this to work correctly -- it's a fragile implementation that can (and did) break whenever we change these views.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Fix focus order of browser toolbar

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 769206 [details] [diff] [review]
Fix focus order of browser toolbar

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

r+ with those changes.

::: mobile/android/base/BrowserToolbar.java
@@ +450,5 @@
> +            mFocusOrder = Arrays.asList(mTabs, mBack, mForward, mLayout,
> +                    mSiteSecurity, mReader, mStop, mActionItemBar, mMenu);
> +        } else {
> +            mFocusOrder = Arrays.asList(mLayout, mSiteSecurity, mReader, mStop,
> +                    mMenu, mTabs);

I think the order should be "mTabs, mMenu".

@@ +882,4 @@
>      private void updateFocusOrder() {
>          View prevView = null;
>  
> +        boolean needsNewFocus = false;

Please add a comment on what this boolean is tracking.

Comment 3

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1e84483fc87

Comment 4

[Brian Nicholson (:bnicholson)]

Bad merge, backed out for bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a49caa21d9

Comment 5

[Brian Nicholson (:bnicholson)]

Second attempt: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7bbdf1b910

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fa7bbdf1b910

Comment 7

[Michal Kajda [:mkajda]]

Unfortunately, this fix introduces a crash.
Please refer to bug 889094 for details.