Closed Bug 996850 Opened 10 years ago Closed 10 years ago

Top sites: Empty tile behavior

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(4 files, 2 obsolete files)

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:"
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.
Component: General → Awesomescreen
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #8407718 - Flags: review?(lucasr.at.mozilla)
Attachment #8407718 - Flags: feedback?(oogunsakin)
Attachment #8407720 - Flags: review?(lucasr.at.mozilla)
Attachment #8407720 - Flags: feedback?(oogunsakin)
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?
Flags: needinfo?(ibarlow)
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!
Attachment #8407718 - Flags: feedback?(oogunsakin) → feedback+
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.
Attachment #8407720 - Flags: feedback?(oogunsakin) → feedback+
(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!
Attachment #8407720 - Attachment is obsolete: true
Attachment #8407720 - Flags: review?(lucasr.at.mozilla)
Attachment #8407934 - Flags: review?(lucasr.at.mozilla)
(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.
Flags: needinfo?(ibarlow)
Attachment #8407934 - Flags: review?(lucasr.at.mozilla)
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?
Flags: needinfo?(ibarlow)
Blocks: 997660
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.
Attachment #8407718 - Flags: review?(lucasr.at.mozilla) → review+
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 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.
Attachment #8407934 - Flags: review?(lucasr.at.mozilla)
Blocks: 913457
Comment on attachment 8407934 [details] [diff] [review]
Part 2: Don't allow empty "Edit" entries v2

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

Yep.
Attachment #8407934 - Flags: review?(lucasr.at.mozilla) → review+
(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.
Flags: needinfo?(ibarlow)
Removed displaying toast and strings, carrying over r+.
Attachment #8407934 - Attachment is obsolete: true
Attachment #8410438 - Flags: review+
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.