935473 - Exiting editing mode while forward button is visible might leave it visible and disabled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Our current UI never shows a disabled forward button in the toolbar. The assumption here is that the forward button will be hidden whenever the action is not available.

However, with the introduction of editing mode in the toolbar (as part of the new about:home stuff), we might end up with a visible forward button in disabled state.

Steps to reproduce:
- Open fennec, go to any URL
- Go back to previous page (forward button will slide in now)
- Tap on any link and enter editing mode while the next page loads (forward button is still visible here, in disabled state while editing)
- Exit editing mode (e.g. press Android's BACK button).

Expected results: forward button slides away
Actual results: forward button stays visible with disabled state

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Replace ViewGroup.MarginLayoutParams with MarginLayoutParams (r=sriram)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Avoid unnecessary animations in forward button (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Factor out method to animate forward button's visibility (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Update forward button visibility when exiting editing mode (r=wesj)

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 828040 [details] [diff] [review]
Avoid unnecessary animations in forward button (r=wesj)

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

::: mobile/android/base/BrowserToolbar.java
@@ +1664,5 @@
>          if (mForward.getVisibility() != View.VISIBLE)
>              return;
>  
> +        MarginLayoutParams fwdParams = (MarginLayoutParams) mForward.getLayoutParams();
> +        if ((fwdParams.leftMargin > 0 && enabled) || (fwdParams.leftMargin == 0 && !enabled)) {

Can you add a little comment about what this does?

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

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

This is mostly a rename it looks like

::: mobile/android/base/BrowserToolbar.java
@@ +1652,5 @@
>      public void updateBackButton(boolean enabled) {
>          setButtonEnabled(mBack, enabled);
>      }
>  
> +    private void animateForwardButtonVisibility(final boolean visible) {

Since you're changing this, don't pass booleans. Also, animateForwardButton seems more concise to me. TBH, I'd like to move all this into a forwardButton class and clean it out of the toolbar...

@@ +1715,4 @@
>          mForwardAnim.start();
>      }
>  
> +    public void updateForwardButton(boolean enabled) {

We should probably change this as well. Or maybe just rename? setForwardEnabled(boolean) I could live with.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 828042 [details] [diff] [review]
Update forward button visibility when exiting editing mode (r=wesj)

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

Hmm... part of me wants updateForwardButton to take a parameter for this, but I just suggested we change it to setForwardButtonEnabled(bool) which makes that less appealing.... I think we need to better doc what's going on. Maybe it should be setAndAnimateForwardEnabled(bool) to make it clear what's going on....

::: mobile/android/base/BrowserToolbar.java
@@ +1344,5 @@
>              setButtonEnabled(mForward, canDoForward(tab));
> +
> +            if (!mIsEditing) {
> +                animateForwardButtonVisibility(canDoForward(tab));
> +            }

This needs a comment explaining what we're doing and why...

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #5)
> Comment on attachment 828040 [details] [diff] [review]
> Avoid unnecessary animations in forward button (r=wesj)
> 
> Review of attachment 828040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1664,5 @@
> >          if (mForward.getVisibility() != View.VISIBLE)
> >              return;
> >  
> > +        MarginLayoutParams fwdParams = (MarginLayoutParams) mForward.getLayoutParams();
> > +        if ((fwdParams.leftMargin > 0 && enabled) || (fwdParams.leftMargin == 0 && !enabled)) {
> 
> Can you add a little comment about what this does?

Done.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 828041 [details] [diff] [review]
> Factor out method to animate forward button's visibility (r=wesj)
> 
> Review of attachment 828041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is mostly a rename it looks like
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1652,5 @@
> >      public void updateBackButton(boolean enabled) {
> >          setButtonEnabled(mBack, enabled);
> >      }
> >  
> > +    private void animateForwardButtonVisibility(final boolean visible) {
> 
> Since you're changing this, don't pass booleans.

I don't follow. Don't pass booleans where? The point of this patch is to split the animation bits out of updateForwardButton(). I still need the boolean argument, no?

> Also, animateForwardButton
> seems more concise to me. TBH, I'd like to move all this into a
> forwardButton class and clean it out of the toolbar...

animateForwardButton() seems better indeed, done. I'm planning to factor out the back/forward bits from the toolbar as part of my refactorings under bug 917891.

> @@ +1715,4 @@
> >          mForwardAnim.start();
> >      }
> >  
> > +    public void updateForwardButton(boolean enabled) {
> 
> We should probably change this as well. Or maybe just rename?
> setForwardEnabled(boolean) I could live with.

updateForwardButton() doesn't only set the forward button's 'enabled' state. It also animates it in and out. I feel like updateForwardButton() better matches the intent of the method.

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 828042 [details] [diff] [review]
> Update forward button visibility when exiting editing mode (r=wesj)
> 
> Review of attachment 828042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm... part of me wants updateForwardButton to take a parameter for this,
> but I just suggested we change it to setForwardButtonEnabled(bool) which
> makes that less appealing.... I think we need to better doc what's going on.
> Maybe it should be setAndAnimateForwardEnabled(bool) to make it clear what's
> going on....

I agree updateForwardButton might sound a bit generic/vague but it at least doesn't imply that it only sets the 'enabled' state on the button. setAndAnimateForwardEnabled() sounds a bit weird :-) Naming is hard...

> ::: mobile/android/base/BrowserToolbar.java
> @@ +1344,5 @@
> >              setButtonEnabled(mForward, canDoForward(tab));
> > +
> > +            if (!mIsEditing) {
> > +                animateForwardButtonVisibility(canDoForward(tab));
> > +            }
> 
> This needs a comment explaining what we're doing and why...

Good point. Done.

Comment 11

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> I don't follow. Don't pass booleans where? The point of this patch is to
> split the animation bits out of updateForwardButton(). I still need the
> boolean argument, no?
I meant don't pass boolean arguments to methods. They're vague. i.e. given: animateForwardButtonVisibility(true), what does the true mean? We could do an enum, or even something like  animateForwardButtonVisibility(View.VISIBLE) is clearer.

> updateForwardButton() doesn't only set the forward button's 'enabled' state.
> It also animates it in and out. I feel like updateForwardButton() better
> matches the intent of the method.

Heh. I don't have any idea what updateForwardButton(enabled) does. I don't have any better ideas off the top of my head now either. I guess that means it lives on. We could try documenting the method (we should probably do that anyway :) ), but that doesn't honestly help someone better understand what's going on when they see this used somewhere else in the code.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> (In reply to Lucas Rocha (:lucasr) from comment #9)
> > I don't follow. Don't pass booleans where? The point of this patch is to
> > split the animation bits out of updateForwardButton(). I still need the
> > boolean argument, no?
> I meant don't pass boolean arguments to methods. They're vague. i.e. given:
> animateForwardButtonVisibility(true), what does the true mean? We could do
> an enum, or even something like 
> animateForwardButtonVisibility(View.VISIBLE) is clearer.

Fair enough. Let me send these refactorings in a separate patchset because I'd like to keep the upliftable patches as small as possible. More patches coming in a minute.
 
> > updateForwardButton() doesn't only set the forward button's 'enabled' state.
> > It also animates it in and out. I feel like updateForwardButton() better
> > matches the intent of the method.
> 
> Heh. I don't have any idea what updateForwardButton(enabled) does. I don't
> have any better ideas off the top of my head now either. I guess that means
> it lives on. We could try documenting the method (we should probably do that
> anyway :) ), but that doesn't honestly help someone better understand what's
> going on when they see this used somewhere else in the code.

Ditto.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Add comment explaining animateForwardButton's behaviour (r=wesj)

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Change animateForwardButton() to take an animation argument (r=wesj)

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

Re-requesting review after clarifications. I'll cover the review comments in the following set of patches to keep the upliftable patches as simple as possible.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8338495 [details] [diff] [review]
Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)

Changing updateBackButton() and updateForwardButton() to get a Tab argument (instead of a vague boolean) makes the 'contract' a bit easier to read.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8338496 [details] [diff] [review]
Add comment explaining animateForwardButton's behaviour (r=wesj)

Just a bit more clarification in animateForwardButton().

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8338497 [details] [diff] [review]
Change animateForwardButton() to take an animation argument (r=wesj)

Change animateForwardButton to get an enum argument instead of a boolean. Easier to read and follow.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

I'm assuming this is something you're rushing to get ready in time for FF26 but we're about to go to build on our last Beta next Monday - if this is something you want to nominate for approval and uplift please make sure the risk/reward and the testing is very thorough in the nomination form so we can weigh this well against leaving it to ride with FF27.  From an outsider's perspective this looks like a lot of churn to put in a final beta & RC without time for user feedback.

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to [e-mail] [:lsblakk] from comment #20)
> I'm assuming this is something you're rushing to get ready in time for FF26
> but we're about to go to build on our last Beta next Monday - if this is
> something you want to nominate for approval and uplift please make sure the
> risk/reward and the testing is very thorough in the nomination form so we
> can weigh this well against leaving it to ride with FF27.  From an
> outsider's perspective this looks like a lot of churn to put in a final beta
> & RC without time for user feedback.

I agree. Given how close we are to the release, I think uplifting to FF27 is a safer way to go. Re-nominating.

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

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

Sounds good. Sorry I missed this.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 8338495 [details] [diff] [review]
Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +665,5 @@
>          return getWidth() - mTabs.getLeft();
>      }
>  
>      private boolean canDoBack(Tab tab) {
> +        return (tab != null && tab.canDoBack() && !mIsEditing);

Who's passing a null tab in here? Are we wallpapering over a bug?

Comment 24

[Wesley Johnston (:wesj)]

Comment on attachment 8338496 [details] [diff] [review]
Add comment explaining animateForwardButton's behaviour (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1365,5 @@
>          setButtonEnabled(mBack, canDoBack(tab));
>      }
>  
>      private void animateForwardButton(final boolean visible) {
> +        // If the forward button is not visible, this means it's being used

There's a missing word in here "that means its <not?> being used at all in the UI"? I'd just write "If the forward button is not visible, we much be in the phone UI"

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 8338497 [details] [diff] [review]
Change animateForwardButton() to take an animation argument (r=wesj)

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

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1117,5 @@
> +                } else {
> +                    animation = ForwardButtonAnimation.HIDE;
> +                }
> +
> +                animateForwardButton(animation);

I'd just use a ternary here. animateForwardButton(canDoForward(tab) ? Animation.SHOW : Animation.HIDE);

@@ +1461,5 @@
> +        } else {
> +            animation = ForwardButtonAnimation.HIDE;
> +        }
> +
> +        animateForwardButton(animation);

Same.

Comment 26

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #25)
> Comment on attachment 8338497 [details] [diff] [review]
> Change animateForwardButton() to take an animation argument (r=wesj)
> 
> Review of attachment 8338497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +1117,5 @@
> > +                } else {
> > +                    animation = ForwardButtonAnimation.HIDE;
> > +                }
> > +
> > +                animateForwardButton(animation);
> 
> I'd just use a ternary here. animateForwardButton(canDoForward(tab) ?
> Animation.SHOW : Animation.HIDE);

