Closed Bug 997477 Opened 10 years ago Closed 10 years ago

Add a way to close edit mode in one tap on tablet

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files, 8 obsolete files)

5.86 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
18.73 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.85 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
As per bug 965548 comment 90 and bug 965548 comment 91, we're splitting this functionality out from bug 965548 (the phone implementation), provided we land it quickly.

See https://bug965548.bugzilla.mozilla.org/attachment.cgi?id=8367611 for mocks.
Component: General → Awesomescreen
Perhaps the tablet changes only seemed overly complicated because they were shoved into one patch, so I'm breaking them apart and uploading them. This patch breaks out the shared animation listener code, so we only need to create one object for each of start and stop editing, and don't need one for phone and one for tablet.
Attachment #8409822 - Flags: review?(lucasr.at.mozilla)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
WIP: Major tablet animation changes. Note that this currently doesn't work because I need to move the URL bar, but this is the main idea. I'll try a few quick hacks but if I can't get anything going quickly, I'll wait for your feedback.
We have a translating edge on all devices, so this patch removes null checks.
Attachment #8409836 - Flags: review?(lucasr.at.mozilla)
Further break out some shared animation code.
Attachment #8409839 - Flags: review?(lucasr.at.mozilla)
Attachment #8409835 - Flags: feedback?(lucasr.at.mozilla)
It's a bit messy to do the animation we had in the previous bug to exit editing mode (crossfading the longer display url bar with the shorter editing one) without a third view so I'm going to hold off until I get some feedback as to whether or not we want to continue this approach.
Blocks: 999211
No longer depends on: 999211
Attachment #8409836 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8409822 [details] [diff] [review]
Part 1: Factor out show/stopEditingAnimationListeners.

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

Good call.
Attachment #8409822 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8409835 [details] [diff] [review]
Part 3: Add tablet editing mode cancel button.

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

Could I have a look at a build? Just want to understand the actual UI behaviour to have more context about what this patch is doing. Overall, I like how you clearly split the tablet-specific code paths from the phone UI. However, we probably want to split the code base for tablets and phones to make it easier to follow. I'm still a bit concerned about the added complexity here but I think we can plan for a series of follow-up refactorings to mitigate it.
Attachment #8409835 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Attachment #8409839 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #7)
> Could I have a look at a build? Just want to understand the actual UI
> behaviour to have more context about what this patch is doing.

As I mentioned in comment 5, the animation we had previously is screwed up here because we removed the third view. From your comments, it seems this approach seems reasonable so I'll work to correct it, and post a build.
This is the same exiting animation we made before with three views.

Build: https://people.mozilla.com/~mcomella/apks/mcomella-997477_01.apk
Slo-mo build: https://people.mozilla.com/~mcomella/apks/mcomella-997477_02.apk

Note that entering a new URL while in editing mode (rather than canceling) is
broken - I'll fix it in part 5.
Attachment #8428130 - Flags: review?(lucasr.at.mozilla)
Ian, what do you think of the builds in comment 10?
Flags: needinfo?(ibarlow)
Forgot to hg add.
Attachment #8428150 - Flags: review?(lucasr.at.mozilla)
Attachment #8428130 - Attachment is obsolete: true
Attachment #8428130 - Flags: review?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #10)
> Note that entering a new URL while in editing mode (rather than canceling) is
> broken - I'll fix it in part 5.

This seems to work correctly with parts 1 - 4. I'm not sure why this might not have been working before.
Comment on attachment 8428150 [details] [diff] [review]
Part 3: Add tablet editing mode cancel button.

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

This is looking pretty good. Before I give an r+, can I have a look at a build with these patches applied?

::: mobile/android/base/resources/drawable/url_bar_translating_edge.xml
@@ +5,5 @@
>  
>  <clip xmlns:android="http://schemas.android.com/apk/res/android"
>        android:drawable="@drawable/url_bar_entry"
>        android:clipOrientation="horizontal"
> +      android:gravity="right"/>

Unrelated change?

::: mobile/android/base/resources/drawable/url_bar_translating_edge_tablet.xml
@@ +1,4 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Can't you just reuse the phone's translating edge drawable? If not, why?
Attachment #8428150 - Flags: review?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #14)
> This is looking pretty good. Before I give an r+, can I have a look at a
> build with these patches applied?

comment 10.

> Unrelated change?

Yes, but, being a largerly untouched file, it isn't likely to be a problem in a backout (like large whitespace changes) so I didn't think managing the extra patch in the queue was worth it - do you disagree?

> Can't you just reuse the phone's translating edge drawable? If not, why?

We can't show both edges of the translating drawable for obvious reasons, so, from my understanding, we need to set the gravity to side that is translating to show the appropriate edge. On tablet, we shrink left, hence gravity=left. On phone, we expand right, hence gravity=right. It does not appear possible to set this attribute through style attributes (no style on Drawables), nor to set it dynamically [1], so we're forced to use two drawables.

