1000149 - Simplify edit mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

We got talking on IRC and realized we could probably streamline the look of our new edit mode title bar a bit. This bug is about

* Replacing the X with a smaller one 
* Removing the vertical pipe divider
* Removing the GO arrow in the URL bar and letting the text take up the whole field. 

Mockup: http://cl.ly/image/3A2W183B180C

Assets to follow momentarily

Comment 1

[Lucas Rocha (:lucasr)]

Mike, could you have a look at this?

Comment 2

[Ian Barlow (:ibarlow)]

Hm so I just looked at the assets I produced originally for this and compared them to screenshots of the app, and we're already scaling them in some weird way in the title bar... 

Instead of posting new assets, Mike I would suggest scaling their size down in code by roughly 20%

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Ian Barlow (:ibarlow) from comment #0)
> * Removing the GO arrow in the URL bar and letting the text take up the
> whole field. 

So to be clear, we're forgoing the go button telemetry in bug 998000 in favor of just removing it now?

Comment 4

[Ian Barlow (:ibarlow)]

... Yes.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Remove go button from toolbar.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

For part 1 (comment 5), note:

* I removed a redundant line to select the url bar on editing mode entry
* I added a line to deselect the url bar on editing mode exit
* I kept both the image and string resources used by the go button because I figured they could be useful later

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Remove edit separator.

Note that the auto-generated file, ThemedView, is no longer used, but could be useful in the future so I left it.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #6)
> * I added a line to deselect the url bar on editing mode exit

Nevermind, I do this in part 3.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Update close button size and toolbar spacing.

Did as Ian suggested and made the drawable resized in code (comment 2). This seems inefficient though - are we sure this is how we want to ship, or is this just for testing purposes?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patched browser toolbar

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

apk: https://people.mozilla.org/~mcomella/apks/mcomella-1000149_01.apk