Done.

> @@ +1461,5 @@
> > +        } else {
> > +            animation = ForwardButtonAnimation.HIDE;
> > +        }
> > +
> > +        animateForwardButton(animation);
> 
> Same.

Done.

Comment 27

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #24)
> Comment on attachment 8338496 [details] [diff] [review]
> Add comment explaining animateForwardButton's behaviour (r=wesj)
> 
> Review of attachment 8338496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +1365,5 @@
> >          setButtonEnabled(mBack, canDoBack(tab));
> >      }
> >  
> >      private void animateForwardButton(final boolean visible) {
> > +        // If the forward button is not visible, this means it's being used
> 
> There's a missing word in here "that means its <not?> being used at all in
> the UI"? I'd just write "If the forward button is not visible, we much be in
> the phone UI"

Fixed.

Comment 28

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #23)
> Comment on attachment 8338495 [details] [diff] [review]
> Change updateBackButton/updateForwardButton to take a Tab argument (r=wesj)
> 
> Review of attachment 8338495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +665,5 @@
> >          return getWidth() - mTabs.getLeft();
> >      }
> >  
> >      private boolean canDoBack(Tab tab) {
> > +        return (tab != null && tab.canDoBack() && !mIsEditing);
> 
> Who's passing a null tab in here? Are we wallpapering over a bug?

Yep, I had to pass null tabs in BrowserApp but it turns I can do this directly in BrowserToolbar. The null check is not necessary anymore. Fixed.

Comment 29

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/b3128b3dc02d
https://hg.mozilla.org/integration/fx-team/rev/8e6348162cc2
https://hg.mozilla.org/integration/fx-team/rev/2c343bc88009
https://hg.mozilla.org/integration/fx-team/rev/dfe673e83ebc
https://hg.mozilla.org/integration/fx-team/rev/a2e2a341e543
https://hg.mozilla.org/integration/fx-team/rev/1353c0a6159a
https://hg.mozilla.org/integration/fx-team/rev/fbc58d273b9c

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3128b3dc02d
https://hg.mozilla.org/mozilla-central/rev/8e6348162cc2
https://hg.mozilla.org/mozilla-central/rev/2c343bc88009
https://hg.mozilla.org/mozilla-central/rev/dfe673e83ebc
https://hg.mozilla.org/mozilla-central/rev/a2e2a341e543
https://hg.mozilla.org/mozilla-central/rev/1353c0a6159a
https://hg.mozilla.org/mozilla-central/rev/fbc58d273b9c

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 828039 [details] [diff] [review]
Replace ViewGroup.MarginLayoutParams with MarginLayoutParams (r=sriram)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 828040 [details] [diff] [review]
Avoid unnecessary animations in forward button (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 828041 [details] [diff] [review]
Factor out method to animate forward button's visibility (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 828042 [details] [diff] [review]
Update forward button visibility when exiting editing mode (r=wesj)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new about:home (bug 862793)
User impact if declined: The toolbar might be left into a invalid state on tablets if edit mode starts while a page is loading. 
Testing completed (on m-c, etc.): Landed on m-c. Let's bake it in Nightly for a day or two and then uplift to FF27. 
Risk to taking this patch (and alternatives if risky): Relatively low. These patches are mostly refactorings. The only functional change is to force animation to hide the forward button when you leave edit mode.   
String or IDL/UUID changes made by this patch: n/a

Comment 35

[Lucas Rocha (:lucasr)]

Ping?

Comment 36

[bhavana bajaj [:bajaj]]

NI on :lucasr to help with uplift on FF27 here so this can get into our next beta going to build MOnday(1/6).

Comment 37

[Wesley Johnston (:wesj)]

Had to do some large unbitrotting, but in these go...

https://hg.mozilla.org/releases/mozilla-beta/rev/c1cc8481e209
https://hg.mozilla.org/releases/mozilla-beta/rev/b57e3944342a
https://hg.mozilla.org/releases/mozilla-beta/rev/499a6bcb6da9
https://hg.mozilla.org/releases/mozilla-beta/rev/ea3d6f1fcaaf