918007 - Disable toolbar elements (menu, back, forward, tabs) while in editing mode on tablets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Not an issue on the phone UI as it's possible to do that.

Comment 1

[Lucas Rocha (:lucasr)]

Ian, any thoughts on how this should behave?

Comment 2

[Ian Barlow (:ibarlow)]

I have a couple of options on how we could approach this. 

http://cl.ly/image/3y0R2c1z2F0N

1. We disable the forward and back buttons when we focus the URL bar. To exit out of edit mode, the user can either use the Android back button or scroll page content

2. The back and forward buttons take on the same characteristics of the Android Back button for this state: they hide the keyboard and unfocus the URL bar. After this, the next time the user taps either back or forward it returns to whatever the previous page was.

Comment 3

[Ian Barlow (:ibarlow)]

Hm, looking at bug 917776 makes me lean more toward option 1: disabling other title bar elements when the URL bar is focused, and re-enabling them when it is unfocused.

Comment 5

[Alex Keybl [:akeybl]]

Is this still targeting 26 on Aurora? We don't have our tracking flags set, so we're assuming not.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

I really want this fixed before going to beta. It is a major usability issue.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Factor out method to enable/disable buttons (r=wesj)

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Disable toolbar elements while in editing mode on tablets (r=wesj)

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 820393 [details] [diff] [review]
Factor out method to enable/disable buttons (r=wesj)

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

r+ with that.