Ian, what do you think?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Lucas, note also that the url bar no longer needs to shrink for phones (at least on the configurations I've tested) so do you want me to yank that code?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try: https://tbpl.mozilla.org/?tree=Try&rev=34f3c6978f9e

Comment 14

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

(In reply to Michael Comella (:mcomella) from comment #12)
> Lucas, note also that the url bar no longer needs to shrink for phones (at
> least on the configurations I've tested) so do you want me to yank that code?

What kind of phones did you test with? Let's just make sure we code the mdpi sizes or other small screen phones.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8411501 [details] [diff] [review]
Part 1: Remove go button from toolbar.

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

The edit layout was only necessary because the editing mode had a ToolbarEditText and the go button. Now edit layout is just the ToolbarEditText, we don't need the extra container for it anymore. We should either use ToolbarEditText directly in BrowserToolbar or make ToolbarEditLayout a ToolbarEditText subclass. Please file a follow-up.

It seems you're mixing a change in how we handle selected state in toolbar with the change to remove the go button. Please avoid that in future patches.

It seems you forgot to remove the respective icons that were only used in the go button?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ -1021,5 @@
>          if (isAnimatingEntry)
>              return;
>  
> -        // Highlight the toolbar from the start of the animation.
> -        setSelected(true);

How is this change related to the go button?

::: mobile/android/base/toolbar/ToolbarEditLayout.java
@@ +75,5 @@
>                 (InputMethodManager) getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>          imm.showSoftInput(mEditText, InputMethodManager.SHOW_IMPLICIT);
>      }
>  
> +    void prepareShowAnimation(final PropertyAnimator animator) {

Why did you have to remove prepareAnimation(boolean, PropertyAnimator) in this patch? Because we don't need prepareHideAnimation() anymore?

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8411505 [details] [diff] [review]
Part 2: Remove edit separator.

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

Nice.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8411506 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

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

Let's either use an image with the proper size or use ScaleDrawable. I'd prefer the former.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +248,5 @@
> +        final Bitmap editCancelBitmap =
> +                ((BitmapDrawable) getResources().getDrawable(R.drawable.close_edit_mode)).getBitmap();
> +        final int dim = (int) (editCancelBitmap.getWidth() * .8);
> +        final Bitmap resizedEditCancelBitmap = Bitmap.createScaledBitmap(editCancelBitmap, dim, dim, true);
> +        editCancel.setImageBitmap(resizedEditCancelBitmap);

You can just use a ScaleDrawable instead. But I'd prefer to just have the image in the right size: simpler and no code change involved.

@@ +1118,5 @@
>          updateProgressVisibility();
>  
> +        // The animation looks cleaner if the text in the URL bar is
> +        // not selected so clear the selection by clearing focus.
> +        urlEditLayout.clearFocus();

How is that related to resizing the 'X' button?

Comment 18

[Lucas Rocha (:lucasr)]

After trying the APK, I noticed that the size of the entry jumps a few pixels after the animation to exit edit mode. It would be nice if you could fix it as part of this bug (or on a follow-up, if it's unrelated to the UI changes done here).

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #12)
> Lucas, note also that the url bar no longer needs to shrink for phones (at
> least on the configurations I've tested) so do you want me to yank that code?

I'd like to keeping the code for shrinking the urlbar as it's more future-proof i.e. just in case some obscure device ends up needing it.

Comment 20

[Ian Barlow (:ibarlow)]

Nice, this is a lot cleaner visually. 

I'm noticing one issue though. Sometimes, when I tap the URL bar the expanded highlighted url appears *before* the animation is done, which kind of breaks the perception of the text being contained by the url field. 

Take a look at this video, http://cl.ly/2b0s2T0B3b2r, it's most evident the second and third time I tap the url bar.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> The edit layout was only necessary because the editing mode had a
> ToolbarEditText and the go button. ... Please file a follow-up.

bug 1001084.

> It seems you forgot to remove the respective icons that were only used in
> the go button?

Intentional (see comment 6), but I'll reupload without them.

> Why did you have to remove prepareAnimation(boolean, PropertyAnimator) in
> this patch? Because we don't need prepareHideAnimation() anymore?

Yep. Since I added this so recently, it made me realize how if I didn't do this, it'd just be a useless indirection. Do this enough times and the code generally becomes more difficult to follow, so it seems best to clean these up as I hit them.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Remove go button from toolbar.

Fixed nit: removed the "ic_url_bar_go" assets.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Update close button size and toolbar spacing.

* Replaced the image assets with smaller ones (20% reduced) via Image Magick:
`convert <in> -resize 80% <out>
* Fixed the animation issue in comment 18 - there was a paddingRight I missed
on the translating edge to match the url bar entry. To simplify the code, I
removed it, and added those pixels into marginRight instead - margin vs.
padding seems to be irrelevant in an ImageView.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Correct url bar shrinking documentation.

Updated the documentation on shrinking because it specifically referenced the menu/tab layout, when that's actually not relevant anymore.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Hide edit layout highlight until the animation completes.

Fixed the animation as per comment 20.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

normal APK: https://people.mozilla.com/~mcomella/apks/mcomella-1000149_02.apk
slow APK: https://people.mozilla.org/~mcomella/apks/mcomella-1000149_03.apk

Ian, what do you think?

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Hide edit layout highlight until the animation completes.

TextView.getHighlightColor was API 16 and up - this updated patch fixes the issue.

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I just want to note that I discovered bug 1001243 while testing for this patch, though I believe it exists because of bug 965548, not this one.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8412194 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

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

Looks generally good. Giving f+ to get answers to my questions before going ahead.

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +106,5 @@
>                 android:contentDescription="@string/edit_mode_cancel"
>                 android:visibility="invisible"/>
>  
> +    <!-- The left margin of the cancel button is larger than the right because
> +         the url bar drawable contains some whitespace, so we compensate by adding

Maybe we should just fix the drawable then? We should not be compensating drawable issues in our layout files. Please file a follow-up.

@@ +117,3 @@
>                    android:paddingLeft="8dp"
>                    android:paddingRight="8dp"
> +                  android:visibility="invisible"/>

What's this change about? Not sure I follow how it related to reducing the 'x' button size and updating margins.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +1104,5 @@
>          updateProgressVisibility();
>  
> +        // The animation looks cleaner if the text in the URL bar is
> +        // not selected so clear the selection by clearing focus.
> +        urlEditLayout.clearFocus();

This will remove the focus state from the entry 'border' and remove any text selection in the edittext before animating the toolbar out of editing mode, right?

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 8412195 [details] [diff] [review]
Part 4: Correct url bar shrinking documentation.

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

Good.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8412228 [details] [diff] [review]
Part 5: Hide edit layout highlight until the animation completes.

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

Patch looks good. But I'm starting to think we shouldn't really cross-fade the edit and display layouts when entering/exiting editing mode. The edit mode is clearly overflowing its 'perceived' boundaries during the transitions. Maybe we should try:
1. Only show the edit layout once the animation to enter edit mode ends.
2. Show display layout immediately before the animation to exit edit mode starts.

Giving f+ to get some discussion going.

Comment 32

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #31)

> Patch looks good. But I'm starting to think we shouldn't really cross-fade
> the edit and display layouts when entering/exiting editing mode. The edit
> mode is clearly overflowing its 'perceived' boundaries during the
> transitions. Maybe we should try:
> 1. Only show the edit layout once the animation to enter edit mode ends.
> 2. Show display layout immediately before the animation to exit edit mode
> starts.
> 

Yep, this. 

Thanks for the slow-mo build, Mike, that was extremely helpful. I think if we add Lucas's suggestions we should be good to go.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #29)
> Maybe we should just fix the drawable then? We should not be compensating
> drawable issues in our layout files. Please file a follow-up.

bug 1001435.

> @@ +117,3 @@
> >                    android:paddingLeft="8dp"
> >                    android:paddingRight="8dp"
> > +                  android:visibility="invisible"/>
> 
> What's this change about? Not sure I follow how it related to reducing the
> 'x' button size and updating margins.

Having the edit toolbar layout post-layout is necessary for getting the urlBarEntryTranslation (e.g. [1]). I'm not sure how to force this in code.

The comment above edit_cancel addresses this (which is also invisible), and formerly the edit separator had similar restrictions.

It doesn't specifically deal with updating margins, but it deals with updating the spacing of the toolbar when expanded.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=2ca6c9728ed9#602

> This will remove the focus state from the entry 'border' and remove any text
> selection in the edittext before animating the toolbar out of editing mode,
> right?

Correct.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I have a few specific questions regarding the guidelines you mentioned in comment 31 (e.g. "pop-in?", "crossfade?", "when?"), but instead I figure I should just use my best judgment and put up a few builds if I'm in question? I'm not sure what the best practices for dealing with UX changes are.

(In reply to Lucas Rocha (:lucasr) from comment #31)
> 2. Show display layout immediately before the animation to exit edit mode
> starts.

This may cause problems with shrinking url bars, though I don't think this should happen on any current phone devices and tablets will be fine because they shrink in the opposite direction.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 5: Change edit/display layout visibility patterns on editing mode transitions.

Ignore comment 34 - it looks pretty good the way you asked for it.

apk: https://people.mozilla.com/~mcomella/apks/mcomella-1000149_04.apk

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Ian, what do you think of the build in comment 35?

Comment 37

[Ian Barlow (:ibarlow)]

(In reply to Michael Comella (:mcomella) from comment #36)
> Ian, what do you think of the build in comment 35?

Much better, thanks Mike! :)

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 8412805 [details] [diff] [review]
Part 5: Change edit/display layout visibility patterns on editing mode transitions.

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

Nice.

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8412194 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

Responses to your f+ are in comment 33.

Comment 40

[Lucas Rocha (:lucasr)]

Comment on attachment 8412194 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

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

Go.

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/27c4e67174a2
remote:   https://hg.mozilla.org/integration/fx-team/rev/9ee5def1b780
remote:   https://hg.mozilla.org/integration/fx-team/rev/4a102d4f823f
remote:   https://hg.mozilla.org/integration/fx-team/rev/7b2397796194
remote:   https://hg.mozilla.org/integration/fx-team/rev/b76e6b4fc866

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27c4e67174a2
https://hg.mozilla.org/mozilla-central/rev/9ee5def1b780
https://hg.mozilla.org/mozilla-central/rev/4a102d4f823f
https://hg.mozilla.org/mozilla-central/rev/7b2397796194
https://hg.mozilla.org/mozilla-central/rev/b76e6b4fc866

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8412074 [details] [diff] [review]
Part 1: Remove go button from toolbar.

This request applies to parts 1-5. Note that this must be uplifted with the dependent bug 1005924.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 965548

User impact if declined:
  We'll be shipping a less desirable version (on appearances, not functionality) of the button added to the toolbar to close editing mode
 
Testing completed (on m-c, etc.):
  On m-c for ~1 wk
 
Risk to taking this patch (and alternatives if risky): 
  Medium - these patches make some significant changes to the appearance of the url bar (which could affect functionality in the worst cases), however, it is one of the most used features and there are a lot of eyes on it.

String or IDL/UUID changes made by this patch:
  None

Comment 44

[Flaviu Cos]

Verified as fixed in build 32.0a1 (2014-05-07);
Device: Motorola Razr (Android 4.0.4.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81a9f45f132b
https://hg.mozilla.org/releases/mozilla-aurora/rev/24acf0cad6f1
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ec3008b2de4
https://hg.mozilla.org/releases/mozilla-aurora/rev/9e7f42ba29cb
https://hg.mozilla.org/releases/mozilla-aurora/rev/f07eb0415d25

Comment 46

[Flaviu Cos]

Verified as fixed in build 31.0a2 (2014-05-09);
Device: Motorola Razr (Android 4.0.4).

Comment 47

[Cristina Madaras, QA [:CristinaM]]

Test case added in Moztrap:
https://moztrap.mozilla.org/manage/case/14081/ - Simplified edit mode