Closed Bug 1050479 Opened 10 years ago Closed 3 years ago

Make find-in-page an action mode

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: wesj, Assigned: koushik3peri, NeedInfo)

References

Details

Attachments

(2 files, 2 obsolete files)

From bug 923370 - We should make find-in-page an action mode using our urlbar. As a start we can create something like:

|textbox_____| Prev Next (we can overflow to a Menu if we add more options)

This project might take a little work. We've only ever used ActionModes for text selection, so I expect to find bugs/holes in the current code.

To implement, I'd start by inserting something in FindInPage.show():
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/FindInPageBar.java#65
to start the action mode, similar to what we do for text selection
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TextSelection.java#206

You'll have to create an ActionModeCallback to pass in there. In its OnCreate method you can add the textbox, prev, and next buttons as GeckoMenuItems. You could even try to inflate a menu from xml, but I'm not sure how well that will work.

I know we've never used a textbox as a menu item, so you may also have to create that widget. Probably could create something like GeckoMenuItem that overrides its getActionView to return a TextView...
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuItem.java#100

Don't be shy to ask if you have questions
Blocks: 923370
Depends on: 1050480
Depends on: 1050481
Blocks: 1050481
No longer depends on: 1050481
Assignee: nobody → koushik3peri
Hi Wesley,

I currently have this feature looking like this: http://imgur.com/cq0fTgn. I am having a little trouble figuring out how to expand the width of the text field and I was wondering if you could help me figure out why the text field is so small.

This is what I currently have under the onCreateActionMode(ActionModeCompat mode, Menu menu) method of my ActionModeCallback class: http://pastebin.com/hpZX10Jz. I am creating GeckoMenuItems and then adding them to the menu object given as a paramter of the onCreateActionMode method. Am I using the right approach for creating the items on the actionbar? I suspect that I can increase the size of the text field using my Layoutparams object but I am not sure which field or method I need to use. I have tried searching stackoverflow for this problem but most of the solutions to similar problems did not seem to work for me.


Also, the reason why I am using R.drawable.validation_arrow and R.drawable.validation_arrow_inverted for the find_prev and find_next items is because both R.drawable.find_prev and R.drawable.find_next are both white and blend completely into the action bar (this took a while for me to discover as I incorrectly assumed that my code for displaying these items was wrong when in fact the real reason was that the items blended into the background).

Cheers,