::: mobile/android/base/BrowserToolbar.java
@@ +1566,5 @@
>  
> +    public void setButtonEnabled(ImageButton button, boolean enabled) {
> +        final Drawable drawable = button.getDrawable();
> +        if (drawable != null) {
> +            final int alpha = (int) (255 * (enabled ? 1.0f : 0.24f));

I don't like this conversion to float, and back to int.
I see this value is used in second patch. But I guess, we can use a comment here and make it simple.

drawable.setAlpha(enabled ? 255 : 77);

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 820394 [details] [diff] [review]
Disable toolbar elements while in editing mode on tablets (r=wesj)

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

r+ with suggested changes.

::: mobile/android/base/BrowserToolbar.java
@@ +1298,5 @@
> +
> +        final int actionItemsCount = mActionItemBar.getChildCount();
> +        for (int i = 0; i < actionItemsCount; i++) {
> +            final View actionItem = mActionItemBar.getChildAt(i);
> +

This new line is not needed.
And thinking again, both the statements could be made into one. It would still be easier to understand.

mActionItemBar.getChildAt(i).setEnabled(enabled);

@@ +1304,5 @@
> +        }
> +        ViewHelper.setAlpha(mActionItemBar, alpha);
> +
> +        final Tab tab = Tabs.getInstance().getSelectedTab();
> +

Could you please enclose the following statements inside "tab != null" check.
That way, we don't need a two new variables.

if (tab != null) {
    setButtonEnabled(mBack, enabled && tab.canDoBack());
    ...
}

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> Comment on attachment 820393 [details] [diff] [review]
> Factor out method to enable/disable buttons (r=wesj)
> 
> Review of attachment 820393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with that.
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1566,5 @@
> >  
> > +    public void setButtonEnabled(ImageButton button, boolean enabled) {
> > +        final Drawable drawable = button.getDrawable();
> > +        if (drawable != null) {
> > +            final int alpha = (int) (255 * (enabled ? 1.0f : 0.24f));
> 
> I don't like this conversion to float, and back to int.
> I see this value is used in second patch. But I guess, we can use a comment
> here and make it simple.
> 
> drawable.setAlpha(enabled ? 255 : 77);

Fair enough. Done.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> Comment on attachment 820394 [details] [diff] [review]
> Disable toolbar elements while in editing mode on tablets (r=wesj)
> 
> Review of attachment 820394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with suggested changes.
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1298,5 @@
> > +
> > +        final int actionItemsCount = mActionItemBar.getChildCount();
> > +        for (int i = 0; i < actionItemsCount; i++) {
> > +            final View actionItem = mActionItemBar.getChildAt(i);
> > +
> 
> This new line is not needed.
> And thinking again, both the statements could be made into one. It would
> still be easier to understand.
> 
> mActionItemBar.getChildAt(i).setEnabled(enabled);

Ok, done.

> @@ +1304,5 @@
> > +        }
> > +        ViewHelper.setAlpha(mActionItemBar, alpha);
> > +
> > +        final Tab tab = Tabs.getInstance().getSelectedTab();
> > +
> 
> Could you please enclose the following statements inside "tab != null" check.
> That way, we don't need a two new variables.
> 
> if (tab != null) {
>     setButtonEnabled(mBack, enabled && tab.canDoBack());
>     ...
> }

This will change the semantics a bit but won't be a problem in this case. Done.

Comment 13

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/2d8cdd7baa58
https://hg.mozilla.org/integration/fx-team/rev/0117133ba8ef

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 820393 [details] [diff] [review]
Factor out method to enable/disable buttons (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: All types of problems such as bug 928589 and bug 928592
Testing completed (on m-c, etc.): Landing in m-c now. Let's bake it for 1-2 days in Nightly and then uplift it.
Risk to taking this patch (and alternatives if risky): It's not a minor fix. But it's critical enough that we have to uplift to Fx26.  
String or IDL/UUID changes made by this patch: n/a

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 820394 [details] [diff] [review]
Disable toolbar elements while in editing mode on tablets (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: All types of problems such as bug 928589 and bug 928592
Testing completed (on m-c, etc.): Landing in m-c now. Let's bake it for 1-2 days in Nightly and then uplift it.
Risk to taking this patch (and alternatives if risky): It's not a minor fix. But it's critical enough that we have to uplift to Fx26.  
String or IDL/UUID changes made by this patch: n/a

Comment 16

[Ed Morley [:emorley]]

Backed out for various test failures:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=0117133ba8ef

remote:   https://hg.mozilla.org/integration/fx-team/rev/5bafaa6987eb
remote:   https://hg.mozilla.org/integration/fx-team/rev/2c803fb352c9

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Use clearEditText() and enterText() to input URLs (r=gbrown)

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 824695 [details] [diff] [review]
Use clearEditText() and enterText() to input URLs (r=gbrown)

This patch changes BaseTest to enter URLs without relying on input injection. Fixes the weird test failures with the patches for this bug (and potentially a few other intermittent failures).

Comment 19

[Lucas Rocha (:lucasr)]

Here's the try run: https://tbpl.mozilla.org/?tree=Try&rev=e0fc21a823ad

Comment 20

[Geoff Brown [:gbrown]]

Comment on attachment 824695 [details] [diff] [review]
Use clearEditText() and enterText() to input URLs (r=gbrown)

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

It would be nice to understand those failures. The sendKeys() approach worked reliably for a long time. But I don't think we gain anything by using sendKeys() for all tests, and I note that it is still used in some tests (testInputUrlBar, testSearchSuggestions, etc), so I think this change is fine.

Comment 21

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/b2g-inbound/rev/053a33ad3c5a
https://hg.mozilla.org/integration/b2g-inbound/rev/fbfc523379ee
https://hg.mozilla.org/integration/b2g-inbound/rev/e6eb8518fcd3

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/053a33ad3c5a
https://hg.mozilla.org/mozilla-central/rev/fbfc523379ee
https://hg.mozilla.org/mozilla-central/rev/e6eb8518fcd3

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 820393 [details] [diff] [review]
Factor out method to enable/disable buttons (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: All types of problems such as bug 928589 and bug 928592
Testing completed (on m-c, etc.): Landing in m-c now. Let's bake it for 1-2 days in Nightly and then uplift it.
Risk to taking this patch (and alternatives if risky): It's not a minor fix. But it's critical enough that we have to uplift to Fx26.  
String or IDL/UUID changes made by this patch: n/a

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 820394 [details] [diff] [review]
Disable toolbar elements while in editing mode on tablets (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: All types of problems such as bug 928589 and bug 928592
Testing completed (on m-c, etc.): Landing in m-c now. Let's bake it for 1-2 days in Nightly and then uplift it.
Risk to taking this patch (and alternatives if risky): It's not a minor fix. But it's critical enough that we have to uplift to Fx26.  
String or IDL/UUID changes made by this patch: n/a

Comment 26

[u421692]

Verified as fixed on Samsung Galaxy Tab (Android 4.0.4), on Nightly 28.0a1 (2013-11-03).

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7692391e5627
https://hg.mozilla.org/releases/mozilla-aurora/rev/52f0310d305a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ee7d585fd16

https://hg.mozilla.org/releases/mozilla-beta/rev/18842bec9e92
https://hg.mozilla.org/releases/mozilla-beta/rev/716ebea3ead7
https://hg.mozilla.org/releases/mozilla-beta/rev/62e3ab6ab188

Comment 28

[u421692]

Verified as fixed on Samsung Galaxy Tab (Android 4.0.4)
- on Aurora 27.0a2 (2013-11-04)
- on Firefox 26 Beta 2

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/18842bec9e92
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/716ebea3ead7
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/62e3ab6ab188