Moving this out of bug 961749 and into its own bug. We've heard a lot of feedback that tapping the url bar puts users into a state that is confusing, given that they have to tap the back button twice to return to the page they were on before. I believe that the back button behaviour we have is correct (once to close the keyboard but leave edit mode open, and again to leave edit mode), but that we need an extra command that lets people leave edit mode in a single tap. I'd like to try doing this with a Cancel button in the title bar. If a user was on a website before they tapped the URL bar, tapping the Cancel button exits edit mode immediately and takes the user back to the page they were on previously. If a user was about:home before they tapped the URL bar, tapping the Cancel button exits edit mode immediately but still leaves them on about:home ------ On tablets, we should also change edit mode such that it hides the other action bar and tab icons, instead of greying them out. The greyed out state has also been causing a lot of confusion.

(In reply to Ian Barlow (:ibarlow) from comment #0) > On tablets, we should also change edit mode such that it hides the other > action bar and tab icons, instead of greying them out. The greyed out state > has also been causing a lot of confusion. Is there a bug for this?

No. I lumped that into this one. Feel free to spin it off into a separate bug if you like, just so long long as we land them both at the same time.

Note to self: Try to throw some UI telemetry in there. :)

I'm starting to dig into this now - Ian, are your mocks up to date?

They sure are!

Ian, is there a specific image asset you want me to use for the close button? [1] appears to already be in the repo, but I'm not sure if the one you want is slightly different. Also, what color is the seperator and the "CANCEL" text? As for the sizes (margins, icon height, etc.), I've been eyeballing as best as I can, but if you want something specific, please let me know. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi/close.png

Hey Michael, I think that icon might be a little big. Let's use these http://cl.ly/1V45362w2m0M The cancel text colour should be #222222 The separator colour should be #a6aeb4 Let me know when you have a screenshot ready and I can take a look :)

What do you think?

Looks good. Not a fan of the "go" arrow there, I guess I forgot about that bit in my mocks. Oh well. I'm going to resist my knee jerk "rip it out" comment until we get better UI telemetry in place. :( Does the icon get replaced with text when there's more room? Also, got a build I could try?

