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)
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.
Assignee | ||
Updated•10 years ago
|
Component: General → Awesomescreen
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
We have a translating edge on all devices, so this patch removes null checks.
Attachment #8409836 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 4•10 years ago
|
||
Further break out some shared animation code.
Attachment #8409839 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8409835 -
Flags: feedback?(lucasr.at.mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8409836 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8409839 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8409835 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Ian, what do you think of the builds in comment 10?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 12•10 years ago
|
||
Forgot to hg add.
Attachment #8428150 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8428130 -
Attachment is obsolete: true
Attachment #8428130 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
(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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
(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
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8428150 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Rebase.
Assignee | ||
Updated•10 years ago
|
Attachment #8409836 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8430375 -
Flags: review+
Comment 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
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?
Comment 20•10 years ago
|
||
(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)
Assignee | ||
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #21) > I'd say pop-in is better. yeah, i agree.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 23•10 years ago
|
||
Here's a non-hacky build: https://people.mozilla.com/~mcomella/apks/mcomella-997477_05.apk Ian, what do you think?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 24•10 years ago
|
||
Changed names to *EditingPhoneAnimationListener as these are only used on phone now.
Attachment #8431905 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8409822 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8409839 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8430374 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8430375 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Updated for the new part 2.
Attachment #8431937 -
Flags: review?(lucasr.at.mozilla)
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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 29•10 years ago
|
||
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+
Comment 30•10 years ago
|
||
(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)
Assignee | ||
Comment 31•10 years ago
|
||
(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
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba7fcac04c5b https://hg.mozilla.org/mozilla-central/rev/3e87d48d9f42 https://hg.mozilla.org/mozilla-central/rev/0e6f1d91ca26
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment 33•10 years ago
|
||
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?
Comment 34•10 years ago
|
||
Too risky for mid-beta for such a hefty UI change.
Assignee | ||
Comment 35•10 years ago
|
||
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.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•