1050480 - Add a match case option to find-in-page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Desktop has this now. We should add support. I think this will depend on having a menu of some sort. That will be handled automatically by the actionmode in bug 1050479 eventually, so I think this will depend on that bug.

Comment 1

[Mark Capella [:capella]]

Attachment: bug1050480.diff

Here's a follow-on after bug 1077574.

I'm probably almost certainly going to need a graphics person to properly do my roughed resources for the match case / no-match case icons.

Pinging tecgirl(?)

   https://www.dropbox.com/s/v116an02cgif2xd/bug1050480_matchCaseOff.png?dl=0
   https://www.dropbox.com/s/z9w5uxyudubaqcn/bug1050480_matchCaseOn.png?dl=0

Comment 2

[Robin Andersen [:tecgirl]]

(In reply to Mark Capella [:capella] from comment #1)
> Created attachment 8502873 [details] [diff] [review]
> bug1050480.diff
> 
> Here's a follow-on after bug 1077574.
> 
> I'm probably almost certainly going to need a graphics person to properly do
> my roughed resources for the match case / no-match case icons.
> 
> Pinging tecgirl(?)
> 
>    https://www.dropbox.com/s/v116an02cgif2xd/bug1050480_matchCaseOff.png?dl=0
>    https://www.dropbox.com/s/z9w5uxyudubaqcn/bug1050480_matchCaseOn.png?dl=0

I think we can do this without an asset and just use text to convey the match case selection. Use 4dp bigger text (to match same y-height), same family and color as x/x, but for the "do not match case state" use color #d02626.

See: http://cl.ly/XzUc

Comment 3

[Mark Capella [:capella]]

Attachment: bug1050480.diff

Oh, well that makes things a whole lot easier -and- looks better  :-D. . .

Comment 4

[Mark Capella [:capella]]

going ahead with a try push
https://tbpl.mozilla.org/?tree=Try&rev=a2f387d1ebe9

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8503479 [details] [diff] [review]
bug1050480.diff

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

I really would rather this went in an overflow menu. I guess that would just add one-more icon in here (but I would rather a menu icon than another one of these icons). I got worried that bug 1050479 had fallen off, but saw a comment in there I had just missed. That said, I'd probably r+ except I want this to use color-state-lists.

::: mobile/android/base/FindInPageBar.java
@@ +25,5 @@
>  import android.widget.LinearLayout;
>  import android.widget.TextView;
>  
>  public class FindInPageBar extends LinearLayout implements TextWatcher, View.OnClickListener, GeckoEventListener  {
> +    private static final String LOGTAG = "FindInPageBar";

Preface this with "Gecko"

@@ +133,5 @@
> +            mMatchCase.setTextColor(
> +                (mMatchCase.getCurrentTextColor() == getResources().getColor(R.color.find_matchcase_on)) ?
> +                    getResources().getColor(R.color.find_matchcase_off) :
> +                    getResources().getColor(R.color.find_matchcase_on)
> +            );

The prettier way to do this might be to use a CheckedTextView. Then you can just set the text color to a color-state-list that has different colors listed for checked/unchecked states. And you can use some nice textView.toggle() methods.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +550,5 @@
>       their desktop or laptop.-->
>  <!ENTITY remote_tabs_last_synced "Last synced: &formatS;">
> +
> +<!-- Find-In-Page strings -->
> +<!ENTITY find_matchcase "Aa">

This probably needs a localization note that its meant to appear as an icon that changes color if match-case is activated. i.e. No more than two letters.

::: mobile/android/base/resources/values/dimens.xml
@@ +148,5 @@
>      <dimen name="find_in_page_text_margin_right">12dip</dimen>
>      <dimen name="find_in_page_text_padding_left">10dip</dimen>
>      <dimen name="find_in_page_text_padding_right">10dip</dimen>
> +    <dimen name="find_in_page_status_margin_right">15dip</dimen>
> +    <dimen name="find_in_page_matchcase_margin_right">10dip</dimen>

These margins are feeling rather random to me still.

1.) Do we have margins like these for other "actionbar" type icons anywhere
2.) Can we make these common so that all of the icons can inherit them?

::: mobile/android/chrome/content/FindHelper.js
@@ +32,5 @@
>    },
>  
>    _findOpened: function() {
>      Messaging.addListener((data) => {
> +      let args = JSON.parse(JSON.stringify(data));

Do you need this? Why?

Comment 6

[Mark Capella [:capella]]

Attachment: bug1050480.diff

re: |These margins are feeling rather random to me still.|

Yes, I'm artistically challenged :( 

I'm not really padding for the icons, as much as the spacing around the new matchCase CheckedTextView indicator. I'd adjusted for appearance based on testing on my phone and tablet, and in a variety of orientation, and results (345454/9987554 vs 1/2, and etc).

Is there a simpler / better way to do this bit of layout design?

Comment 7

[Mark Capella [:capella]]

Attachment: bug1050480.diff

Re-based ...

Comment 8

[Mark Capella [:capella]]

Push to try-server
https://tbpl.mozilla.org/?tree=Try&rev=f521e8d8a31a

Comment 9

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/48f774595580

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48f774595580

Comment 11

[Nick Alexander :nalexander [he/him]]

A string from this ticket landed in services/strings.xml.in; I'll put it in the correct place.

Comment 12

[Mark Capella [:capella]]

Oh ouch... thanks.

Comment 13

[Richard Newman [:rnewman]]

This is being turned off in non-Nightly builds by Bug 1113296 until we address some issues:

https://hg.mozilla.org/integration/fx-team/rev/a208ca082735

I'm marking this as 'disabled' in 37, because it'll only be enabled in 37 until 37 leaves Nightly. I'll mark 36 as 'disabled' when we uplift Bug 1113296.

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

Nightly only feature at this time. Setting Nightly+