244.86 KB, image/png
250.19 KB, text/plain
46 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
3.16 MB, video/3gpp
Created attachment 8604816 [details] timelinediff.png Description: When a user attempts to import a new track to their ringtones from within the Settings app, the user may observe that the timeline will display "0:00 <--> -0:01", and the preview track does not play initially. The user would expect the track to play upon selecting from their shared list from Music. PreReq: * phone has mp3 songs in Music app Repro Steps: 1) Update a Flame to 20150512010209 2) Open the Settings app 3) Navigate to 'Manage Tones' 4) Tap '+' to observe songs from your Music app 5) Tap a song to open preview 6) Observe UI Actual: Music Timeline displays "0:00 <--> -0:01", does not play on open Expected: Music Timeline displays full length of song, plays on initial load Environmental Variables: -------------------------------------------------- Device: Flame Master Build ID: 20150512010209 Gaia: 6089234ace8b294a8feef064387604bae16254e3 Gecko: 502e1a5e722f Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67 Version: 41.0a1 (Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0 ******************************* Issue has different UI on 2.2 and 2.1 for flame devices: Results: UI displays "00:00 <--> ---:--", song does not play on load Device: Flame 2.2 BuildID: 20150512002502 Gaia: c4c1bf443f2b01c2ba918780510fd4c639a3c360 Gecko: 70782f19acbf Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Device: Flame 2.1 BuildID: 20150512001201 Gaia: c80865cb0bf73f1b97defbc646083b404feb3ac4 Gecko: 2fd3ef3fc14a Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a Version: 34.0 (2.1) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 -------------------------------------------------- Repro frequency: 5/5 See attached: video- https://youtu.be/3hUvmsl4Tgc screenshot logcat
QA Whiteboard: [QAnalyst-Triage?]
Flame 2.2 and 2.1 seems to have the same issue as 3.0 but just exhibit slightly different behavior. NI component owner to see if this should block release
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
status-b2g-v2.1: unaffected → affected
status-b2g-v2.2: unaffected → affected
Flags: needinfo?(pbylenga) → needinfo?(npark)
The reason that 2.2 and 2.1 has different issue could be because of the fix for bug 1150322, but it's strange since that bug is already uplifted as well. Perhaps :djf might be able to see whether it is related. The problem with this bug is that it does not auto start the music. The song does play when the user taps the play button. I'm sort of surprised to see that this is not a regression, but it does not stop the use to check on the ringtone. ni?ing hema for assessment.
First off, this issue is with the Music app's pick activity, and is not specific to ringtones, and it is certainly not a settings app bug, so I've changed the component to music and have edited the bug description. No-Jun says in comment 3 that the bug is that the music does not auto-start. I suspect that was an intentional ux decision. If you're listening to a song and then decide you want it as your ringtone, or decide that you want to email or text it to a friend, you'd like to be able to pick it without interrupting the music that is currently playing. So I think that is correct as it stands. Let's ignore that part. (No-Jun you can consult with UX on that if you want to double-check) From the screenshot that Oliver attached, it seems like the major issue he was reporting is that the display shows a duration of one second when, in fact, we have no idea what the duration of the song is. That part of the bug is 3.0 only and is a (minor) regression. In bug 1150322, I landed a gaia patch to help work around the issue before we had a gecko patch. Once we got the gecko patch we never uplifted the gaia patch to 2.2, so I'm guessing that this one second duration thing on master is a regression that I caused with 1150322. Hub: is this something you have time to take? If not, please assign it to me and I'll get to it once my current batch of 2.2 gallery bugs are under control. Clearing the needinfo on Hema.
status-b2g-v2.1: affected → unaffected
status-b2g-v2.2: affected → unaffected
Component: Gaia::Settings → Gaia::Music
Summary: [Settings][Ringtone] When user attempts to add ringtone from Music, preview does not play initially and timeline has odd display → [Music] Pick activity initially displays -00:01 for the duration instead of ---:--
Thanks for the clarification. :djf is right, the issues can be summarized as: - The selected song does not auto-start (this could be per design) - Counter displays --:-- or -0:01 So I'm guessing the initial display of --:-- is correct, because this is about the ringtone and not about playing a song?
I can reproduce.... --:-- is when we have no duration. -0:01 we got a duration of one second.
David, If I revert your patch for bug 1150322 I get -:-- until I play. Shall we revert it? (I can do it) Thanks
Reverting that patch would re-introduce code that we know to be wrong (the startTime thing), so I'd rather figure out a better fix here. Where is the 00:01 coming from? This happens for songs of any length, so it seems to me that something weird is going on. Is the <audio> element actually reporting a one second duration?
Flags: needinfo?(dflanagan) → needinfo?(hub)
The -0:01 is the '1' that is the "max" value of the seek bar that is set player_view.js:771 when we pass a duration that is NaN with a currentTime of 0 (this change come from bug 1150322). I don't know where that '1' value come from though. If I put a breakpoint at player_view.js:781 (it is first called to "reset" the seek bar) then I end up getting the correct song time displayed - like because this time we do get the currentTime properly. So it looks like that: 1. the changes in bug 1150322 force having a finite endTime that we use to set seekBar.max - but then seekBar.max is already set at 1. 2. we are getting the event "timeupdate"/"durationchange" but the audio.duration isn't set properly yet... (the breakpoint temporise this) I'll dig it more.
As for seekBar.max it is set to 1 because it can't be zero: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress "The max attribute, if present, must have a value greater than zero and be a valid floating point number." So addressing item 1. to not set the endTime if it is not finite ought to be enough to keep the previous behaviour.
Assignee: nobody → hub
Status: NEW → ASSIGNED
Comment on attachment 8608327 [details] [review] [gaia] hfiguiere:bug1164148 > mozilla-b2g:master cancelling review for now. After looking more closely, it looks like the fix has to be more thorough in the way we deal with the <progress> element.
Comment on attachment 8608327 [details] [review] [gaia] hfiguiere:bug1164148 > mozilla-b2g:master I left a couple of comments on github: you probably shouldn't set the accessiblity properties when we have an unknown duration. Otherwise, the patch looks okay. (I didn't test it though). Note that if gecko ever returns an infinite duration then this patch will take us back to where we were in 1150322 with the thumb displayed to the far left instead of the far right. Another approach would be to leave in the Math.max() for endTime but add a new flag that specifies whether we've ever gotten a valid duration from gecko. If that flag is not set (no valid duration received) then the code that actually displays the remaining time in the song can just always display "---:--". Actually, you probably don't even need a flag to do it that way: maybe in the code that displays the remaining time, you could just use audio.duration instead of slider.maxValue, and if duration is not finite, then always display "---:--". I'd slightly prefer that approach, but I'm okay with landing this patch as it stands if you'd prefer to just be done with this for now. Also, I like the simplification in this patch of getting rid of the startTime argument.
Attachment #8608327 - Flags: review?(dflanagan) → review+
Thanks. Dealt with the aria-maxvalue case and rebased. When we support stream we'll have to revisit the slider anyway.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
This bug has been verified as "pass" on latest Nightly build of Flame v3.0 by the STR in Comment 0. Actual results: Music Timeline displays full length of song and plays on initial load, and it does not shows the "-00:01" before music plays. See attachment: verified_v3.0.mp4 Reproduce rate: 0/6 Device: Flame v3.0 build(Pass) Build ID 20150614010203 Gaia Revision 1bf2da102560481748ff3f6202fbed5c4daa5832 Gaia Date 2015-06-13 00:22:05 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/c223b8844264 Gecko Version 41.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150614.044513 Firmware Date Sun Jun 14 04:45:25 EDT 2015 Bootloader L1TC000118D0
Status: RESOLVED → VERIFIED
status-b2g-master: affected → verified
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.