Closed
Bug 947850
Opened 11 years ago
Closed 9 years ago
[fugu][buri][music] Tap on "Enter" during music search will return to previous page
Categories
(Firefox OS Graveyard :: Gaia::Music, defect)
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: angelc04, Assigned: chens)
References
Details
(Whiteboard: [priority])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
rudyl
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-github-pull-request
|
Details |
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
Updated•11 years ago
|
Flags: needinfo?(styang)
Comment 1•11 years ago
|
||
Dominic, could you comment on this issue?
Flags: needinfo?(styang) → needinfo?(dkuo)
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Comment 2•11 years ago
|
||
Feels like the default event of keyboard triggers the action to clear the search, I will investigate it.
Assignee: nobody → dkuo
Comment 3•11 years ago
|
||
Adding myself for testing as requested by Media triage team.
Flags: needinfo?(mozillamarcia.knous)
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(mozillamarcia.knous)
Comment 7•11 years ago
|
||
Please fix it on v1.3. It causes bad UE.
Attachment #8359033 -
Flags: review?(ehung)
Flags: needinfo?(ehung)
Comment 8•11 years ago
|
||
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.
Flags: needinfo?(ehung)
Flags: needinfo?(dkuo)
Updated•11 years ago
|
Attachment #8359033 -
Flags: review?(ehung) → review?(rlu)
Updated•11 years ago
|
Comment 9•11 years ago
|
||
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
blocking-b2g: 1.3? → ---
Comment 10•11 years ago
|
||
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.
Attachment #8359033 -
Flags: review?(rlu) → review+
Comment 11•11 years ago
|
||
dominic, do you want to get this into 1.4 since you already have a patch (low risk?)
Flags: needinfo?(dkuo)
Whiteboard: [priority]
Comment 12•10 years ago
|
||
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.
Flags: needinfo?(dkuo)
Updated•10 years ago
|
Assignee: dkuo → nobody
Assignee | ||
Comment 14•10 years ago
|
||
Original patch from attachment 8359033 [details] [diff] [review], create github pull request and wait for gaia-try status
Assignee | ||
Comment 16•10 years ago
|
||
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
Flags: needinfo?(dkuo)
Comment 17•10 years ago
|
||
So what the status of this? Dominic? We should try to get this in at least in master.
Assignee | ||
Updated•10 years ago
|
Attachment #8493475 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8493478 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
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!
Assignee: nobody → shchen
Attachment #8505217 -
Flags: review?(dkuo)
Comment 19•10 years ago
|
||
Comment on attachment 8505217 [details]
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!!!
Flags: needinfo?(dkuo)
Attachment #8505217 -
Flags: review?(dkuo)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8505217 [details]
Pull request to master
Hi Dominic, I've addressed the comments and add some integration tests on it, could you review that again? Thanks!
Attachment #8505217 -
Flags: review?(dkuo)
Comment 21•10 years ago
|
||
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!!!
Flags: needinfo?(shchen)
Assignee | ||
Comment 22•10 years ago
|
||
Thanks, I will rebase and update PR later.
Flags: needinfo?(shchen)
Comment 23•10 years ago
|
||
Comment on attachment 8505217 [details]
Pull request to master
Cancel the review flag first because patch needs update.
Attachment #8505217 -
Flags: review?(dkuo)
Comment 24•9 years ago
|
||
This is fixed on master.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•