Koushik
Flags: needinfo?(wjohnston)
Sorry for the delay :( This looks pretty good. If you want to upload a WIP patch, I could play with it better to try and help you. You might try putting a weight in the layout params for the textbox:

http://developer.android.com/reference/android/widget/LinearLayout.LayoutParams.html#weight

That may fix things... I hope? We loop through the items in ActionModeCompatView and measure them to see how many fit in the bar. But that should give a min-width for them. When we finally put them in the LinearLayout, it should relayout/size the textbox to fill its width. You may also want to set a maxWith on it so that on tablets it doesn't stretch full screen.

You could alternatively grab our menu inflater (use ours, not Androids):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#322

and try to inflate your stuff from an xml menu resource. I don't think its ever been used for any menus that weren't our main menu, so I'm not sure what to expect if you do that. You would have to write a new MenuItem type to return the textbox as an ActionView. I think your current approach is probably easier :)
Flags: needinfo?(wjohnston)
Attached patch Bug 1050479 - WIP patch (obsolete) — Splinter Review
Hi Wesley,

So I tried a couple more things, including some of your suggestions, and I am still having problems getting the text field to display properly :( . There are three main things that I have tried: using a LayoutInflater, manually adding items to the menu variable and using Gecko's MenuInflater. As requested, I have attached a WIP patch (or diff file actually) that I generated using this link https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F so that you can look at what I am doing and hopefully see my error. Since this is a work in progress patch, there is a lot of commented code but hopefully its still readable (the final, working product will  be much cleaner!).

There are three files included in the patch file: BrowserApp.java (two changes), FindInPageBar.java (majority of the changes), and a new menu xml file called find_in_page_menu.xml which I will be inflating using GeckoMenuInflater. 


I have included the three different implementations mentioned above in commented out blocks in the method onCreateActionMode(ActionModeCompat mode, Menu menu) in the file, FindInPageBar.java. All three implementations seem really close to working and I think that they are just missing one or two lines of code to work but I am fresh out of ideas. The menuinflater method seems really promising if I could just figure out why my code throws a classcastexception when other examples from stackoverflow work fine with the same code.


Thanks for taking the time out of your day to help me out :)


PS: Hopefully I created and uploaded the patch correctly onto Bugzilla. It has been a while since I did this for a Co-op work term...
Flags: needinfo?(wjohnston)
(In reply to Koushik Peri from comment #3)
> PS: Hopefully I created and uploaded the patch correctly onto Bugzilla. It
> has been a while since I did this for a Co-op work term...
Worked like a charm. Thanks :) And sorry for the delay. I put a little time into playing with this and finally figured out that we basically ignore your call to setActionView right now. We've got our own custom menu code, and some of it is just stubbed out. setActionView does nothing!

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuItem.java#230

I also then thought maybe you could implement GeckoActionProvider instead, and have it return a view that you want. In fact, you could do that, but GeckoActionProvider is really specialized to do one thing and probably won't be easy to adapt (i.e. you might have to override all of its methods, but it may not be that bad (we should really make it an interface).

A simple fix is to just make that setActionView set the mActionView field. You'll also have to update setShowAsAction since it will overwrite your actionView with a default "button" one.

Making android:actionViewClass work in the xml would be quite a bit more work. You'd have to implement parsing of the attribute here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuInflater.java#124 and then have that generate a GeckoActionProvider that would generate the correct view when asked for it.
Flags: needinfo?(wjohnston)
Oh wow, I can't believe I didn't see that setActionView doesn't do anything..good find!

So I have set the mActionView field inside setActionView as well as added a line in setShowAsAction where it only modifies the value of mActionView if I haven't set it already in setActionView ( if(mActionView!=null) return; ). By itself, this doesn't seem to work  so I tried to implement a GeckoActionProvider and override most of the methods in that class to no avail. I will play around with this some more tonight and try and override all of its methods to see if this makes a difference. 

However, while I was looking at the code, I noticed that setShowAsAction creates a default MenuItemActionBar object and GeckoActionProvider has a MenuItemActionView object that it displays to the actionbar.  In my changes, I am not creating either one of these so I wanted to ask you if either of these two classes are necessary to display a wide textview object onto the actionbar? Or are they not relevant to what I am trying to do?
I'm so sorry. I missed this question :(

I don't think either of those classes should matter. I have a feeling that just returning in setShowAsAction hurt you. You probably need to just avoid this little slot here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenuItem.java#322

but the code after that that sets mActionEnum and calls onShowAsActionChanged(this) probably needs to run.
No worries, thank you for your help :). I think I have all the information needed to get this thing working!! On a side note, I noticed that there are several other bugs regarding the find in page feature (Bug 1005231, Bug 1050480, and Bug 1077574 for example) and there seems to be new functionality added as a result of these bugs. Should I also incorporate these features into the action bar and perhaps have some options overflow into a menu. I can take a screenshot later on to show you how I was thinking of arranging it..
Flags: needinfo?(wjohnston)
Heh. Yeah, I was hoping this would make it in before those so that you wouldn't have to worry about them, but they suddenly picked up speed :) I think putting them in the overflow menu would be good though. But yeah, we should try to transition them over.
Flags: needinfo?(wjohnston)
Yeah, I was hoping to have this done before school started but I had no idea that I would have so much trouble getting the text bar to inflate properly haha. No matter, I'll  try transitioning those changes over now. I'm a little busy until next Tuesday so I am hoping to have something ready to show you by next Friday at the latest (fingers crossed).
Attached patch Proposed patch for this Bug (obsolete) — Splinter Review
Proposed first patch for this bug that needs to be code reviewed.
Attachment #8497967 - Attachment is obsolete: true
Hey Wesley,

