The editable text shifts a bit to the right when you enter edit mode in the new tablet UI.

I haven't started work on editing mode yet (bug 1058902) so this will likely be duped to that bug when that lands.

Aligning the text looks pretty dumpy. If we align the text, I see a few options: 1) Keep the site security icon (i.e. globe, lock) - this is kind of lame ("Why is my toolbar space being taken up for this?) 2) Switch to an editing mode icon (e.g. pencil writing) 3) Translate the text over in an animation - this sucks because then there's more of a delay to enter editing mode. Anthony, what do you think?

Comment on attachment 8508217 [details] Screenshot of shifted text I might be missing some context here, but am I looking at the white space to the left of the highlighted text? The space that normally has the sites credentials like the green lock? Currently on my N7 we just remove it without an animation (when the user focuses on the awesome bar) and the text bumps over to close this gap - this is fine for V1 I think. I'd like to get a quick animation in there soon, but we definitely shouldn't leave that space there lying under-utilized like is :)

(In reply to Anthony Lam (:antlam) from comment #3) > Currently on my N7 we just remove it without an animation (when the user > focuses on the awesome bar) and the text bumps over to close this gap - this > is fine for V1 I think. Lucas filed this to make the text align itself. > I'd like to get a quick animation in there soon, but we definitely shouldn't > leave that space there lying under-utilized like is :) Sure, then I'll leave this for v2.

Alternative idea (which we can work on in v2): show a dimmed globe icon while editing i.e. the same behaviour than Firefox on desktop.

:antlam says we should go with comment 5, but use a magnifying glass instead of the globe. He says it should already be in our assets, but if not, NI him for assets.

via irc.

You can look in our mxr code search tool to find the asset. I'd recommend starting here: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/

Anthony, do we want a placeholder grey globe, or an active placeholder grey globe? :)

Placeholder grey until they start typing, then active placeholder ! Thanks guys!

Oh, actually, we don't use the placeholder text color yet so let's just go with "active placeholder grey" for now. Ponç, look for a magnifying glass in a dark grey, almost black (i.e. "active placeholder grey"). See [1] for the palette. You'll want to put that in org.mozilla.gecko.toolbar.ToolbarEditLayout. If it's not there, needinfo :antlam for an updated asset. Let me know if you have any questions! Happy coding! [1]: https://bug1127517.bugzilla.mozilla.org/attachment.cgi?id=8566330

Ok, I'll be working on it next days

Unassigning due to inactivity - Ponç, let me know if you're still working on this.

