996850 - Top sites: Empty tile behavior

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: "user-entered:" tile name

This might be moot with the new Tiles behavior, but a couple of things I noticed:

- Don't show a context menu for an empty tile - none of these options (Share, Add to home screen, Edit) are useful
- From the edit dialog, if the user doesn't enter anything and hits enter, the tile should revert back to "Add a site" instead of "user-entered:"

Comment 1

[Wesley Johnston (:wesj)]

Now that we support actionmodes across all Android versions, I wonder if its worth using that here instead of context menus like we originally tried.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Part 1: Don't show context menu for empty tile

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Part 2: Don't allow empty "Edit" entries

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Edit text toast

Ian, we should allow users to hit "enter" if they don't have any text entered in the Top Sites "Edit" dialog. What do you think about this Toast text?

Comment 5

[Sola Ogunsakin [:sola]]

Comment on attachment 8407718 [details] [diff] [review]
Part 1: Don't show context menu for empty tile

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

looks good to me!

Comment 6

[Sola Ogunsakin [:sola]]

Comment on attachment 8407720 [details] [diff] [review]
Part 2: Don't allow empty "Edit" entries

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

Looks good! Just a small edge case.

::: mobile/android/base/home/PinSiteDialog.java
@@ +110,5 @@
>  
>                  // If the user manually entered a search term or URL, wrap the value in
>                  // a special URI until we can get a valid URL for this bookmark.
>                  final String text = mSearch.getText().toString();
> +                if (!TextUtils.isEmpty(text)) {

Don't believe this handles the case where a user enters only whitespace.

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Part 2: Don't allow empty "Edit" entries v2

(In reply to Sola Ogunsakin [:sola] from comment #6)
> Comment on attachment 8407720 [details] [diff] [review]
> Part 2: Don't allow empty "Edit" entries
> 
> Review of attachment 8407720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Just a small edge case.
> 
> ::: mobile/android/base/home/PinSiteDialog.java
> @@ +110,5 @@
> >  
> >                  // If the user manually entered a search term or URL, wrap the value in
> >                  // a special URI until we can get a valid URL for this bookmark.
> >                  final String text = mSearch.getText().toString();
> > +                if (!TextUtils.isEmpty(text)) {
> 
> Don't believe this handles the case where a user enters only whitespace.

Good catch!

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #4)
> Created attachment 8407722 [details]
> Screenshot: Edit text toast
> 
> Ian, we should allow users to hit "enter" if they don't have any text
> entered in the Top Sites "Edit" dialog. What do you think about this Toast
> text?

I think i would prefer to disable the Enter/Go button, to be honest. Showing a toast feels like a bit of a strange workaround.

Comment 9

[Chenxia Liu [:liuche]]

Ian, sorry, I'm not sure there's a way to enable/disable keys on the soft keyboard on the fly based on what's been typed - we can only intercept the enter key when it's been pressed and then "do something". I tried a few things (and also googled a bit), but without success. (Keep in mind this dialog is different from the edit bookmark or history dialog in that it doesn't have a "Go" button, just a text box and any search entries that match what's been typed, and solely relies on keyboard input.)

I can't think of any Android apps off the top of my head that do that - do you know of any?

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8407718 [details] [diff] [review]
Part 1: Don't show context menu for empty tile

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

Looks good.

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +110,5 @@
>      /**
> +     * @return true, if this view has no content to show.
> +     */
> +    public boolean isEmpty() {
> +        return mIsEmpty;

mIsEmpty is going away in bug 997660. Added a dependency as a note to self to rebase my patches.

::: mobile/android/base/home/TopSitesGridView.java
@@ +124,5 @@
>              @Override
>              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
>                  Cursor cursor = (Cursor) parent.getItemAtPosition(position);
>  
> +                TopSitesGridItemView gridView = (TopSitesGridItemView) view;

nit: rename gridView to item for clarity.

@@ +125,5 @@
>              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
>                  Cursor cursor = (Cursor) parent.getItemAtPosition(position);
>  
> +                TopSitesGridItemView gridView = (TopSitesGridItemView) view;
> +                if (cursor == null || gridView.isEmpty()) {

Note to self: this will become item.getType() == TopSites.TYPE_BLANK.

Comment 11

[Chenxia Liu [:liuche]]

After further investigation, I don't think there is a way to disable a specific key on the keyboard. We do switch keyboard IME types in ToolbarEditText, but I don't think there's a keyboard that lacks an "action" key (be that return, "Done", or "Next"), so I don't think there's a workaround for disabling the return key.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8407934 [details] [diff] [review]
Part 2: Don't allow empty "Edit" entries v2

Ian, is there something else that you'd like to see instead, other than a toast? I haven't been able to find something that fits what you're asking for regarding the keyboard.

Lucas, if you have another suggestion, that's welcome too! I tried actionNone, flagNoEnterAction, and flagNoAccessoryAction in imeOptions, but none of them remove the action button.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8407934 [details] [diff] [review]
Part 2: Don't allow empty "Edit" entries v2

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

Yep.

Comment 14

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #12)
> Comment on attachment 8407934 [details] [diff] [review]
> Part 2: Don't allow empty "Edit" entries v2
> 
> Ian, is there something else that you'd like to see instead, other than a
> toast? I haven't been able to find something that fits what you're asking
> for regarding the keyboard.

I guess my only other suggestion would be that if there isn't any text in the url bar and the user hits Go, nothing should happen. Not even a toast.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Part 2: Don't allow empty "Edit" entries v3

Removed displaying toast and strings, carrying over r+.

Comment 16

[Chenxia Liu [:liuche]]

Part 1: https://hg.mozilla.org/integration/fx-team/rev/a4891371e28e

Part 2 and backout (for Toast):
https://hg.mozilla.org/integration/fx-team/rev/fcd1191c12b5
https://hg.mozilla.org/integration/fx-team/rev/3f2032f4dad7

Part 2 reland:
https://hg.mozilla.org/integration/fx-team/rev/4c8ee55dbdd1

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a4891371e28e
https://hg.mozilla.org/mozilla-central/rev/4c8ee55dbdd1