So I have finished incorporating the FindInPage feature into the actionbar and this is what it now looks like now: 

http://imgur.com/p0c2JUF
http://imgur.com/SOX6mSi
http://imgur.com/aEQkTOI

As you probably noticed, I also managed to transition the additional match count feature that was added after this bug was opened. 

I have attached my code changes to this Bug in the form of a patch which is ready to be code reviewed. Hopefully everything looks reasonable :)
Flags: needinfo?(wjohnston)
Will try to look today. Flagging UX to look at this as well, but I'd be fine landing like this and restyling in a separate bug.
Flags: needinfo?(wjohnston) → needinfo?(alam)
We have some work around this already in bug 1077574 if I'm not mistaken. Can we share that styling? particularly around iconography and padding.
Flags: needinfo?(alam) → needinfo?(wjohnston)
Yeah. We need to invert those arrow icons, or better yet get them in the correct color for our actionbar. We can reuse the textbox style/background as well. Sound good Koushik?
Flags: needinfo?(wjohnston)
Comment on attachment 8511820 [details] [diff] [review]
Proposed patch for this Bug

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

Make sure you mark these r? when you upload them so they show up in my list :)

This looks good though. Lots of whitespace and code style cleanup (plus the UX requested style changes), but seems good.

::: mobile/android/base/FindInPageBar.java
@@ +189,5 @@
> +        @Override
> +        public boolean onCreateActionMode(ActionModeCompat mode, Menu menu) {
> +            
> +            //Populate menu with items 
> +            

Theres lots of leading white space in here you should remove.

@@ +195,5 @@
> +            try{
> +                mFindText = new ActionBarSearchField(mContext, null); 
> +                mFindText.addTextChangedListener(this);              
> +
> +                GeckoMenuItem find_text = (GeckoMenuItem) menu.add(Menu.NONE,R.id.find_text,0,R.string.find_text).setActionView(mFindText);

We don't use underscores in variable names. Just findText is fine. Also, make every variable final if you can.

@@ +239,5 @@
> +        }
> +
> +        @Override
> +        public void onDestroyActionMode(ActionModeCompat mode) {  
> +            hide();            

I think you should destroy mFindText and mStatusText in here so that we don't keep them in memory.

@@ +243,5 @@
> +            hide();            
> +        }
> +    }
> +    
> +    private class ActionBarSearchField extends CustomEditText{

I would probably just move both these inner classes to their own files under mobile/android/base/menu. They're mostly resusable (but you might change matchCountView's name to ActionBarTextView).

@@ +254,5 @@
> +            
> +            this.setLayoutParams(params);
> +            this.setImeOptions(EditorInfo.IME_ACTION_SEARCH);
> +            this.setInputType(InputType.TYPE_CLASS_TEXT);
> +            this.setSingleLine();