(In reply to Michael Comella (:mcomella) from comment #13) > Unassigning due to inactivity - Ponç, let me know if you're still working on > this. Sorry you have not been warned. I was struggling to keep up with it. I Wish I could go on with your project , but I can not at present.

(In reply to Ponç Bover [:pbover] from comment #14) > Sorry you have not been warned. I was struggling to keep up with it. I Wish > I could go on with your project , but I can not at present. It's okay - if you'd ever like to come back and work on something, just let me know! :)

via bug 795767 comment 15.

(In reply to henry from comment #17) > Created attachment 8588505 [details] [diff] [review] > Initial attempt at displaying an inactive search icon when in editing mode. Left with display an active search icon when user is typing. Submitted this for a quick review to get feedback on whether I'm on the right track.

Comment on attachment 8588505 [details] [diff] [review] Initial attempt at displaying an inactive search icon when in editing mode. Review of attachment 8588505 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good so far! :) ::: mobile/android/base/toolbar/ToolbarEditLayout.java @@ +137,5 @@ > + // in editing mode > + displaySearchIcon(context, R.drawable.search_icon_active); > + } > + > + void displayInactiveSearchIcon(Context context) { I'd prefer one method (maybe `updateSearchIcon`) that'd take an `isActive` boolean. @@ +144,5 @@ > + displaySearchIcon(context, R.drawable.search_icon_inactive); > + } > + > + private void displaySearchIcon(Context context, int searchIconResId) { > + if (NewTabletUI.isEnabled(context)) { I'm actually removing the NewTabletUI class in bug 1106935 because we no longer support old tablet - you can use HardwareUtils.isTablet() here. @@ +146,5 @@ > + > + private void displaySearchIcon(Context context, int searchIconResId) { > + if (NewTabletUI.isEnabled(context)) { > + mEditText.setCompoundDrawablesWithIntrinsicBounds( > + searchIconResId, 0, R.drawable.ab_mic, 0); ab_mic is dynamic based on whether or not we have support for voice recognition [1] - you'll have to add some data to maintain state on which drawables are available and active. Note that as part of bug 602818 comment 34, there will be two icons to the right of the url bar - I'm not sure how this would change the code you need to write (but you could always land this first! ;). [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarEditText.java?rev=38a668c3efaa#478

- Move setting of the search icon to the `ToolbarEditText` class. It was easier to track it's state that way. - Remove the use of `NewTabletUI.isEnabled` and replace that with `HardwareUtils.isTablet` - Track the state of the `EditText` in the `afterTextChanged` method to know when user is typing so the appropriate state of the icon is displayed. - Make sure the icon `ab_mic` is set approriately.

(In reply to Michael Comella (:mcomella) from comment #20) > Comment on attachment 8588505 [details] [diff] [review] > Initial attempt at displaying an inactive search icon when in editing mode. > > Review of attachment 8588505 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good so far! :) > > ::: mobile/android/base/toolbar/ToolbarEditLayout.java > @@ +137,5 @@ > > + // in editing mode > > + displaySearchIcon(context, R.drawable.search_icon_active); > > + } > > + > > + void displayInactiveSearchIcon(Context context) { > > I'd prefer one method (maybe `updateSearchIcon`) that'd take an `isActive` > boolean. > > @@ +144,5 @@ > > + displaySearchIcon(context, R.drawable.search_icon_inactive); > > + } > > + > > + private void displaySearchIcon(Context context, int searchIconResId) { > > + if (NewTabletUI.isEnabled(context)) { > > I'm actually removing the NewTabletUI class in bug 1106935 because we no > longer support old tablet - you can use HardwareUtils.isTablet() here. > > @@ +146,5 @@ > > + > > + private void displaySearchIcon(Context context, int searchIconResId) { > > + if (NewTabletUI.isEnabled(context)) { > > + mEditText.setCompoundDrawablesWithIntrinsicBounds( > > + searchIconResId, 0, R.drawable.ab_mic, 0); > > ab_mic is dynamic based on whether or not we have support for voice > recognition [1] - you'll have to add some data to maintain state on which > drawables are available and active. > > Note that as part of bug 602818 comment 34, there will be two icons to the > right of the url bar - I'm not sure how this would change the code you need > to write (but you could always land this first! ;). > We could replace the search icon with the QR code depending on the state or lay it before the search icon > [1]: > https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ > ToolbarEditText.java?rev=38a668c3efaa#478

Apologies if my changes looks spammy. I'm still figuring out `bzexport` extension. It diff'd against an earlier patch I submitted and marked it as obsolete. All my recent patches should be valid.

Henry, I'm having some difficulty following your patch series. Ideally, there should be a series of patches (or just one) that have their commit messages and BZ attachment names labeled with their ordering, e.g. "Part 1: Refactor to avoid repetition in determing voice recognizer support" (if there's just one patch, feel free to omit "Part X"). However, it appears that some of your patches undo work of your previous patches and that maybe they can be combined into one larger patch. Without knowing the specific details, I can only give some general advice on fixes (assuming you're using mercurial queues): use `hg qseries` to view your patch queue and see what's applied - you should probably have 1 patch here. If not you'll probably want to fold them together - see `hg qfold` (you can type `hg help <commandName>` to find out more about a command). Note that bzexport will push the currently checked out commit to Bugzilla (in the case of queues, this is the topmost applied patch). Let me know if you need any help. I'm going to clear the reviews for now - please reflag me when you're ready for a review.

Hi Michael, Apologies for the mess. I went ahead and created a new patch from when I started working on this bug to date in order to create a single patch for the changes. I used `hg export 238486:d7be9d9ec841 238576:c489b2338bad -o ~/bug-1079182.patch` to create the attached patch. Doing `hg qseries` listed a bunch of old patches that have already been applied. I then went ahead to delete them all with `hg qremove name.patch` after, I issued `hq qnew name.patch` but it gave me an empty patch with no changes at all. This is my workflow. Before making any new changes to the code base, I do 1. `hg pull` 2. `hg merge tip` 3. `hg commit` 4. Make changes to the code base 5. Commit changes 6. Create a patch using `hg export tip -o ~/name.patch Recently I discovered `hg bzexport -e` which I started using. I'm yet to grasp how it creates patches. Here are the changes in this patch. - Moved setting of the search icon to the `ToolbarEditText` class. It was easier to track it's state that way. - Removed the use of `NewTabletUI.isEnabled` and replace that with `HardwareUtils.isTablet` as you pointed out in previous review. - Tracked the state of the `EditText` in the `afterTextChanged` method to know when a user is typing so the appropriate state of the icon is displayed. - Make sure the icon `ab_mic` icon is set appropriately. It only get displayed if support recognizer is available. - Used a boolean flag to determine when to show `ab_mic` icon to avoid duplicate call to the method that determines if `ab_mic` icon is supported. I'm still getting my head around Mercurial and it's extensions. Hopefully I'm on the right track. Thanks

- Set the search icon in the `ToolbarEditText` class. It was easier to track it's state that way. - Use `HardwareUtils.isTablet` for checking for tablet devices. - Track the state of the `EditText` in the `afterTextChanged` method to know when a user is typing so the appropriate state of the icon is displayed. - Make sure the icon `ab_mic` icon is set appropriately. It only get displayed if support recognizer is available. - Used a boolean flag to determine when to show `ab_mic` icon to avoid duplicate call to the method that determines if `ab_mic` icon is supported.

Hey, Henry - I apologize for the delay! It sounds to me like you're mixing two different workflows - mercurial queues ("mq" for short) and making commits. We formerly used mq at Mozilla but have been recently switching over to commits and bookmarks (similar to git branching). I'd say you should just move over to bookmarks and commits and forget about using mq for now (pretty much any command pre-pended with "hg q"). Sorry if I made this confusing. There is some pretty good documentation on modern Mercurial at Mozilla at [1], in particular the section on bookmarks. (In reply to henry from comment #27) > I used `hg export 238486:d7be9d9ec841 238576:c489b2338bad -o > ~/bug-1079182.patch` to create the attached patch. Doing `hg qseries` listed > a bunch of old patches that have already been applied. I then went ahead to > delete them all with `hg qremove name.patch` after, I issued `hq qnew > name.patch` but it gave me an empty patch with no changes at all. That sounds expected but to avoid mq confusion, I won't explain the details - let me know if you want them! > This is my workflow. Before making any new changes to the code base, I do > 1. `hg pull` > 2. `hg merge tip` > 3. `hg commit` I don't use merge in my workflow - we generally use `hg rebase` as it provides a cleaner commit history (e.g. global, linear commit history (i.e. with a single head), no hard-to-read merge commits). > 4. Make changes to the code base > 5. Commit changes To avoid the need to merge, you can use rebase. If you run `hg rebase -d fx-team` (assuming you're checking out fx-team), you'll rebase the currently checked out revision onto the fx-team tag. Bookmarks make managing these operations much easier by giving revisions names with which to access them. > 6. Create a patch using `hg export tip -o ~/name.patch > > Recently I discovered `hg bzexport -e` which I started using. I'm yet to > grasp how it creates patches. bzexport will take the currently applied commit and upload it to Bugzilla - it shouldn't make any changes locally. It's the preferred way to upload patches to Bugzilla (over `hg export`). For reference, my workflow is roughly: 1) Name my current development "branch" - `hg bookmark globe_with_mag-1079182` 2) Make changes 3) `hg commit -m "Bug 1079182 - Made changes. r=henry"` 4) `hg pull fx-team` 5) `hg rebase -d fx-team` (make more changes, etc.) [1]: https://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html

Oh, you can jump between different bookmarks (and tags and revisions) by using `hg update` so... `hg update fx-team` (now working from the fx-team head, or remote revision) `hg bookmark another_bookmark-1111111` (now I can start working on Bug 1111111) `hg commit -m "Bug 1111111 - ..."` Oh, actually, I need to work on bug 1079182 again! `hg update globe_with_mag-1079182` (make changes) `hg commit -m "Bug 1079182 - ..."`

Comment on attachment 8592538 [details] [diff] [review] Replace globe with magnifying glass in tablet editing mode to align text Review of attachment 8592538 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good! I noticed the icon does not directly replace the globe (on the home page) - would it be possible to match that padding? Note that the assets could have whitespace padding in them and that padding could differ. If that's the case, we may have to edit these assets get them to look right - let me know if that's the case. Also, the very first time that the toolbar is tapped, the magnifying glass does not appear - can you take a look into that? By the way, the review flag is typically used to indicate when you're ready for a final lookover at the patch and want to get it checked in while feedback is typically used to get WIP feedback on the direction you're taking in your patch. You shouldn't need to use both. ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +502,5 @@ > + > + /** > + * Convenient method for setting the inactive state of the search icon > + */ > + private void displayInActiveSearchIcon() { I would prefer to call updateSearchIcon directly instead of using these methods - it creates too many method definitions to dive into to read easily. @@ +523,5 @@ > + } > + > + } > + > + private void setSearchIcon(int searchIconResId) { This does more than just set the search icon - this sets the toolbar icon state. I'd prefer if the name was more descriptive for this - e.g. updateToolbarIcons(int). Also, why not flatten this with updateSearchIcon? So `updateToolbarIcons(boolean isSearchActive)`.

(In reply to Michael Comella (:mcomella) from comment #29) > Hey, Henry - I apologize for the delay! > > It sounds to me like you're mixing two different workflows - mercurial > queues ("mq" for short) and making commits. We formerly used mq at Mozilla > but have been recently switching over to commits and bookmarks (similar to > git branching). I'd say you should just move over to bookmarks and commits > and forget about using mq for now (pretty much any command pre-pended with > "hg q"). Sorry if I made this confusing. > > There is some pretty good documentation on modern Mercurial at Mozilla at > [1], in particular the section on bookmarks. > > (In reply to henry from comment #27) > > I used `hg export 238486:d7be9d9ec841 238576:c489b2338bad -o > > ~/bug-1079182.patch` to create the attached patch. Doing `hg qseries` listed > > a bunch of old patches that have already been applied. I then went ahead to > > delete them all with `hg qremove name.patch` after, I issued `hq qnew > > name.patch` but it gave me an empty patch with no changes at all. > > That sounds expected but to avoid mq confusion, I won't explain the details > - let me know if you want them! > > > This is my workflow. Before making any new changes to the code base, I do > > 1. `hg pull` > > 2. `hg merge tip` > > 3. `hg commit` > > I don't use merge in my workflow - we generally use `hg rebase` as it > provides a cleaner commit history (e.g. global, linear commit history (i.e. > with a single head), no hard-to-read merge commits). > > > 4. Make changes to the code base > > 5. Commit changes > > To avoid the need to merge, you can use rebase. If you run `hg rebase -d > fx-team` (assuming you're checking out fx-team), you'll rebase the currently > checked out revision onto the fx-team tag. Bookmarks make managing these > operations much easier by giving revisions names with which to access them. > > > 6. Create a patch using `hg export tip -o ~/name.patch > > > > Recently I discovered `hg bzexport -e` which I started using. I'm yet to > > grasp how it creates patches. > > bzexport will take the currently applied commit and upload it to Bugzilla - > it shouldn't make any changes locally. It's the preferred way to upload > patches to Bugzilla (over `hg export`). > > For reference, my workflow is roughly: > > 1) Name my current development "branch" - `hg bookmark > globe_with_mag-1079182` > 2) Make changes > 3) `hg commit -m "Bug 1079182 - Made changes. r=henry"` > 4) `hg pull fx-team` > 5) `hg rebase -d fx-team` > (make more changes, etc.) > > [1]: > https://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/ > index.html Hi Michael, Thank you for sharing your rough workflow with me. It really helped in getting it right with `hg` I'm now using `bookmarks` and `hg rebase`

> This is looking pretty good! Thank you. > > I noticed the icon does not directly replace the globe (on the home page) - > would it be possible to match that padding? Note that the assets could have > whitespace padding in them and that padding could differ. If that's the > case, we may have to edit these assets get them to look right - let me know > if that's the case. It seems the `ToolbarDisplayLayout`[1] uses a different layout[2] for displaying the globe than what I'm doing in the `ToolbarEditText` class by setting a compound drawable with an icon to the left. I think I can play around with the drawable paddings to make it align like it's with the globe. If that doesn't work I'll have to replicate a similar layout [2] for the `ToolbarEditDisplayLayout`. > > Also, the very first time that the toolbar is tapped, the magnifying glass > does not appear - can you take a look into that? Yup. I noticed that as well. Looking into it. > > By the way, the review flag is typically used to indicate when you're ready > for a final lookover at the patch and want to get it checked in while > feedback is typically used to get WIP feedback on the direction you're > taking in your patch. You shouldn't need to use both. Thanks for the tip. I'll flag for a review when the patch is ready for landing. > This does more than just set the search icon - this sets the toolbar icon > state. I'd prefer if the name was more descriptive for this - e.g. > updateToolbarIcons(int). Updated accordingly > > Also, why not flatten this with updateSearchIcon? So > `updateToolbarIcons(boolean isSearchActive)`. Makes sense. Refactored to make it a single method for updating the toolbar icons [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#145 [2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/toolbar_display_layout.xml

Michael, I'm hitting this Bug 962391 at the moment. I'm not able to make successful builds to test my changes. This is going to slow me down a bit. Let me know if you know of a workaround for this issue.

(In reply to henry from comment #34) > Michael, I'm hitting this Bug 962391 at the moment. I'm not able to make > successful builds to test my changes. This is going to slow me down a bit. > Let me know if you know of a workaround for this issue. Unfortunately, I haven't seen this before. If you don't mind sparing the resources, could try recloning fx-team to see if you have the same issues in a new repository.

Fixed and refactored per comment #31

Hey, Henry. Sorry about the delay - I didn't realize the request queue was organized the way it was and I was skipping over feedback flags. x_x I'll be sure to get to this tomorrow.

Comment on attachment 8601910 [details] [diff] [review] Show seach icon when in editing mode on a tablet device Review of attachment 8601910 [details] [diff] [review]: ----------------------------------------------------------------- Beyond nits, the code looks good! However, behavior-wise, I think we should be showing the inactive search icon when the placeholder text is present and the active search icon every other time the magnifying glass should be present. In particular, when we lose focus on the url bar, it goes to inactive - I think it should stay active. Follow the text color! Also, have you tried to adjust the padding so this aligns with the display mode icon (i.e. globe, lock, etc.)? ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +163,5 @@ > + * Update the search icon at the left of the edittext based > + * on it's state. > + * > + * @param isActive The state of the edittext. Active is when the initialized > + * text has changed and not empty. nit: -> and is not empty @@ +167,5 @@ > + * text has changed and not empty. > + */ > + void updateSearchIcon(boolean isActive) { > + int abMic = mIsVoiceRecognizerSupported ? R.drawable.ab_mic : 0; > + if(!HardwareUtils.isTablet()) { nit: `if (` @@ +171,5 @@ > + if(!HardwareUtils.isTablet()) { > + setCompoundDrawablesWithIntrinsicBounds(0, 0, abMic, 0); > + return; > + } > + // When on tablet show a magnifying glass in editing mode nit: newline above. @@ +654,5 @@ > removeAutocomplete(editable); > } > > + // Update search icon with an active state since user is typing > + if(textLength > 0) { nit: -> updateSearchIcon(textLength > 0);

Fixed issues per feedback. See comment #38. In particular fixed all the nit issues and adjusted the behavior of the search icon state per the comment. Michael, I couldn't figure out how to fix the padding issue. I don't see any changes when I change the padding in the `toolbar_edit_layout.xml` file. I'm suspecting I have to implement a layout similar to `toolbar_display_layout.xml` Let me know if there is an easier way to fix that than modifying a layout file. I looked at the search icon images and they don't have extra whitespace. The `favicon_globe` is slightly bigger though. Thank you for your great feedback.

Fix nit issues

A few best practices, Henry: * When you post multiple patches that are to be landed in a sequence, you should include "Part #" in their description (e.g. Part 1 - Show seach icon when in editing mode on a tablet device). * When you post a new version of a patch, you should obsolete the old one. You can do this by going to the uploaded patch, clicking Details, hitting "edit details" on the loaded page, checking obsolete, and hitting save changes. Note that the obsolete menu is also given while uploading patches directly to Bugzilla and that patches uploaded via bzexport use some heuristic to do the same (I think it's if the comment description matches or maybe if it uses the same part #). Do you mind obsoleting your patch?

Comment on attachment 8605974 [details] [diff] [review] Set an active search icon state when toolbar loses focus Review of attachment 8605974 [details] [diff] [review]: ----------------------------------------------------------------- This is not quite right - when I type, unfocus the toolbar, and then click the toolbar again, the icon should still be active. To be explicit, in editing mode, the icon should be: * active when there is user entered content in the toolbar * inactive any other time

(In reply to henry from comment #39) > Michael, I couldn't figure out how to fix the padding issue. I don't > see any changes when I change the padding in the `toolbar_edit_layout.xml` > file. I'm suspecting I have to implement a layout similar to > `toolbar_display_layout.xml` A helpful tool you can use to figure out what is going on is in Android settings -> developer tools -> Show layout bounds. Using this tool, it looks like display mode's favicon aligns with the left of the back button while the ToolbarEditText (or layout) seems to have padding/margin on either side of it. Have you tried overriding the values (e.g. set padding=0 on the EditText and then padding=0 on the EditLayout and seeing if it makes a difference)? Or setting a background color on each view (so you can see the bounds of each view? It's like show layout bounds, but old school :P)? Play around and let me know if you're still having trouble!

(In reply to Michael Comella (:mcomella) from comment #41) > A few best practices, Henry: > * When you post multiple patches that are to be landed in a sequence, you > should include "Part #" in their description (e.g. Part 1 - Show seach icon > when in editing mode on a tablet device). > * When you post a new version of a patch, you should obsolete the old one. > You can do this by going to the uploaded patch, clicking Details, hitting > "edit details" on the loaded page, checking obsolete, and hitting save > changes. Note that the obsolete menu is also given while uploading patches > directly to Bugzilla and that patches uploaded via bzexport use some > heuristic to do the same (I think it's if the comment description matches or > maybe if it uses the same part #). Do you mind obsoleting your patch? Thanks for pointing me in the right direction.

(In reply to Michael Comella (:mcomella) from comment #43) > A helpful tool you can use to figure out what is going on is in Android > settings -> developer tools -> Show layout bounds. > Thanks for this tip. It came in handy. > Using this tool, it looks like display mode's favicon aligns with the left > of the back button while the ToolbarEditText (or layout) seems to have > padding/margin on either side of it. Have you tried overriding the values > (e.g. set padding=0 on the EditText and then padding=0 on the EditLayout and > seeing if it makes a difference)? Or setting a background color on each view > (so you can see the bounds of each view? It's like show layout bounds, but > old school :P)? I was editing the wrong layout earlier. I dug deeper in the code and realize there this a toolbar layout for lager screens. I was able to match the padding I think. I can't trust my myopic eyes :). Could you double check?

- Adjust the left padding for the `ToolbarEditLayout` view and that of drawablePadding in the `toolbar_edit_layout` - Use `tab.getURL()` value to compare the text in the edittex view to know which search icon state to display

- Minor typo and comment wrap fixes

Hi Michael, I was hoping to get some feedback on my diffs. Let me know when you get the chance. Thanks

Comment on attachment 8609105 [details] [diff] [review] Part #2 - Fix a typo in inline comment Review of attachment 8609105 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about that Henry - I've been a bit busy with some of my work and overlooked these reviews. :( I won't let it happen again.

Comment on attachment 8608636 [details] [diff] [review] Fix search icon padding and properly display its state icons Review of attachment 8608636 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +123,5 @@ > + if ((searchTerm.equalsIgnoreCase(tab.getURL())) || (TextUtils.isEmpty(searchTerm))){ > + isActive = false; > + } > + } > + updateSearchIcon(isActive); This method is missing - is this built on some previous patches? Can you fold them into this? I'll try to piece it together for now.

In order, I applied: Show seach icon when in editing mode on a tablet device (6.08 KB, patch) Set an active search icon state when toolbar loses focus (1.51 KB, patch) Fix nit issues from code review (2.33 KB, patch) (non-obsolete patches) Fix search icon padding and properly display its state icons (5.03 KB, patch) Part #2 - Fix a typo in inline comment (1.38 KB, patch) And built successfully. I'll review this stack, but you should still fold the patches together.

Comment on attachment 8608636 [details] [diff] [review] Fix search icon padding and properly display its state icons Review of attachment 8608636 [details] [diff] [review]: ----------------------------------------------------------------- It looks like the text shifts maybe 1dp to the right when entering editing mode but it's really close! You can use https:// addresses to check because the text doesn't change between the two modes (i.e. they don't get shortened). Also, the icon should be active when the toolbar is initially clicked (if you deselect the text, it'll be the active color). The inactive state is only used when the placeholder text is shown. I wouldn't be against merging the display text layout and editing text layout in code so the alignment is automatic - but let's leave that to a follow-up if you're interested. ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +119,5 @@ > + boolean isActive = true; > + if (tab != null) { > + final String searchTerm = getText().toString(); > + // Make search icon inactive when edit toolbar search term isn't user entered searc term > + if ((searchTerm.equalsIgnoreCase(tab.getURL())) || (TextUtils.isEmpty(searchTerm))){ As per my previous comment, you could probably remove the equals() and this will work correctly.

(In reply to Michael Comella (:mcomella) from comment #49) > Sorry about that Henry - I've been a bit busy with some of my work and > overlooked these reviews. :( I won't let it happen again. No problem. I got busy myself last week. Hence the late response. Thanks for the feedback

(In reply to Michael Comella (:mcomella) from comment #52) > Comment on attachment 8608636 [details] [diff] [review] > Fix search icon padding and properly display its state icons > > Review of attachment 8608636 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks like the text shifts maybe 1dp to the right when entering editing > mode but it's really close! You can use https:// addresses to check because > the text doesn't change between the two modes (i.e. they don't get > shortened). I really couldn't see the difference. I think my defected eyes play a roll in this. So I have reverted to the original paddings. Even with that, I couldn't see the difference. If it's still there with my current patch, could you aid with a screenshot if possible? Thank you. > As per my previous comment, you could probably remove the equals() and this > will work correctly. That did the trick.

(In reply to Michael Comella (:mcomella) from comment #52) > I wouldn't be against merging the display text layout and editing text > layout in code so the alignment is automatic - but let's leave that to a > follow-up if you're interested. I'm happy to work on this. Could you assign the bug to me if you get the chance. I'll find time to work on it. Also there were couple of bugs you indicated I could work on. Could you assign those as well to me? I would like to work on them in parallel. Thanks

Remove the check for `equals`

- Collapse the different changes into one

(In reply to henry from comment #56) > (In reply to Michael Comella (:mcomella) from comment #52) > > I wouldn't be against merging the display text layout and editing text > > layout in code so the alignment is automatic - but let's leave that to a > > follow-up if you're interested. > I'm happy to work on this. Could you assign the bug to me if you get the > chance. I'll find time to work on it. There's no bug on file so you can file it, assign yourself (you should have permissions now), and CC me. > Also there were couple of bugs you indicated I could work on. Could you > assign those as well to me? I would like to work on them in parallel. Thanks Feel free to assign yourself and post the links here (I'm not sure which bugs those are).

(In reply to henry from comment #55) > I really couldn't see the difference. The text still shifts, but we'll fix that in the bug that merges the layouts - just be warned that this could be pretty messy and you'll need to be very careful to get this right - it'd help (at least, for me to review and trust the refactor does not introduce issues) if you did the refactor in multiple patches. Consider how you can keep the encapsulation between the ToolbarDisplayLayout and ToolbarEditLayout methods (e.g. ToolbarDisplayLayout.setText and ToolbarDisplayLayout.setText are not the same). Otherwise, the behavior looks good! :)

(In reply to Michael Comella (:mcomella) from comment #60) > just be warned that this could be pretty messy and you'll need to be very > careful to get this right To be really explicit so you know what you'd be getting yourself into, if this patch makes the code less readable, less efficient, or doesn't work as before (with exceptions to desired improvements), we won't land it. Feel free to prototype and see how feasible it really is.

(In reply to Michael Comella (:mcomella) from comment #61) > To be really explicit so you know what you'd be getting yourself into, if > this patch makes the code less readable, less efficient, or doesn't work as > before (with exceptions to desired improvements), we won't land it. An alternative and simpler (and probably the correct) approach would be to have the various bits share LayoutParams. Though, I'm not sure if Android would handle that as I would expect and want it to... To be clear again, I'm not sure that this refactor can even make the code better so at your own risk.

Comment on attachment 8616536 [details] [diff] [review] Show seach icon when in editing mode on a tablet device Review of attachment 8616536 [details] [diff] [review]: ----------------------------------------------------------------- Collapsed patches look good! ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +116,5 @@ > super.onFocusChanged(gainFocus, direction, previouslyFocusedRect); > > + final Tab tab = Tabs.getInstance().getSelectedTab(); > + boolean isActive = true; > + if (tab != null) { I don't think we need this check anymore. @@ +118,5 @@ > + final Tab tab = Tabs.getInstance().getSelectedTab(); > + boolean isActive = true; > + if (tab != null) { > + final String searchTerm = getText().toString(); > + // Make search icon inactive when edit toolbar search term isn't user entered nit: -> a user entered @@ -115,5 @@ > return; > } > > removeAutocomplete(getText()); > - nit: try not to change unrelated lines (i.e. you removed a line here), or if you do, make it a separate explicit patch (e.g. commit summary includes "Whitespace fixes"). @@ +518,5 @@ > + // Voice Search is not supported > + mIsVoiceRecognizerSupported = false; > + // Set an inactive search icon with the mic button removed since > + // voice recognizer is not supported > + updateSearchIcon(false); I'd be a little clearer in these comments above updateSearchIcon that the order of these calls is important because it relies on mIsVoiceRecognizerSupported.

(In reply to Michael Comella (:mcomella) from comment #59) > There's no bug on file so you can file it, assign yourself (you should have > permissions now), and CC me. > Done. Here is the Bug #1173652.

(In reply to Michael Comella (:mcomella) from comment #60) > (In reply to henry from comment #55) > > I really couldn't see the difference. I went back to the original comment and saw the sample screenshot you've attached to this bug. Now I see what you mean by text shifting. > > The text still shifts, but we'll fix that in the bug that merges the layouts > - just be warned that this could be pretty messy and you'll need to be very > careful to get this right - it'd help (at least, for me to review and trust > the refactor does not introduce issues) if you did the refactor in multiple > patches. Consider how you can keep the encapsulation between the > ToolbarDisplayLayout and ToolbarEditLayout methods (e.g. > ToolbarDisplayLayout.setText and ToolbarDisplayLayout.setText are not the > same). Just to be clear, you meant `ToolbarDisplayLayout.setText` is different from `ToolbarEditLayout.setText`? > > Otherwise, the behavior looks good! :) Great to hear this. If I fix all the comments from the review, will you be opened to land this patch or you want the text shifting issue to be fixed as well before landing it? Want to know before I proceed with Bug #1173652

(In reply to Michael Comella (:mcomella) from comment #62) > (In reply to Michael Comella (:mcomella) from comment #61) > > To be really explicit so you know what you'd be getting yourself into, if > > this patch makes the code less readable, less efficient, or doesn't work as > > before (with exceptions to desired improvements), we won't land it. Noted. I'll differently ping you for feedback to get me in the right direction. > > An alternative and simpler (and probably the correct) approach would be to > have the various bits share LayoutParams. Though, I'm not sure if Android > would handle that as I would expect and want it to... > > To be clear again, I'm not sure that this refactor can even make the code > better so at your own risk. Heh :) Thanks for being explicit with what I'm getting myself into.

(In reply to Henry Addo from comment #66) > > The text still shifts, but we'll fix that in the bug that merges the layouts > > - just be warned that this could be pretty messy and you'll need to be very > > careful to get this right - it'd help (at least, for me to review and trust > > the refactor does not introduce issues) if you did the refactor in multiple > > patches. Consider how you can keep the encapsulation between the > > ToolbarDisplayLayout and ToolbarEditLayout methods (e.g. > > ToolbarDisplayLayout.setText and ToolbarDisplayLayout.setText are not the > > same). > > Just to be clear, you meant `ToolbarDisplayLayout.setText` is different from > `ToolbarEditLayout.setText`? If you want to change the text in ToolbarDisplayLayout, the method to do that has to be different than the method to change the text in the ToolbarEditLayout. (i.e. ToolbarLayout.setText would not be intuitive). Make sure the distinction between the two layout's methods is very clear (e.g. ToolbarLayout.setEditText or ToolbarLayout.getEditLayout().setText() or...). > > Otherwise, the behavior looks good! :) > > Great to hear this. If I fix all the comments from the review, will you be > opened to land this patch or you want the text shifting issue to be fixed > as well before landing it? Want to know before I proceed with Bug #1173652 Yep - we'll either handle the text shifting issue in bug 1173652, or if we determine that bug isn't going to work out, we'll file a follow-up bug.

Fix issues raised per comment #63

- Fix issues raised per comment #63 - Collapsed changesets into one patch

Hi Michael, I'm going to find time this week to look into bug 1173652. Are there any outstanding issues with my current patch?

Comment on attachment 8622138 [details] [diff] [review] Fix Replace globe with magnifying glass in tablet editing mode Review of attachment 8622138 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, Henry. I was on PTO last week. (In reply to Henry Addo from comment #71) > Are there any outstanding issues with my current patch? Nope, looks good!

Don't forget to add the checkin-needed keyword when the push goes green!

(In reply to Michael Comella (:mcomella) from comment #72) > Comment on attachment 8622138 [details] [diff] [review] > Fix Replace globe with magnifying glass in tablet editing mode > > Review of attachment 8622138 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry for the delay, Henry. I was on PTO last week. Welcome back > > (In reply to Henry Addo from comment #71) > > Are there any outstanding issues with my current patch? > > Nope, looks good! Thank you.

The patch no longer applies cleanly: > patching file mobile/android/base/toolbar/ToolbarEditText.java > Hunk #1 FAILED at 6 > Hunk #3 succeeded at 93 with fuzz 2 (offset -16 lines). > Hunk #5 FAILED at 503 > 2 out of 6 hunks FAILED -- saving rejects to file mobile/android/base/toolbar/ToolbarEditText.java.rej Please rebase it and mark checkin-needed again.

- Remove unused imported classes - Fix text shifting on nexus 7 or small tablet devices by removing `paddingRight=12dp` in browser toolbar layout for tablet devices. Strangely it displays perfectly on large tablet devices with the padding set. This I believe doesn't warrant a layout merge. See Bug #1173652

Forgot to mention patch #8627178 Fixes previous patch to make it apply cleanly. See comment #76

Hi Michael, I'm just looking at the patch and it seems Bug 602818 has removed alot of the existing code which caused my patch not to apply cleanly. This means, I have to go back to the drawing board again. I just realized my current patch has reintroduced all the code removed by Bug 602818. I'm basing this off here; https://github.com/mozilla/gecko-dev/commit/ba45639acc6c61c984e26d49c32a1bc1b8a27d9b

- Revert to latest changes in `fx-team` branch to make patch apply cleanly. - Show state based icon when in editing mode on tablet devices. Refactor implementation based on changes in the latest `fx-team` branch - Fix text shifting on nexus 7 or small tablet devices by removing `paddingRight=12dp` in browser toolbar layout for tablet devices. Strangely it displays perfectly on large tablet devices with the padding set. This I believe doesn't warrant a layout merge. See Bug #1173652 for info on that.

Hi Michael, I was hoping to get a comment on this? Thank you.

(In reply to Henry Addo from comment #81) > I was hoping to get a comment on this? Thank you. Sorry, Henry - I've been a bit disorganized from our company work week, travel, and the US holiday. I promise to be more on top of this in the future - I'll look at your patch now.

I noticed bug 1180907 is causing issues with the build (the magnifying glass gets tinted light grey - it's not your fault!) - I'm going to fix that and then double-check the build functionality.

With my fix for bug 1180907, functionality looks good! :) To the code...

Comment on attachment 8627237 [details] [diff] [review] Fix Replace globe with magnifying glass in tablet editing mode Review of attachment 8627237 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good - just a few more quick fixes. ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml @@ +35,5 @@ > android:background="@drawable/new_tablet_url_bar_nav_button"/> > > <org.mozilla.gecko.toolbar.ToolbarEditLayout android:id="@+id/edit_layout" > style="@style/UrlBar.Button" > + android:paddingLeft="10dp" Did you mean to remove android:paddingRight here? It changes the location of the voice input icon. ::: mobile/android/base/toolbar/ToolbarEditText.java @@ +102,5 @@ > + // search term > + if (TextUtils.isEmpty(getText())){ > + isActive = false; > + } > + updateSearchIcon(isActive); nit: final boolean isActive = !TextUtils.isEmpty(getText()); updateSearchIcon(isActive); @@ +156,5 @@ > + * @param isActive The state of the edittext. Active is when the initialized > + * text has changed and is not empty. > + */ > + void updateSearchIcon(boolean isActive) { > + // When on tablet show a magnifying glass in editing mode This is also affecting phone - perhaps add a if (isTablet) { return; } See HardwareUtils for determining device.

- Only show magnifying glass on tablet devices. - Fix nit issues per comment #85. - Move the voice input icon to it's original position.

(In reply to Michael Comella (:mcomella) from comment #84) > With my fix for bug 1180907, functionality looks good! :) To the code... Thank you Michael. Could you confirm Bug #1173652 is invalid? Thanks

(In reply to Henry Addo from comment #87) > Could you confirm Bug #1173652 is invalid? Thanks What do you mean by invalid? I'd still like to align the two icons, though we don't have to do that by merging the layouts. I wonder if the alignment issues can be attributed to different sized assets.

(In reply to Michael Comella (:mcomella) from comment #88) > I'd still like to align the two icons, though > we don't have to do that by merging the layouts. Looking at it on device, it does look pretty close - is that what you meant? I think ideally the text would also align in both display and editing modes - I'm not sure what's causing this. Anyway, we should land this and continue this discussion in a file-up, if that's what you meant!

Comment on attachment 8630268 [details] [diff] [review] Fix Replace globe with magnifying glass in tablet editing mode Review of attachment 8630268 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/resources/layout-large-v11/browser_toolbar.xml @@ +36,5 @@ > > <org.mozilla.gecko.toolbar.ToolbarEditLayout android:id="@+id/edit_layout" > style="@style/UrlBar.Button" > + android:paddingLeft="10dp" > + android:paddingRight="10dp" Actually, I think paddingRight should remain 12dp (as it was before the patch) - paddingRight doesn't have anything to do with the new magnifying glass asset.

(In reply to Michael Comella (:mcomella) from comment #89) > > Looking at it on device, it does look pretty close - is that what you meant? Yes. I had to make `padding:right="10dp"` for it align properly on small tablet devices. The nexus 7 for example. > I think ideally the text would also align in both display and editing modes > - I'm not sure what's causing this. My primary suspect is the call of `android:selectAllOnFocus="true"`. Its behaviour shifts the text when it's too long. See screenshot attachment #8630821 [details] I experimented a bit by removing`android:selectAllOnFocus="true"` from the`toolbar_edit_layout` file. It behaves as expected however we lose highlight upon focus. See attachment #8630822 [details] Perhaps overwrite select all? > > Anyway, we should land this and continue this discussion in a file-up, if > that's what you meant! I thought I fixed the issue by reducing the `padding:right` value. Though it got it close. Funky.

- Revert paddingRight value to its original value

Hi Michael, If you get the chance, could you check-in the current patch so we can land this fix. After, I'll follow up with the text-shifting issue on Bug 1173652. Thanks

Sorry, I think we're talking about two different things! The alignment I was concerned about is the left side of the text, not the right, so when you click the toolbar, the text will stay in the same place and, for some websites, stay the same, just with a different color (and highlighted). But let's do this in a follow-up.

Comment on attachment 8630826 [details] [diff] [review] Fix Replace globe with magnifying glass in tablet editing mode Review of attachment 8630826 [details] [diff] [review]: ----------------------------------------------------------------- I made a push to our try test servers (above). Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

:) Thanks, Henry!