947850 - [fugu][buri][music] Tap on "Enter" during music search will return to previous page

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

This is still reproducible on latest V1.2 and V1.3.

STR:
1. Load some music to SD card
2. Launch Music
3. On any page of Music app, pull down the screen, search box will appear
4. Tap on Search box, and enter some letters to search
   Search result appears
5. Tap on "Enter" on keyboard
   --> It return to previous page and Keyboard still shows.

I think we should stay on the searching list page after user press "Enter" and keyboard should disappear.

Test build:
Gaia f615ae7acb6731d191b3094e10e314bc28359bbb
Gecko  http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f684b8f159a3
BuildID 20131208004002
Version 26.0 ro.build.version.incremental=eng.archermind.20131114.105818

Comment 1

[Steven Yang [:styang]]

Dominic, could you comment on this issue?

Comment 2

[Dominic Kuo [:dkuo]]

Feels like the default event of keyboard triggers the action to clear the search, I will investigate it.

Comment 3

[Marcia Knous [:marcia]]

Adding myself for testing as requested by Media triage team.

Comment 4

[Hema Koka [:hema]]

(In reply to Dominic Kuo [:dkuo] from comment #2)
> Feels like the default event of keyboard triggers the action to clear the
> search, I will investigate it.

Also it looks like this is happening if the search results are empty. Marcia is going to test and confirm

Comment 5

[Tony Chung [:tchung]]

marcia, when you add your comments, please let us know if its a regression also, in addition to how bad this is.   This will help triage determine if this should block 1.3 or not.

Comment 6

[Marcia Knous [:marcia]]

I tested with Buri and went back to a November 1.2 build to see the behavior when searching:

Gaia   5ec2963fff60492c840707df8d8090f9908a5251
SourceStamp 2d454e0de2ed
BuildID 20131120004000
Version 26.0

Scenario 1:

1. Start with music home page
2. Invoke the search bar and search for a known term in a song
3. Once the word is found and highlighted, press the Enter button

Expected: It would be selected when I press the Enter button and start playing
Actual: Nothing happens and I am return the the music home screen

If I select the term directly from the suggestions the song plays fine.

The same behavior happens with 1.3 and Mozilla Central. I don't believe this to be a regression but if needed I could go back further to the time that search landed, which was Bug 838033. The dropbox UX images no longer link to anything there, so it would be useful to go back and see what the intended behavior was when we designed it.

How bad is it? As long as you select the song directly it still plays, so I would say the current workaround is OK, but not completely desirable.

Comment 7

[James Zhang (Spreadtrum)]

Attachment: music_keyboard_issue.patch

Please fix it on v1.3. It causes bad UE.

Comment 8

[Dominic Kuo [:dkuo]]

Thanks for working on this James, I think you are on the right track and you patch seems fixed this issue, I am redirecting the review to Rudy because this is about the default action that the form will trigger and related to the keyboard, so it's more suitable for him to review.

Comment 9

[Hema Koka [:hema]]

Media Triage 1/15 update: Not a blocker for 1.3 release. But if the patch is relatively low risk and ready to land, we can look into approval

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8359033 [details] [diff] [review]
music_keyboard_issue.patch

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

r=me if you help fixed the nits about indent.
Thank you for working on this.

::: apps/music/js/music.js
@@ +882,5 @@
>  
>          break;
>  
> +      case 'keypress':
> +          if (target.id === 'views-tiles-search-input') {

In general, we use 2 spaces indent instead of 4.

@@ +1477,5 @@
>  
>          break;
>  
> +      case 'keypress':
> +          if (target.id === 'views-list-search-input') {

same here, need to correct indent level.

Comment 11

[Hema Koka [:hema]]

dominic, do you want to get this into 1.4 since you already have a patch (low risk?)

Comment 12

[Dominic Kuo [:dkuo]]

This bug was forgotten and apologize about it! Sherman is fixing bug 1065282 and it look like the same issue, let's see which patch we should use then land it.

Comment 14

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request

Original patch from attachment 8359033 [details] [diff] [review], create github pull request and wait for gaia-try status

Comment 15

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request 2

Original patch from attachment 8487666 [details] [review] in bug 1065282.

Comment 16

[Sherman Chen [:chens] (inactive)]

Pull request 2 tries to consolidate search bar related action in touchend event, and click event will only have to handle play music actions without calling preventDefault. So in this case search form can receive submit event, which implicit meaning the keyboard will close and the following things.

But actually both are workaround based on input_area workaround, and I can't tell which one we should go for. Do you have any preference on it

Comment 17

[Hubert Figuiere [:hub]]

So what the status of this? Dominic? We should try to get this in at least in master.

Comment 18

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to master

Hi Dominic, this patch is originally from attachment 8359033 [details] [diff] [review], and I feel it should be better one to land, could you also review this patch? Thanks!

Comment 19

[Dominic Kuo [:dkuo]]

Comment on attachment 8505217 [details] [review]
Pull request to master

Sherman, the patch looks good to me, I have one minor issue and please take a look on the github comments, see if it make sense to be addressed, also, is it possible for you to write an integration test for this case? I would be very happy to see it happen and let's do it!!!

Comment 20

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8505217 [details] [review]
Pull request to master

Hi Dominic, I've addressed the comments and add some integration tests on it, could you review that again? Thanks!

Comment 21

[Dominic Kuo [:dkuo]]

Sherman, sorry to keep you waiting, the phase 1 of music refactoring(bug 1055043) has landed and that was a huge patch with many changes, would you please rebase you patch first? thanks!!!

Comment 22

[Sherman Chen [:chens] (inactive)]

Thanks, I will rebase and update PR later.

Comment 23

[Dominic Kuo [:dkuo]]

Comment on attachment 8505217 [details] [review]
Pull request to master

Cancel the review flag first because patch needs update.

Comment 24

[Jim Porter (:squib)]

This is fixed on master.