[1]: https://developer.android.com/reference/android/graphics/drawable/ClipDrawable.html#attr_android:gravity
I moved the start/stopEditingWithAnyDeviceAnimation outside each of the respective tablet/phone start/stopEditing. This should probably be in part 2, but I didn't want to waste time with the rebase.
Attachment #8430374 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8430374 [details] [diff] [review]
Part 3: Add tablet editing mode cancel button.

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

This is looking good. Has this been approved by ibarlow/antlam yet? Given that the phone UI changes in bug 965548 involved fixing a few fallouts after they landed in m-c, are you confident that this is something you want to land a ~1 week before the merge? Just wondering.

::: mobile/android/base/resources/drawable/url_bar_translating_edge_tablet.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

If we actually need a different drawable for the tablet UI, let's just let the resources system do its thing and have this drawable in drawable-large-v11/url_bar_translating_edge.xml instead (analogous to what we with browser_toolbar.xml).
Attachment #8430374 - Flags: review?(lucasr.at.mozilla) → review+
Tried the apk. I think this needs more iterations. It's hard to understand what the animation is actually 'doing' spatially i.e. the entry shrinks but the rest of the elements fade out. It doesn't 'make sense' visually. Also, the space between the 'x' and the entry looks a bit arbitrary.

I like the added clarity of the toolbar when in editing mode through. I almost feel like we should just snap between states on tablets.

Thoughts?
(In reply to Lucas Rocha (:lucasr) from comment #19)
> Tried the apk. I think this needs more iterations. It's hard to understand
> what the animation is actually 'doing' spatially i.e. the entry shrinks but
> the rest of the elements fade out. It doesn't 'make sense' visually. Also,
> the space between the 'x' and the entry looks a bit arbitrary.
> 
> I like the added clarity of the toolbar when in editing mode through. I
> almost feel like we should just snap between states on tablets.

I kind of agree. There's a lot going on here, and it's difficult to imagine what transition or combination of transitions we could even use to make it look right. Want to try a simple one that just snaps? Or has like a quick 200ms crossfade?
Flags: needinfo?(ibarlow)
Ian, what do you think?

Slow-mo crossfade looks pretty terrible: https://people.mozilla.org/~mcomella/apks/mcomella-997477_03.apk

There are some refinements that could be made, but I don't think they'd help.

Pop-in: https://people.mozilla.org/~mcomella/apks/mcomella-997477_04.apk

Technically, this is with a duration of 1ms, to save development time, so that's why there's a bit of delay/jank after hitting the button.

I'd say pop-in is better.
Flags: needinfo?(ibarlow)
(In reply to Michael Comella (:mcomella) from comment #21)

> I'd say pop-in is better.

yeah, i agree.
Flags: needinfo?(ibarlow)
Here's a non-hacky build: https://people.mozilla.com/~mcomella/apks/mcomella-997477_05.apk

Ian, what do you think?
Flags: needinfo?(ibarlow)
Changed names to *EditingPhoneAnimationListener as these are only used on phone now.
Attachment #8431905 - Flags: review?(lucasr.at.mozilla)
Removed part 2 as animation on any device is no longer relevant. This patch creates a pop-in transition for tablet.
Attachment #8431910 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8431905 [details] [diff] [review]
Part 1: Factor out show/stopEditingAnimationListeners.

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

Yep.
Attachment #8431905 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8431910 [details] [diff] [review]
Part 2: Add tablet editing mode cancel button.

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

Looking pretty good. Please remove updateChildrenEnabledStateForEditing() if it's not actually needed anymore.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +934,3 @@
>       * related to editing mode.
>       */
> +    private void updateChildrenEnabledStateForEditing() {

Do we still need this method? It's only used to disable toolbar elements on editing mode but now you're hiding all of them anyway.
Attachment #8431910 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8431937 [details] [diff] [review]
Part 3: Remove guards around urlBarTranslatingEdge in appropriate configs.

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

Good.
Attachment #8431937 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Michael Comella (:mcomella) from comment #23)
> Here's a non-hacky build:
> https://people.mozilla.com/~mcomella/apks/mcomella-997477_05.apk
> 
> Ian, what do you think?

Looks good to me.
Flags: needinfo?(ibarlow)
(In reply to Lucas Rocha (:lucasr) from comment #28)
> Do we still need this method? It's only used to disable toolbar elements on
> editing mode but now you're hiding all of them anyway.

No - nice catch! However, removing it was more complicated than I hoped - see bug 1019127.

remote:   https://hg.mozilla.org/integration/fx-team/rev/ba7fcac04c5b
remote:   https://hg.mozilla.org/integration/fx-team/rev/3e87d48d9f42
remote:   https://hg.mozilla.org/integration/fx-team/rev/0e6f1d91ca26
On Firefox for Android 31 Beta 4, the cancel edit mode button is available just on phones, on tablets does not appear.
Will this be uplifted?
Too risky for mid-beta for such a hefty UI change.
I agree with Aaron - this was never intended to be uplifted because it is a much bigger change on tablets than it is on phone.
Depends on: 1047264
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: