Closed Bug 1000149 Opened 5 years ago Closed 5 years ago

Simplify edit mode

Categories

(Firefox for Android :: General, defect)

31 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 --- verified
firefox32 --- verified
relnote-firefox --- 31+

People

(Reporter: ibarlow, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Keywords: feature)

Attachments

(6 files, 4 obsolete files)

9.86 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
161.46 KB, image/png
Details
14.10 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
11.21 KB, patch
lucasr
: review+
lucasr
: feedback+
Details | Diff | Splinter Review
1.75 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
2.87 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
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
Mike, could you have a look at this?
Assignee: lucasr.at.mozilla → michael.l.comella
Flags: needinfo?(michael.l.comella)
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%
(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?
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella) → needinfo?(ibarlow)
... Yes.
Flags: needinfo?(ibarlow)
OS: Mac OS X → Android
Hardware: x86 → All
Attachment #8411501 - Flags: review?(lucasr.at.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
Note that the auto-generated file, ThemedView, is no longer used, but could be useful in the future so I left it.
Attachment #8411505 - Flags: review?(lucasr.at.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.
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?
Attachment #8411506 - Flags: review?(lucasr.at.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?
(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 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?
Attachment #8411501 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8411505 [details] [diff] [review]
Part 2: Remove edit separator.

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

Nice.
Attachment #8411505 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #8411506 - Flags: review?(lucasr.at.mozilla) → feedback+
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).
(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.
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.
Flags: needinfo?(ibarlow)
(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.
Fixed nit: removed the "ic_url_bar_go" assets.
Attachment #8411501 - Attachment is obsolete: true
* 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.
Attachment #8412194 - Flags: review?(lucasr.at.mozilla)
Attachment #8411506 - Attachment is obsolete: true
Updated the documentation on shrinking because it specifically referenced the menu/tab layout, when that's actually not relevant anymore.
Attachment #8412195 - Flags: review?(lucasr.at.mozilla)
Fixed the animation as per comment 20.
Attachment #8412196 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(ibarlow)
TextView.getHighlightColor was API 16 and up - this updated patch fixes the issue.
Attachment #8412228 - Flags: review?(lucasr.at.mozilla)
Attachment #8412196 - Attachment is obsolete: true
Attachment #8412196 - Flags: review?(lucasr.at.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.
Depends on: 965548
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?
Attachment #8412194 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 8412195 [details] [diff] [review]
Part 4: Correct url bar shrinking documentation.

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

Good.
Attachment #8412195 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8412228 - Flags: review?(lucasr.at.mozilla) → feedback+
(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.
Flags: needinfo?(ibarlow)
(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.
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.
Ian, what do you think of the build in comment 35?
Flags: needinfo?(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! :)
Flags: needinfo?(ibarlow)
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.
Attachment #8412805 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8412228 - Attachment is obsolete: true
Comment on attachment 8412194 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

Responses to your f+ are in comment 33.
Attachment #8412194 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8412194 [details] [diff] [review]
Part 3: Update close button size and toolbar spacing.

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

Go.
Attachment #8412194 - Flags: review?(lucasr.at.mozilla) → review+
Depends on: 1005924
Flags: in-moztrap?(fennec)
Keywords: feature
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
Attachment #8412074 - Flags: approval-mozilla-aurora?
Attachment #8412074 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build 32.0a1 (2014-05-07);
Device: Motorola Razr (Android 4.0.4.
Verified as fixed in build 31.0a2 (2014-05-09);
Device: Motorola Razr (Android 4.0.4).
Status: RESOLVED → VERIFIED
Test case added in Moztrap:
https://moztrap.mozilla.org/manage/case/14081/ - Simplified edit mode
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.