Closed
Bug 965548
Opened 11 years ago
Closed 11 years ago
Add a way to close edit mode in one tap on phones
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox31 verified, fennec30+)
VERIFIED
FIXED
Firefox 31
People
(Reporter: ibarlow, Assigned: mcomella)
References
Details
Attachments
(11 files, 22 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 30+
Assignee | ||
Comment 1•11 years ago
|
||
(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?
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
Note to self: Try to throw some UI telemetry in there. :)
Component: General → Awesomescreen
OS: Mac OS X → Android
Hardware: x86 → All
Assignee | ||
Comment 4•11 years ago
|
||
I'm starting to dig into this now - Ian, are your mocks up to date?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 6•11 years ago
|
||
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
Flags: needinfo?(ibarlow)
Reporter | ||
Comment 7•11 years ago
|
||
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 :)
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 8•11 years ago
|
||
What do you think?
Attachment #8396604 -
Flags: feedback?(ibarlow)
Reporter | ||
Comment 9•11 years ago
|
||
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?
Reporter | ||
Updated•11 years ago
|
Attachment #8396604 -
Flags: feedback?(ibarlow) → feedback+
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8396728 -
Flags: review?(bnicholson)
Assignee | ||
Comment 12•11 years ago
|
||
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.
Attachment #8396760 -
Flags: review?(bnicholson)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8396781 -
Flags: review?(bnicholson)
Assignee | ||
Comment 14•11 years ago
|
||
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.
Assignee | ||
Comment 15•11 years ago
|
||
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?
Attachment #8396785 -
Flags: feedback?(bnicholson)
Assignee | ||
Comment 16•11 years ago
|
||
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).
Assignee | ||
Comment 17•11 years ago
|
||
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).
Assignee | ||
Comment 18•11 years ago
|
||
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.
Attachment #8396785 -
Flags: feedback?(bnicholson)
Assignee | ||
Comment 19•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8396785 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 8396853 [details] [diff] [review]
Part 3: Add cancel button for phone layout. v2
What do you think of this approach?
Attachment #8396853 -
Flags: feedback?(bnicholson)
Comment 21•11 years ago
|
||
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.
Attachment #8396760 -
Flags: review?(bnicholson) → review?(lucasr.at.mozilla)
Updated•11 years ago
|
Attachment #8396728 -
Flags: review?(bnicholson) → review+
Updated•11 years ago
|
Attachment #8396781 -
Flags: review?(bnicholson) → review+
Comment 22•11 years ago
|
||
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
Attachment #8396781 -
Flags: review+ → review-
Comment 23•11 years ago
|
||
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.
Attachment #8396853 -
Flags: feedback?(bnicholson) → feedback+
Assignee | ||
Comment 24•11 years ago
|
||
Opted for edit_mode_cancel because I can see where using a generic dialog cancel (i.e. button_cancel) could go wrong.
Attachment #8397322 -
Flags: review?(bnicholson)
Assignee | ||
Updated•11 years ago
|
Attachment #8396781 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8397322 -
Attachment is obsolete: true
Attachment #8397322 -
Flags: review?(bnicholson)
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #8397329 -
Flags: review?(bnicholson) → review+
Comment 27•11 years ago
|
||
(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!
Assignee | ||
Comment 28•11 years ago
|
||
With animations. Still need to investigate the possibility of using linear layout.
Assignee | ||
Updated•11 years ago
|
Attachment #8396853 -
Attachment is obsolete: true
Assignee | ||
Comment 29•11 years ago
|
||
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?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 30•11 years ago
|
||
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.
Reporter | ||
Comment 31•11 years ago
|
||
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.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 32•11 years ago
|
||
(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?
Reporter | ||
Comment 33•11 years ago
|
||
Yeah, that can happen in a quicker motion as in the current Nightly.
Assignee | ||
Comment 34•11 years ago
|
||
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.
Flags: needinfo?(ibarlow)
Comment 35•11 years ago
|
||
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?
Attachment #8396760 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 36•11 years ago
|
||
(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?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 37•11 years ago
|
||
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
Attachment #8396760 -
Attachment is obsolete: true
Assignee | ||
Comment 38•11 years ago
|
||
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.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 39•11 years ago
|
||
Ian, is the color of the separator correct in private browsing mode?
Reporter | ||
Comment 40•11 years ago
|
||
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 :(
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 41•11 years ago
|
||
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).
Attachment #8399731 -
Flags: review?(bnicholson)
Assignee | ||
Updated•11 years ago
|
Attachment #8397361 -
Attachment is obsolete: true
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #8399732 -
Flags: review?(bnicholson)
Assignee | ||
Comment 43•11 years ago
|
||
This should probably have happened sooner, but hg patch queues are hard. This will be clearer with the tablet patches.
Attachment #8399733 -
Flags: review?(bnicholson)
Assignee | ||
Comment 44•11 years ago
|
||
Also probably should have happened sooner.
Attachment #8399734 -
Flags: review?(bnicholson)
Comment 45•11 years ago
|
||
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.
Attachment #8399731 -
Flags: review?(bnicholson) → review+
Comment 46•11 years ago
|
||
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 47•11 years ago
|
||
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 48•11 years ago
|
||
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.
Attachment #8399734 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 49•11 years ago
|
||
(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 50•11 years ago
|
||
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 51•11 years ago
|
||
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.
Attachment #8399732 -
Flags: review?(bnicholson) → review?(lucasr.at.mozilla)
Assignee | ||
Comment 52•11 years ago
|
||
Note: Builds on the patch in bug 991256.
Attachment #8400881 -
Flags: review?(bnicholson)
Assignee | ||
Comment 53•11 years ago
|
||
Rebased and fixed nits (which are also addressed in bug 991256).
Assignee | ||
Updated•11 years ago
|
Attachment #8399731 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8400887 -
Flags: review+
Comment 54•11 years ago
|
||
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?
Attachment #8400881 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 55•11 years ago
|
||
Fixed nits.
Assignee | ||
Updated•11 years ago
|
Attachment #8400881 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8400926 -
Flags: review+
Assignee | ||
Comment 56•11 years ago
|
||
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.
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 57•11 years ago
|
||
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.
Assignee | ||
Comment 58•11 years ago
|
||
Rebase.
Assignee | ||
Updated•11 years ago
|
Attachment #8400887 -
Attachment is obsolete: true
Assignee | ||
Comment 59•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #57)
> Brian also had the idea of actually resizing the View...
during the animation.
Assignee | ||
Updated•11 years ago
|
Attachment #8400952 -
Flags: review+
Assignee | ||
Comment 60•11 years ago
|
||
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.
Assignee | ||
Comment 61•11 years ago
|
||
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
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 62•11 years ago
|
||
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.
Assignee | ||
Comment 63•11 years ago
|
||
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.
Comment 64•11 years ago
|
||
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/
Flags: needinfo?(lucasr.at.mozilla)
Comment 65•11 years ago
|
||
Comment on attachment 8399732 [details] [diff] [review]
Part 4: Shrink the url bar on appropriate phones.
Clearing review for now.
Attachment #8399732 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 66•11 years ago
|
||
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.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 67•11 years ago
|
||
(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?
Assignee | ||
Comment 68•11 years ago
|
||
(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
Assignee | ||
Comment 69•11 years ago
|
||
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
Comment 70•11 years ago
|
||
(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
Assignee | ||
Comment 71•11 years ago
|
||
(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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 72•11 years ago
|
||
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.
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 73•11 years ago
|
||
Feedback on the builds in comment 68 and comment 69 please! :)
Flags: needinfo?(ibarlow)
Comment 74•11 years ago
|
||
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.
Attachment #8399733 -
Flags: review?(bnicholson)
Assignee | ||
Comment 75•11 years ago
|
||
(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.
Assignee | ||
Comment 76•11 years ago
|
||
Merged part 5 (variable renames) and addressed nits. Note that a later patch will make the variable conventions consistent in BrowserToolbar.
Attachment #8402878 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8399732 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8399733 -
Attachment is obsolete: true
Assignee | ||
Comment 77•11 years ago
|
||
Rebase (part 6 -> part 5).
Assignee | ||
Comment 78•11 years ago
|
||
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+.
Attachment #8402884 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8399734 -
Attachment is obsolete: true
Assignee | ||
Comment 79•11 years ago
|
||
Forgot to add a file from the qfold.
Attachment #8402909 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8402878 -
Attachment is obsolete: true
Attachment #8402878 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Comment 80•11 years ago
|
||
(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.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 81•11 years ago
|
||
WIP - I can't get the forward button to work properly so this patch ignores it for now.
Assignee | ||
Updated•11 years ago
|
Attachment #8404362 -
Flags: feedback?(lucasr.at.mozilla)
Comment 82•11 years ago
|
||
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?
Assignee | ||
Comment 83•11 years ago
|
||
(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 84•11 years ago
|
||
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
Attachment #8402909 -
Flags: review?(lucasr.at.mozilla) → feedback+
Comment 85•11 years ago
|
||
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.
Attachment #8404362 -
Flags: feedback?(lucasr.at.mozilla)
Assignee | ||
Comment 86•11 years ago
|
||
(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.
Attachment #8407206 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8402909 -
Attachment is obsolete: true
Assignee | ||
Comment 87•11 years ago
|
||
Clarified a comment.
Attachment #8407210 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8407206 -
Attachment is obsolete: true
Attachment #8407206 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 88•11 years ago
|
||
If the guide view is necessary, Finkle brought up the idea of ViewStubbing it, or adding it dynamically, to save cycles on startup.
Assignee | ||
Comment 89•11 years ago
|
||
Sorry for spam - fixing xml alignment.
Attachment #8407216 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8407210 -
Attachment is obsolete: true
Attachment #8407210 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 90•11 years ago
|
||
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?
Flags: needinfo?(ibarlow)
Reporter | ||
Comment 91•11 years ago
|
||
That's fine, as long as we move quickly on the tablet follow-up.
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 92•11 years ago
|
||
Updated the spacing between editing mode components under bnicholson's recommendations.
Assignee | ||
Updated•11 years ago
|
Attachment #8400952 -
Attachment is obsolete: true
Assignee | ||
Comment 93•11 years ago
|
||
Comment on attachment 8407810 [details] [diff] [review]
Part 3: Add cancel button for phone layout.
Review of attachment 8407810 [details] [diff] [review]:
-----------------------------------------------------------------
Move r+.
Attachment #8407810 -
Flags: review+
Assignee | ||
Comment 94•11 years ago
|
||
Comment on attachment 8404362 [details] [diff] [review]
Part 7: Add tablet editing mode cancel button.
As per comment 91, invalidating the tablet patch.
Attachment #8404362 -
Attachment is obsolete: true
Assignee | ||
Comment 95•11 years ago
|
||
Factored out the rightEdge -> translatingEdge rename from (what is now) part 5.
Attachment #8407817 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8407216 -
Attachment is obsolete: true
Attachment #8407216 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 96•11 years ago
|
||
Attachment #8396604 -
Attachment is obsolete: true
Assignee | ||
Comment 97•11 years ago
|
||
Attachment #8407832 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 98•11 years ago
|
||
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?
Flags: needinfo?(ibarlow)
Assignee | ||
Comment 99•11 years ago
|
||
Fixed a bug with devices that shrink and do not animate.
Attachment #8407872 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8407832 -
Attachment is obsolete: true
Attachment #8407832 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 100•11 years ago
|
||
Rebase (was formerly part 5). Note that this was previously r+'d by bnicholson.
Attachment #8407875 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 101•11 years ago
|
||
So much churn. D:
Attachment #8407879 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8402884 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Summary: Add a way to close edit mode in one tap → Add a way to close edit mode in one tap on phones
Reporter | ||
Comment 102•11 years ago
|
||
(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
Flags: needinfo?(ibarlow)
Comment 103•11 years ago
|
||
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
Attachment #8407817 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 104•11 years ago
|
||
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.
Attachment #8407872 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 105•11 years ago
|
||
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.
Attachment #8407875 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 106•11 years ago
|
||
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.
Attachment #8407879 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 107•11 years ago
|
||
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
Assignee | ||
Comment 108•11 years ago
|
||
I'm silly - I broke tablet.
Attachment #8408534 -
Flags: review?(bnicholson)
Updated•11 years ago
|
Attachment #8408534 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 109•11 years ago
|
||
Saving Private Tablet: https://hg.mozilla.org/integration/fx-team/rev/62a5caa65d45
Comment 110•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment 111•11 years ago
|
||
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...
Comment 112•11 years ago
|
||
Hmm.. Tinderbox builds are fine for me. Maybe I'm just crazy.
Comment 113•11 years ago
|
||
Verified as fixed on Nightly 31.0a1(2014-04-18) on Alcatel One Touch(Android 4.1.2).
Target Milestone: Firefox 31 → ---
Assignee | ||
Comment 114•11 years ago
|
||
(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.
Assignee | ||
Comment 115•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #114)
> Thus, for correctness, it should be ViewGroup.MarginLayoutParam.
See bug 998426.
Updated•11 years ago
|
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
status-firefox31:
--- → verified
Updated•4 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
•