(In reply to Ian Barlow (:ibarlow) from comment #9) > Does the icon get replaced with text when there's more room? Also, got a > build I could try? Working on it - they're are issues where the first appearance is not correct, and I'm also working on the landscape version now. I'll get back to you when I have something.

I think these lines are supposed to make an alpha transition on tablets (because phones are taken care of by the edit layout transitions) but they honestly don't seem to do anything on my galaxy tab, so I thought I might take them out.

Adds the cancel button for the phone layout. Caveats: * I still need to look into ViewStubs * It does not work properly on first layout, I believe because the Go button position is used to shift the url bar, and it's not yet inflated. Any help on this would be nice. * I do not change to the "Cancel" text on landscape, as per the mockup. It does not seem we relayout on rotation - I could also use some help on this.

Comment on attachment 8396785 [details] [diff] [review] Part 3: Add cancel button for phone layout. bnicholson, does this seem like a reasonable approach? Do you have any insight on the issues in comment 14?

I noticed that part 3 messes with the tablet layout - once I figure out how the tablet works (which I'm currently working on), I'll work on making a reasonable solution that works well with both (e.g. sharing files appropriately).

It occurs to me that the separator and the cancel button should be moved into browser_toolbar, as the toolbar_edit_layout is designed to encompass the white surface of the toolbar (e.g. the tablet layout). Working on that for the phone now (which should smooth out the tablet layout issues in comment 16).

Comment on attachment 8396785 [details] [diff] [review] Part 3: Add cancel button for phone layout. Almost finished with the improved patch - cancelling feedback until that's done.

Corrected patch for phones. Note that animation is not yet implemented. Additionally, the caveat raised in comment 14 that this does not work on first layout has been fixed, however, the positioning of the white area around the go button looks a little awkward to me. This can be fixed by using the go button position (which caused the aforementioned issues) or an arbitrary constant offset.

Comment on attachment 8396853 [details] [diff] [review] Part 3: Add cancel button for phone layout. v2 What do you think of this approach?

Comment on attachment 8396760 [details] [diff] [review] Part 0: Remove unnecessary alpha settings in ToolbarEditLayout. Review of attachment 8396760 [details] [diff] [review]: ----------------------------------------------------------------- Think this is Lucas's code, so we should get him to take a look.

Comment on attachment 8396781 [details] [diff] [review] Part 2: Add "cancel" string. Review of attachment 8396781 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I think we want a more specific name here than just "cancel". Note that there are already several other definitions of "Cancel" at [1], [2], [3]. [2] is a generic "cancel" used for buttons, so I wouldn't be opposed to using that here. Otherwise, please make the name more specific (e.g., edit_mode_cancel). [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#288 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#307 [3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#416

Comment on attachment 8396853 [details] [diff] [review] Part 3: Add cancel button for phone layout. v2 Review of attachment 8396853 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/resources/layout/browser_toolbar.xml @@ +86,5 @@ > android:layout_marginTop="12dip" > android:layout_alignRight="@id/tabs"/> > > + <!-- TODO: Tablet, landscape and such. --> > + <!-- TODO: How might I use ViewStubs? --> I wouldn't worry about ViewStubs here -- we'll almost invariably end up showing this image shortly after Fennec is opened, so stubbing this wouldn't earn us anything. @@ +91,5 @@ > + <!-- TODO: Would a linear layout be more efficient? --> > + <!-- Note that the edit components are invisible so that the views > + depending on their location can properly layout. --> > + <ImageView android:id="@+id/edit_cancel" > + style="@style/UrlBar.ImageButton.Icon" Nit: align these attributes to the "android:id" above.

Opted for edit_mode_cancel because I can see where using a generic dialog cancel (i.e. button_cancel) could go wrong.

Silly me.

(In reply to Brian Nicholson (:bnicholson) from comment #23) > I wouldn't worry about ViewStubs here -- we'll almost invariably end up > showing this image shortly after Fennec is opened, so stubbing this wouldn't > earn us anything. Finkle mentioned it may help startup time, but I'll defer to your judgement.

(In reply to Michael Comella (:mcomella) from comment #26) > Finkle mentioned it may help startup time, but I'll defer to your judgement. Oh, I was simply thinking in terms of saving memory. For startup perf, my guess would be that this view/image is too small to make any measurable difference, but I don't know that for sure. Good investigation opportunity!

With animations. Still need to investigate the possibility of using linear layout.

Ian, here is an implementation for phones: https://people.mozilla.org/~mcomella/apks/mcomella-965548_01.apk Note that there is no change for landscape mode (yet). What do you think?

Ian, how should the transition occur between states? On the phone (in the build I gave you), the url bar slides out while the go button, the X, and the separator fade in. I think it might look better if they slid too (though maybe as a followup, as not to block this). I'm not sure what to do on tablet, besides maybe a crossfade of all elements.

This is really nice. :) (In reply to Michael Comella (:mcomella) from comment #30) > Ian, how should the transition occur between states? > > On the phone (in the build I gave you), the url bar slides out while the go > button, the X, and the separator fade in. I think it might look better if > they slid too (though maybe as a followup, as not to block this). I'm ok with them fading in, but I think you need to delay it a bit more. Try a current Nightly build -- see how the arrow waits until the URL bar animation is done before it appears? I think we should do roughly the same for the arrow, divider and X icon, they appear a little too early right now. > I'm not sure what to do on tablet, besides maybe a crossfade of all elements. That's what I was thinking too, yeah.

(In reply to Ian Barlow (:ibarlow) from comment #31) > I'm ok with them fading in, but I think you need to delay it a bit more. Try > a current Nightly build -- see how the arrow waits until the URL bar > animation is done before it appears? I think we should do roughly the same > for the arrow, divider and X icon, they appear a little too early right now. Sure. I noticed that the close animation on the current Nightly has the arrow clip the sliding tabs counter/menu, etc. Is this okay for the arrow, separator, and X?

Yeah, that can happen in a quicker motion as in the current Nightly.

Ian, here's a hacky tablet implementation (no animations, no forward button support): https://people.mozilla.org/~mcomella/apks/mcomella-965548_02.apk What do you think? The url bar's left edge moves away from the back button in the mocks, so I wasn't sure what to do there.

Comment on attachment 8396760 [details] [diff] [review] Part 0: Remove unnecessary alpha settings in ToolbarEditLayout. Review of attachment 8396760 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/ToolbarEditLayout.java @@ -134,5 @@ > > animator.addPropertyAnimationListener(new PropertyAnimationListener() { > @Override > public void onPropertyAnimationStart() { > - ViewHelper.setAlpha(mGo, 0.0f); If you remove these setAlpha calls, the go button will be visible while the toolbar entry expands, no?

(In reply to Michael Comella (:mcomella) from comment #34) > Ian, here's a hacky tablet implementation (no animations, no forward button > support): https://people.mozilla.org/~mcomella/apks/mcomella-965548_02.apk > > What do you think? The url bar's left edge moves away from the back button > in the mocks, so I wasn't sure what to do there. I think you might need to try to shrink the left side of the url bar a bit when it's tapped. That big gap on the left is pretty awkward looking at the moment. Also the back button icon isn't appearing on about:home in this build. Separate issue? Related?

Comment on attachment 8396760 [details] [diff] [review] Part 0: Remove unnecessary alpha settings in ToolbarEditLayout. (In reply to Lucas Rocha (:lucasr) from comment #35) > If you remove these setAlpha calls, the go button will be visible while the > toolbar entry expands, no? Okay, I see: * These calls make the go button pop in, as opposed to fade, after the url bar animation completes. * The ToolbarEditLayout fades in [1], which I originally thought was intended to include the go button, hence why I removed the calls * I mentioned I was trying to get the go button to fade in, which Ian seemed okay with in comment 31. However, he wanted it done after the url bar appears, which will complicate the code a bit so this patch is no longer relevant (and patch 3 needs to be updated). [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=a593f2a9a80c#837

Spoke with Ian: we're going to try popping in the go button, separator, and the X button. Here's a build: https://people.mozilla.org/~mcomella/apks/mcomella-965548_03.apk Ian, what do you think? Note: I noticed this is broken on devices with menu buttons so I'll work on fixing that.

Ian, is the color of the separator correct in private browsing mode?

No, let's make it dark: #222222 Otherwise this looks good. I couldn't test the tablet version as this build doesn't seem to work on my Nexus 7 :(

Notes: * The go button now pops out when exiting editing mode. * Does not handle displaying "Cancel" in landscape. * Does not properly handle devices with soft menu buttons, which will shrink the toolbar (see part 4).

This should probably have happened sooner, but hg patch queues are hard. This will be clearer with the tablet patches.

Also probably should have happened sooner.

Comment on attachment 8399731 [details] [diff] [review] Part 3: Add cancel button for phone layout. Review of attachment 8399731 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +134,5 @@ > private MenuPopup mMenuPopup; > private List<View> mFocusOrder; > > + private final GeckoBaseView editSeparator; > + private final View editCancel; Since you're adding code to a file that's already using the mX convention, please continue using that convention; mixing the old and new convention makes things harder to read. Alternatively, create a patch 0 that changes these all to the new convention and use that. @@ +589,5 @@ > mMenu.setNextFocusDownId(nextId); > } > > private int getUrlBarEntryTranslation() { > + // We could use the right-most point of the edit layout instead of the edit separator s/could/would ideally/ since "could" means we have that option, but we're choosing not to. In this case, we're not doing it because we *can't*. @@ +848,5 @@ > viewToShow.setVisibility(View.VISIBLE); > + > + final int editVisibility = (showEditLayout ? View.VISIBLE : View.INVISIBLE); > + editSeparator.setVisibility(editVisibility); > + editCancel.setVisibility(editVisibility); Nit: factor these two lines into a setCancelVisibility helper, then use here and below. ::: mobile/android/base/widget/GeckoBaseView.java.in @@ +2,5 @@ > +#define EXTENDS_VIEW 1 > +/* We name this GeckoBaseView, rather than GeckoView, to avoid > + * name conflicts with the View that hosts web content. */ > +#define VIEWTYPE BaseView > +#include GeckoView.java.frag I wouldn't make this change; these preprocessed files are already complicated enough. I don't think it's a big concern that we have two different GeckoView classes -- that's what namespaces are for. Besides, BrowserToolbar.java doesn't import o.m.g.GeckoView anyway. That said, I don't think the Gecko* prefix is a good name for these generated widgets as they have nothing to do with Gecko. If you wanted to rename them to use a different prefix (e.g., "Themed*"), I wouldn't be opposed.

Comment on attachment 8399732 [details] [diff] [review] Part 4: Shrink the url bar on appropriate phones. Review of attachment 8399732 [details] [diff] [review]: ----------------------------------------------------------------- I'd like more context about this patch. When does the URL bar need to shrink? Why doesn't Nightly require this, but your patch does? ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +966,5 @@ > } > > + final int curveTranslation = getUrlBarCurveTranslation(); > + final int entryTranslation = getUrlBarEntryTranslation(); > + shouldShrinkURLBar = (entryTranslation < 0) ? true : false; Nit: ternary is redundant; just use `entryTranslation < 0`.

Comment on attachment 8399733 [details] [diff] [review] Part 5: Rename url bar elements for clarity between phone/tablet configs. Review of attachment 8399733 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +191,5 @@ > mDefaultForwardMargin = res.getDimensionPixelSize(R.dimen.forward_default_offset); > mUrlDisplayLayout = (ToolbarDisplayLayout) findViewById(R.id.display_layout); > mUrlBarEntry = findViewById(R.id.url_bar_entry); > mUrlEditLayout = (ToolbarEditLayout) findViewById(R.id.edit_layout); > + urlBarEditFinal = findViewById(R.id.url_bar_edit_final); It'd be nice if this has parity with urlBarTranslatingEdge. What about urlBarStaticEdge/urlBarTranslatingEdge instead?

Comment on attachment 8399734 [details] [diff] [review] Part 6: Improve readability for BrowserToolbar.start/stopEditing methods. Review of attachment 8399734 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup.

(In reply to Brian Nicholson (:bnicholson) from comment #46) > I'd like more context about this patch. When does the URL bar need to > shrink? Why doesn't Nightly require this, but your patch does? On devices with hardware menu buttons, the software menu button is missing and so the url bar is a larger size. On the currently Nightly, the url bar expands to the right edge and so the url bar will always expand. However, in this patch, the close button and separator are larger than the tabs button (since menu is missing), causing the toolbar to shrink on these devices. I wrote these patches generally, rather than "if (mHardwareMenuButton)" so it'd cause less issues down the line with later changes and unforseen device configurations where this may apply. I agree that it's ambiguous and I should add a comment w/ "e.g. devices without software menu buttons".

Comment on attachment 8399731 [details] [diff] [review] Part 3: Add cancel button for phone layout. Review of attachment 8399731 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/widget/GeckoBaseView.java.in @@ +1,3 @@ > +#filter substitution > +#define EXTENDS_VIEW 1 > +/* We name this GeckoBaseView, rather than GeckoView, to avoid Consider splitting GeckoView.java.frag into two parts and conditionally including the bits you need.

Comment on attachment 8399732 [details] [diff] [review] Part 4: Shrink the url bar on appropriate phones. Review of attachment 8399732 [details] [diff] [review]: ----------------------------------------------------------------- OK, I'm going to hand r? off to lucasr for this one since he's done most of the work on the URL bar animations.

Note: Builds on the patch in bug 991256.

Rebased and fixed nits (which are also addressed in bug 991256).

Comment on attachment 8400881 [details] [diff] [review] Part 2.5: Make Themed* processing more robust. Review of attachment 8400881 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, though "suffix" sounds more natural to me than "postfix". Also, doesn't the convention normally include underscores instead of cramming the words together?

Fixed nits.

Lucas, for part 4, Brian brought up the possibility of resizing the url_bar_entry once when the url bar is tapped and then translating the url_bar_right_edge, rather than maintaining three Views (as my patch does). I think this is a more intuitive solution and should simplify the code though I wonder what the performance differences will be - do you have any ideas? I'm working on a test patch now.

Brian also had the idea of actually resizing the View, like the ToolbarProgressView [1], but we deemed that would be more expensive than the current system due to the additional `layout` calls.

Rebase.

(In reply to Michael Comella (:mcomella) from comment #57) > Brian also had the idea of actually resizing the View... during the animation.

It seems resizing url_bar_entry is messy: we use FILL_PARENT ordinarily, and by setting an explicit width to reduce the size, we remove this state and the toolbar does not fill the missing space on rotation (nevermind the ugliness to store and restore the changing margin state). Alternative: We could add a transparent spacer View, but since we're adding another View to the hierarchy, it's probably just as expensive as the three Views I use for the animation in part 4.

Ian, how does this feel for tablets? Note that the forward button behavior is not yet correct (and the underlying code is still a bit hacky): https://people.mozilla.org/~mcomella/apks/mcomella-965548_04.apk

For the record, with the build in comment 61, I find the tablet close animation to look funny, specifically the url bar translating back into position while the back button fades in. For me, the animations tend to be choppy if you don't dismiss the keyboard first so I recommend doing that to better see what I mean.

re build in comment 61: the open animation on tablets also looks a bit funny (specifically the url bar shrinking). liuche has also mentioned that the gap on the right side of the url bar, between it and the separator, looks awkward.

Hey Mike, what about experimenting with some of these techniques[1] to see if it helps us simplify the edit mode transitions and toolbar layout a bit? For instance, we can advantage of the fact that we don't animate the toolbar in pre-ICS and use some more advanced APIs here. See, for example, this little experiment I wrote here to get the gist of this transition technique here: https://gist.github.com/lucasr/9508844 [1] http://lucasr.org/2014/03/13/how-android-transitions-work/

Comment on attachment 8399732 [details] [diff] [review] Part 4: Shrink the url bar on appropriate phones. Clearing review for now.

This is looking good overall! (In reply to Michael Comella (:mcomella) from comment #62) > For the record, with the build in comment 61, I find the tablet close > animation to look funny, specifically the url bar translating back into > position while the back button fades in. For me, the animations tend to be > choppy if you don't dismiss the keyboard first so I recommend doing that to > better see what I mean. I agree. The "url bar tap" transition looks pretty good, the "leave edit mode" one is a little choppy. > liuche has also mentioned that the gap on the right side of the url bar, between it > and the separator, looks awkward. I don't really mind this. I feel like the less we animate things around in the title bar, the better.

(In reply to Lucas Rocha (:lucasr) from comment #64) > Hey Mike, what about experimenting with some of these techniques[1] to see > if it helps us simplify the edit mode transitions and toolbar layout a bit? > For instance, we can advantage of the fact that we don't animate the toolbar > in pre-ICS and use some more advanced APIs here. As I understand it, Transitions are only available in KitKat and don't appear to be available in the support libraries. That means we'd have to duplicate our animation code - one for KitKat+ and one for pre-KitKat - is that what you're asking for?

(In reply to Ian Barlow (:ibarlow) from comment #66) > I agree. The "url bar tap" transition looks pretty good, the "leave edit > mode" one is a little choppy. Here's a build that crossfades the URL bar on editing mode exit, which I think looks pretty good: https://people.mozilla.com/~mcomella/apks/mcomella-965548_05.apk However, juxtaposed against the exit animation, I think the entry anmiation looks a bit silly so here's this build with the editing change and a crossfade into editing mode as well (which I think also looks a bit silly): https://people.mozilla.com/~mcomella/apks/mcomella-965548_06.apk

And build 6 from comment 68, but with a slightly slower entry animation (recommended by liuche): https://people.mozilla.org/~mcomella/apks/mcomella-965548_07.apk

(In reply to Michael Comella (:mcomella) from comment #67) > (In reply to Lucas Rocha (:lucasr) from comment #64) > > Hey Mike, what about experimenting with some of these techniques[1] to see > > if it helps us simplify the edit mode transitions and toolbar layout a bit? > > For instance, we can advantage of the fact that we don't animate the toolbar > > in pre-ICS and use some more advanced APIs here. > > As I understand it, Transitions are only available in KitKat and don't > appear to be available in the support libraries. That means we'd have to > duplicate our animation code - one for KitKat+ and one for pre-KitKat - is > that what you're asking for? What I meant is using the onPreDrawListener technique described in the blog post to animate layout transitions. The sample code I linked to doesn't use KitKat's transitions framework: https://gist.github.com/lucasr/9508844

(In reply to Lucas Rocha (:lucasr) from comment #70) > What I meant is using the onPreDrawListener technique described in the blog > post to animate layout transitions. I'm not sure I see the benefits of this approach (disclaimer: only after having read through your post and code sample). Considering we don't have AutoTransitions, it seems the benefit is that we would call our animation code from an OnPreDrawListener, rather than directly in show/stopEditing, better encapsulating the code. Note that I don't see any reason to store the start and end states. However, we still need to attach the listener. In your code sample, this requires extending a View class and overriding any events that would cause an animation, which seems like a lot of overhead for any View we want to animate. More simply, I think we can attach the listener in the show/stopEditing functions before we return, but then it's equivalent to calling an "animateShow/StopEditing" function and so the OnPreDrawListener doesn't provide much. Am I missing something? Note: I moved the animation code into showEditingWithAnimation helper methods in part 6, which I think greatly improves the readability of the show/stopEditing methods (and is equivalent to my current understanding of what I can do with the OnPreDrawListener). (One benefit that I don't think is important for us is if we store start/end state as in Transitions, we can cancel animations mid-animation) > For instance, we can advantage of the fact that we don't animate the toolbar > in pre-ICS and use some more advanced APIs here. I was originally unfamiliar with these APIs, and after skimming over their docs, they don't seem to offer much over our current code (one thing that they might do are give us the ability to remove excess views by having more comprehensive animations). Thus, I'm not sure it's worth the added time to refactor and fix the various fallouts. Do you know something I don't?

Spoke to Lucas on IRC - I'm going to look into running the animations in the PreDrawListener, which should allow us to use a single toolbar view, simplifying both the code and the View hierarchy. If it's within the scope of this bug, I'll do it here, otherwise I'll file a followup.

Feedback on the builds in comment 68 and comment 69 please! :)

Comment on attachment 8399733 [details] [diff] [review] Part 5: Rename url bar elements for clarity between phone/tablet configs. Review of attachment 8399733 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review for now until this gets updated.

(In reply to Michael Comella (:mcomella) from comment #72) > Spoke to Lucas on IRC - I'm going to look into running the animations in the > PreDrawListener, which should allow us to use a single toolbar view, > simplifying both the code and the View hierarchy. If it's within the scope > of this bug, I'll do it here, otherwise I'll file a followup. I'm just too unfamiliar with the APIs so I filed bug 993069.

Merged part 5 (variable renames) and addressed nits. Note that a later patch will make the variable conventions consistent in BrowserToolbar.

Rebase (part 6 -> part 5).

Comment on attachment 8402884 [details] [diff] [review] Part 5: Improve readability for BrowserToolbar.start/stopEditing methods. Review of attachment 8402884 [details] [diff] [review]: ----------------------------------------------------------------- Move r+.

Forgot to add a file from the qfold.

(In reply to Michael Comella (:mcomella) from comment #73) > Feedback on the builds in comment 68 and comment 69 please! :) Thanks for providing options, Michael! I don't have a strong opinion on which of these I prefer, to be honest. In all of the builds, entering edit mode looks good, and leaving is a little jankier but not too bad. How about we land the one in comment 69 and get some more people using it. To me, this is a big interaction improvement, and we can refine the UI transitions if we want to in follow up bugs once we get some more feedback.

WIP - I can't get the forward button to work properly so this patch ignores it for now.

Comment on attachment 8402909 [details] [diff] [review] Part 4: Shrink the url bar on appropriate phones. Review of attachment 8402909 [details] [diff] [review]: ----------------------------------------------------------------- Just a bit of background first, this patch is necessary because the entry might expand or shrink depending on the device having a hw menu button or not?

(In reply to Lucas Rocha (:lucasr) from comment #82) > Just a bit of background first, this patch is necessary because the entry > might expand or shrink depending on the device having a hw menu button or > not? Correct - when the software menu button is missing, the toolbar is larger in display mode than edit mode, where it's made shorter by the separator and cancel button.

Comment on attachment 8402909 [details] [diff] [review] Part 4: Shrink the url bar on appropriate phones. Review of attachment 8402909 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall. Wondering: have you considered simply changing the layout params of url_bar_entry to align to tabs or editing_layout depending on the device? It would be nice if we could avoid adding more views to toolbar (which is quite bloated already). Giving f+ to get some discussion before going ahead with this approach. ::: 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/. --> I thought this patch was only covering the toolbar on phones. Why did you have to add this file here? ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +119,5 @@ > > private ToolbarDisplayLayout mUrlDisplayLayout; > private ToolbarEditLayout mUrlEditLayout; > private View mUrlBarEntry; > + private View urlBarEntryShrunken; nit: be consistent with the rest of the file i.e. use 'm' prefix. @@ +120,5 @@ > private ToolbarDisplayLayout mUrlDisplayLayout; > private ToolbarEditLayout mUrlEditLayout; > private View mUrlBarEntry; > + private View urlBarEntryShrunken; > + private ImageView urlBarTranslatingEdge; Tip for future patches: I would have renamed this member in a separate patch. I would make this patch a little less noisy. Ditto for coding style. @@ +137,5 @@ > > private final ThemedView editSeparator; > private final View editCancel; > > + private boolean shouldShrinkURLBar = false; Ditto for coding style. @@ +968,5 @@ > } > > + final int curveTranslation = getUrlBarCurveTranslation(); > + final int entryTranslation = getUrlBarEntryTranslation(); > + shouldShrinkURLBar = entryTranslation < 0; nit: enclose expression with ()'s

Comment on attachment 8404362 [details] [diff] [review] Part 7: Add tablet editing mode cancel button. Review of attachment 8404362 [details] [diff] [review]: ----------------------------------------------------------------- I'm getting really worried about the explosion in complexity here. It's pretty clear that we're starting to bend toolbar's layout and code a bit too hard. The toolbar layout is already in need for a major cleanup anyway, so let's step back and see how we can restructure the layout in order to better accommodate these UI changes. ::: mobile/android/base/BrowserApp.java @@ +1539,5 @@ > final Tab selectedTab = Tabs.getInstance().getSelectedTab(); > mTargetTabForEditingMode = (selectedTab != null ? selectedTab.getId() : null); > > + final int animationDuration = HardwareUtils.isTablet() ? 350 : 250; > + final PropertyAnimator animator = new PropertyAnimator(animationDuration); Why would we want a slower animation on tablets? @@ +1924,5 @@ > * the root (mMenu). > */ > private void addAddonMenuItemToMenu(final Menu menu, final MenuItemInfo info) { > info.added = true; > + Unrelated. @@ +2060,5 @@ > public boolean onCreateOptionsMenu(Menu menu) { > // Sets mMenu = menu. > super.onCreateOptionsMenu(menu); > > + // Inform the menu about the action-items bar. Unrelated. @@ +2474,5 @@ > > /* > * If the app has been launched a certain number of times, and we haven't asked for feedback before, > * open a new tab with about:feedback when launching the app from the icon shortcut. > + */ Unrelated. ::: 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.

(In reply to Lucas Rocha (:lucasr) from comment #84) > Looks good overall. Wondering: have you considered simply changing the > layout params of url_bar_entry to align to tabs or editing_layout depending > on the device? It would be nice if we could avoid adding more views to > toolbar (which is quite bloated already). Giving f+ to get some discussion > before going ahead with this approach. For some reason, I strayed away from doing things dynamically. Here's a patch which does just that, but I couldn't figure out how to remove the extra view. However, it does significantly clean up the code and makes more intuitive sense, imo. > ::: > mobile/android/base/resources/drawable/url_bar_translating_edge_tablet.xml > I thought this patch was only covering the toolbar on phones. Why did you > have to add this file here? Mistake - removed. > nit: be consistent with the rest of the file i.e. use 'm' prefix. The intent was to make the final patch in the series remove the prefixes throughout the file. This was intended to be the final patch (rather than a pre) to avoid wasting my time with rebase churn.

Clarified a comment.

If the guide view is necessary, Finkle brought up the idea of ViewStubbing it, or adding it dynamically, to save cycles on startup.

Sorry for spam - fixing xml alignment.

Ian, I spoke with Lucas over Vidyo and, due to a painful increase in code complexity, he suggested landing the phone version in this bug, and a tablet version later, in a dependent bug (hopefully after we clean up the code a bit). This way, we can see if this is a reasonable solution to our issues without any unnecessary engineering effort (and a much easier backout). How would you feel about this?

That's fine, as long as we move quickly on the tablet follow-up.

Updated the spacing between editing mode components under bnicholson's recommendations.

Comment on attachment 8407810 [details] [diff] [review] Part 3: Add cancel button for phone layout. Review of attachment 8407810 [details] [diff] [review]: ----------------------------------------------------------------- Move r+.

Comment on attachment 8404362 [details] [diff] [review] Part 7: Add tablet editing mode cancel button. As per comment 91, invalidating the tablet patch.

Factored out the rightEdge -> translatingEdge rename from (what is now) part 5.

Note also that I don't have the "Cancel" message in landscape mode working - there are l10n considerations that make it difficult to code around (in addition to our rotation being wonky). Ian, is this also okay to miss in the v1?

Fixed a bug with devices that shrink and do not animate.

Rebase (was formerly part 5). Note that this was previously r+'d by bnicholson.

So much churn. D:

(In reply to Michael Comella (:mcomella) from comment #98) > Note also that I don't have the "Cancel" message in landscape mode working - > there are l10n considerations that make it difficult to code around (in > addition to our rotation being wonky). > > Ian, is this also okay to miss in the v1? Heh, yeah i guess so

Comment on attachment 8407817 [details] [diff] [review] Part 4: Rename url bar right edge to translating edge. Review of attachment 8407817 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with the coding style fix. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +119,5 @@ > > private ToolbarDisplayLayout mUrlDisplayLayout; > private ToolbarEditLayout mUrlEditLayout; > private View mUrlBarEntry; > + private ImageView urlBarTranslatingEdge; urlBarTranslatingEdge -> mUrlBarTranslatingEdge

Comment on attachment 8407872 [details] [diff] [review] Part 5: Shrink the url bar on appropriate phones. Review of attachment 8407872 [details] [diff] [review]: ----------------------------------------------------------------- This is a lot better, thanks! Same note on the coding style though. Please keep it consistent with the rest of the file.

Comment on attachment 8407875 [details] [diff] [review] Part 6: Improve readability for BrowserToolbar.start/stopEditing methods. Review of attachment 8407875 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8407879 [details] [diff] [review] Part 7: Remove unused members and class variable prefixes. Review of attachment 8407879 [details] [diff] [review]: ----------------------------------------------------------------- Ignore my previous comments on coding style :-) Somehow missed this patch.

Woosh: remote: https://hg.mozilla.org/integration/fx-team/rev/b9a4644f4039 remote: https://hg.mozilla.org/integration/fx-team/rev/ee46cc7283c2 remote: https://hg.mozilla.org/integration/fx-team/rev/8998623049ce remote: https://hg.mozilla.org/integration/fx-team/rev/6a49f6cb3582 remote: https://hg.mozilla.org/integration/fx-team/rev/be018ccdcb82 remote: https://hg.mozilla.org/integration/fx-team/rev/440224cc83d9 remote: https://hg.mozilla.org/integration/fx-team/rev/9a5586190013 remote: https://hg.mozilla.org/integration/fx-team/rev/d46231f4d4ae

I'm silly - I broke tablet.

Saving Private Tablet: https://hg.mozilla.org/integration/fx-team/rev/62a5caa65d45

https://hg.mozilla.org/mozilla-central/rev/b9a4644f4039 https://hg.mozilla.org/mozilla-central/rev/ee46cc7283c2 https://hg.mozilla.org/mozilla-central/rev/8998623049ce https://hg.mozilla.org/mozilla-central/rev/6a49f6cb3582 https://hg.mozilla.org/mozilla-central/rev/be018ccdcb82 https://hg.mozilla.org/mozilla-central/rev/440224cc83d9 https://hg.mozilla.org/mozilla-central/rev/9a5586190013 https://hg.mozilla.org/mozilla-central/rev/d46231f4d4ae https://hg.mozilla.org/mozilla-central/rev/62a5caa65d45

I just pulled and built. This won't launch on my phone. Its using a 19 API in BrowserToolbar to create a RelativeLayout.LayoutParam: http://developer.android.com/reference/android/widget/RelativeLayout.LayoutParams.html#RelativeLayout.LayoutParams%28android.widget.RelativeLayout.LayoutParams%29 http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java#193 Is it something from me. Why aren't our tests seeing this...

Hmm.. Tinderbox builds are fine for me. Maybe I'm just crazy.

Verified as fixed on Nightly 31.0a1(2014-04-18) on Alcatel One Touch(Android 4.1.2).

(In reply to Wesley Johnston (:wesj) from comment #111) > I just pulled and built. This won't launch on my phone. Its using a 19 API > in BrowserToolbar to create a RelativeLayout.LayoutParam: Oh, hm – it should default to the ViewGroup.MarginLayoutParams constructor instead, which is the super-class of RelativeLayout.LayoutParam, but this could be trouble if that constructor does different things from the one I specified. Thus, for correctness, it should be ViewGroup.MarginLayoutParam. I guess we'll see if anyone else starts to have issues.

(In reply to Michael Comella (:mcomella) from comment #114) > Thus, for correctness, it should be ViewGroup.MarginLayoutParam. See bug 998426.