You don't need "this" here.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +319,5 @@
>          if (actionEnum > 0) {
>              if (!mShowAsActionChangedListener.hasActionItemBar())
>                  return;
>  
> +            if (mActionView==null&!hasActionProvider()) {

Spaces between operators and variables.
Attachment #8511820 - Flags: feedback+
Thank you for reviewing my code Wesley! Your input is really appreciated :D!

For the UX changes, I'm assuming you guys want the actionbar to look like this image from bug 1077574: https://www.dropbox.com/s/6iadhyuh8mkt5zs/bug1077574_Phone.png?dl=0 ?

When you say "invert those arrow icons", what do you mean? Invert the color or invert their orientation? I'm currently using R.drawable.validation_arrow instead of R.drawable.find_next to display the arrow since R.drawable.find_next does not show very well on a white background. However, if I make the actionbar background black, this shouldn't be an issue anymore. Do you want me to change the background of the actionbar to black and change the arrows to a white filling like the image?  As for adding more padding, do you mean that the text bar should have a smaller height and the match count field occupy a little more horizontal space like the image?

It might just be simpler for me to look at the updated xml file in bug 1077574 and try to add all those attributes to my objects programmatically as that xml file already contains approved UX changes :).
Flags: needinfo?(wjohnston)
Flags: needinfo?(alam)
(In reply to Koushik Peri from comment #16)
> When you say "invert those arrow icons", what do you mean? Invert the color
> or invert their orientation? I'm currently using R.drawable.validation_arrow

Invert their color is what I meant.
Flags: needinfo?(wjohnston)
You got it, Koushik.

Let's try to unify the visual style - no sense it in being completely different IMO :)
Flags: needinfo?(alam)
Do we also want to modify the icon and padding of the checkmark that closes the actionmode? I was in the middle of changing the icon and the order in which it appears in the actionbar to reflect the desired look by modifying /src/mobile/android/base/resources/layout/actionbar.xml when I realized that this would mess up everything else that used the action bar (ex. Textselection). Should I leave the checkmark and divider in their current place and only modify the icons and padding of everything else? Personally, I think the current view looks a little lopsided since the checkmark is much larger and takes up more room than everything else...
Flags: needinfo?(wjohnston)
Flags: needinfo?(alam)
(In reply to Koushik Peri from comment #19)
> Do we also want to modify the icon and padding of the checkmark that closes
> the actionmode? I was in the middle of changing the icon and the order in
> which it appears in the actionbar to reflect the desired look by modifying
> /src/mobile/android/base/resources/layout/actionbar.xml when I realized that
> this would mess up everything else that used the action bar (ex.
> Textselection). Should I leave the checkmark and divider in their current
> place and only modify the icons and padding of everything else? Personally,
> I think the current view looks a little lopsided since the checkmark is much
> larger and takes up more room than everything else...

Not sure where you're referring to exactly, would you mind attaching a screen shot of what you mean?
Flags: needinfo?(alam)
My fennec build is being weird and calling system.exit() right now so I can't take a recent screen shot but here is one from earlier that hopefully shows what I am talking about:
[1] http://imgur.com/218vA6Y 
[2] http://imgur.com/U69Sfw0
[3] https://www.dropbox.com/s/6iadhyuh8mkt5zs/bug1077574_Phone.png?dl=0
The Checkmark, line separator and the padding in between them from [2] is something that is drawn by default when I open the actionbar and not something that I can control from FindInPageBar.java. When I change the color of the actionbar to dark grey just for FindInPageBar, this checkmark and separator blend in to the background and become really hard to see so to make sure that they are visible, I have to modify these icons in actionbar.xml and styles.xml. [1] is the result of changing the color of the checkmark, as well as reshuffling the icons and removing the separator so that I can get closer to the desired look in [3]. However, this change is also reflected in everything else that uses the actionbar such as TextSelection.java where the default background color is white like [2]. So now, after the changes in [1], textselection view has a barely visible, white checkmark on the right side of the screen. So what I am trying to say is that any changes I make to the checkmark and separator to get the visual style of [3] is reflected over all views that use the actionbar. So my question is, do I continue with the changes in [1] and create a new bug to deal with the UX changes that are made to TextSlection mode (perhaps make the textselection mode appear on a dark grey background)? If yes, should I also substitute the checkmark icon from  [1] with the "x" icon from [3]? 

Hopefully, this makes a little more sense. If this is still a bit confusing, I can provide more images once I get my build working again.
Flags: needinfo?(alam)
So I have finished modifying the UX and I have two different views: 

[1] http://imgur.com/XHsXITg
[2] http://imgur.com/YpXfNzd

The second one is the most similar to the UX from Bug1077574 but I cannot get rid of the empty space to the right of the "x" mark. I'm not sure where its originating from and I've already tried looking at actionbar.xml and styles.xml to no avail... I created [2] to compare the two different views since the checkmark is the default icon for closing all other actionbar instances. Let me know what you guys think of the images and which one we should go with. I also took a screenshot of my textselection mode after incorporating the changes necessary to get [1] to appear to show you what I was refering to in my last comment:

[2] http://imgur.com/UsdPaLf

As you can see, the checkmark is barely visible now and the view looks quite disorganized because of the changes that I made to actionbar.xml to get FindInPageBar to appear correctly. If we go with view [2], then the checkmark will be replaced with a small, white "x". Further work may need to be done here...

I have attached a new patch which includes the changes suggested by you, Wesley, as well as the  code to get the padding and iconography from image [1]. I also created separate files for the menu items as suggested but I am not sure how to include these files in the build path. I tried looking at the Makefile but I got confused so if someone could direct me to the right place to add these files to the build path, that would be great :).
Attachment #8511820 - Attachment is obsolete: true
Attachment #8516531 - Flags: review?(wjohnston)
Attachment #8516531 - Flags: review?(alam)
CC'ing Robin here too since she worked on the non-Action bar implementation of this too.
Comment on attachment 8516531 [details] [diff] [review]
Bug1050479 second proposed patch

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

Overall this looks good. I'm not a fan of the dark styling, but I have a meeting soon, so we'll get what UX wants in here. There's also lots of trailing white space on lines. You might open the patch in bugzilla to see and clean it out.

::: mobile/android/base/FindInPageBar.java
@@ +54,2 @@
>          
> +        if (mContext instanceof ActionModeCompat.Presenter) {    

Trailing ws.

@@ +184,5 @@
>  
>          @Override
>          public boolean onCreateActionMode(ActionModeCompat mode, Menu menu) {
> +            //change the background color to firefox's theme
> +            mActionBar.setBackground(new ColorDrawable(getResources().getColor(R.color.background_tabs)));

I don't really want to do this, but we'll see what UX thinks.

@@ +226,5 @@
> +                final GeckoMenuItem nextButton =  ((GeckoMenuItem)menu.add(Menu.NONE,R.id.find_next,3,R.string.find_next).setActionView(nextNavButton));
> +                nextButton.setShowAsAction(GeckoMenuItem.SHOW_AS_ACTION_ALWAYS, R.attr.menuItemActionModeStyle);
> +            }catch(ClassCastException e){
> +                //ignore
> +            }

Hmm... Its a little strange you need a button here. Why? i.e. The menuItem should look fine on its own? You also don't need a GeckoMenuItem in that case I think... Maybe just write a little helper here:

private MenuItem createMenuItem(int id, int title, Drawable icon, ActionMode mode) {
  final MenuItem item = menu.add(Menu.NONE, id, Menu.NONE, title);
  item.setIcon(icon);
  item.setShowAsAction(mode, R.attr.menuItemActionModeStyle);
  return item;
}

and then reuse it for the items you want to add?

@@ +241,4 @@
>          @Override
>          public boolean onActionItemClicked(ActionModeCompat mode, MenuItem item) {
>              // Ignore
>              return false;

If you don't use item.setActionView(nextNavButton)); above, you can handle the clicks here...

::: mobile/android/base/menu/ActionBarSearchField.java
@@ +10,5 @@
> +import android.view.inputmethod.EditorInfo;
> +import android.widget.LinearLayout;
> +
> +public class ActionBarSearchField extends CustomEditText{
> +    public ActionBarSearchField(Context context, AttributeSet attrs) {

I think these might be better if they were in XML (just because people are used to looking for stuff there. But it gets a bit tricky here because we really don't know what container they're going into, so I think I can live with this.

@@ +17,5 @@
> +        final int pxLeftPadding = getResources().getDimensionPixelSize(R.dimen.find_in_page_text_padding_left);
> +        final int pxRightPadding = getResources().getDimensionPixelSize(R.dimen.find_in_page_text_padding_right);
> +        final int pxLeftMargin = getResources().getDimensionPixelSize(R.dimen.find_in_page_text_margin_left);
> +        final int pxRightMargin =  getResources().getDimensionPixelSize(R.dimen.find_in_page_text_margin_right);
> +        final LinearLayout.LayoutParams params = new  LinearLayout.LayoutParams(0, LinearLayout.LayoutParams.WRAP_CONTENT);

Add an empty line above this, just to help make this more readable.

@@ +21,5 @@
> +        final LinearLayout.LayoutParams params = new  LinearLayout.LayoutParams(0, LinearLayout.LayoutParams.WRAP_CONTENT);
> +        params.weight=1;
> +        params.setMargins(pxLeftMargin, params.topMargin, pxRightMargin, params.bottomMargin);
> +        params.gravity = Gravity.CENTER_VERTICAL|Gravity.LEFT;
> +        setLayoutParams(params);

And then one above this.

::: mobile/android/base/menu/FindBarNavigationButtons.java
@@ +7,5 @@
> +import android.widget.ImageView;
> +import android.widget.LinearLayout;
> +
> +public class FindBarNavigationButtons extends ImageView{
> +    public FindBarNavigationButtons(Context context) {

Ahh, and this is why you're using setActionView up above.... I guess we'll need to do that if we want this custom styling.

::: mobile/android/base/resources/layout/actionbar.xml
@@ +14,5 @@
> +    <Button android:id="@+id/actionmode_title"
> +         	 android:layout_height="wrap_content"
> +            	 android:layout_width="wrap_content"	
> +		 android:layout_marginTop="@dimen/find_in_page_control_margin_top"	
> +            	 style="@style/GeckoActionBar.Title"/>

I don't really want to move these. Lets see what UX says I guess. I have a meeting with them soon about this. I wonder if a better way to do this flip might be to expose a "setLayoutGravity()" property on the ActionModeCompat library instead.

::: mobile/android/base/resources/values/styles.xml
@@ +141,2 @@
>          <item name="android:ellipsize">end</item>
> +        <item name="android:lineSpacingMultiplier">1.3</item>

Remove these changes :)
Attachment #8516531 - Flags: review?(wjohnston) → review+
Just talked to Wes and he helped me understand the issues a bit more.

Let's keep the white UI that comes with the Action bar. That seems to solve a lot of the problems we're running into and I don't see a reason to make a dark Action bar anyways.

Robin will attach icons! :)
Flags: needinfo?(alam)
Sorry for the really late reply. Been a bit busy with the end of term.

So here are the new images for the find in page feature:
[1]http://imgur.com/irIUZ1f
[2]http://imgur.com/NuipqPn

And here is the text selection page now:

[3]http://imgur.com/652mgXP

Let me know what you guys think :)! In particular, I'm not sure if the padding between icons is alright and if I should be reverting the ordering of the elements back to its original order where the cancel is the leftmost element? This album shows what the find in page feature and the text selection look like after reverting their ordering:
http://imgur.com/a/NwexY

 Furthermore, I couldn't use the icons that you attached, Robin, because they were too big but I got the smaller versions from the android website (I'm assuming the ones you provided came from here too).

I won't attach the code again since there are hardly any changes that I made (other than removing trailing whitespace).
Flags: needinfo?(randersen)
Flags: needinfo?(alam)
Hey Koushik,

Let's keep 15dp on both sides so the UI is equally spaced away from the edges of the screen. This would mean less space on the right side, and more on the left. The rest of the spacing in between each icon should be distributed evenly :)

As for ordering, we should probably stick to the current ordering of our controls. I'm having some trouble following this bug though did we mention changing this before?

Do you mind reposting what icons you need for this? maybe I can help you get some.
Flags: needinfo?(alam) → needinfo?(koushik3peri)
This might be our solution for Bug 1113297, which will allow Bug 1050480 to ride the trains.
Blocks: 1113297
Status: NEW → ASSIGNED
No longer depends on: 1050480
OS: Linux → Android
Hardware: x86_64 → All
Flags: needinfo?(wjohnston)
No longer blocks: 1113297
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: