Closed Bug 1025147 Opened 6 years ago Closed 5 years ago

[B2G][Video]The slider overlaps the video time length when the playback ends

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.0+
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jschmitt, Assigned: rnicoletti)

References

Details

(Keywords: regression, Whiteboard: [2.1-flame-test-run-3])

Attachments

(6 files, 2 obsolete files)

Attached file log.txt
Description:
When a video is finished playing the slider overlaps the time length before reseting to the beginning.

Prerequisite: Have a video on the DUT

Repro Steps:
1) Update a Flame to 20140613000203
2) Open Video app
3) Select a video and play

Actual:
The slider overlaps the video time length

Expected:
The slider does not overlap the time

Environmental Variables:
Device: Flame
Build ID: 20140613000203
Gaia: a3a5322692578e0a577fb7fa08e32144b2b05ba3
Gecko: 8897bc43f59b
Version: 32.0a2 (2.0) 
Firmware Version: v121-2

User Agent: Mozilla/5.0 (Mobile;rv:32.0) Gecko/32.0 FIrefox/32.0

Notes:
Repro frequency: 100%
See attached: screenshot, logcat
Attached image Video.png
Issue does NOT repro on 1.4 Flame

v1.4 Environmental Variables
Device: Flame v1.4
BuildID: 20140613000202
Gaia: 1dae62556e642b0b2e08689e35e24e56daa8c79b
Gecko: 30224c7f5e58
Version: 30.0
Firmware Version: v121-2
Qawanted to confirm and to check other devices and 2.1 Flame
QA Whiteboard: [QAnalyst-Triage?]
Keywords: qawanted, regression
Correction: 

Qawanted to check other devices.
This issue is reproducible on Nexus 4. The build details are below:-

OS Version : 2.0.0.0-prerelease
Hardware Revision : Mako
Platform Version : 32.0a1
Build Identifier : 20140613193818
Git Commit Info : 2014-06-13 13:16:40

Please check the attached image.
Attached image IMG_0001.jpg
Image showing the slider overlaps the video time when playback ends
Attached file PR (obsolete) —
Kindly review the PR.
Attachment #8442017 - Flags: review?(tshakespeare)
Comment on attachment 8442017 [details] [review]
PR

Hey Amitav! 

Don't forget that I'm not a developer so I do UI-reviews not code reviews - I've switched the flag.

I think David might be able to do the review?, if not he'll know who can.

Overall, nice catch. Do you think you can put just a little more padding between the circle and the counter? It appears to be overlapping ever so slightly still. I'm also going to include Amy from visual design to take a look. 

Amy, feel free to pass to Hung if you don't have time.

Thanks guys!
Attachment #8442017 - Flags: ui-review-
Attachment #8442017 - Flags: review?(tshakespeare)
Attachment #8442017 - Flags: feedback?(amlee)
Flags: needinfo?(dflanagan)


(In reply to Tiffanie Shakespeare from comment #7)
> Comment on attachment 8442017 [details] [review]
> PR
> 
> Hey Amitav! 
> 
> Don't forget that I'm not a developer so I do UI-reviews not code reviews -
> I've switched the flag.
> 
> I think David might be able to do the review?, if not he'll know who can.
> 
> Overall, nice catch. Do you think you can put just a little more padding
> between the circle and the counter? It appears to be overlapping ever so
> slightly still. I'm also going to include Amy from visual design to take a
> look. 
> 
> Amy, feel free to pass to Hung if you don't have time.
> 
> Thanks guys!

Hung had worked on video player refinements so I'm not sure if this is a regression (In reply to Tiffanie Shakespeare from comment #7)
> Comment on attachment 8442017 [details] [review]
> PR
> 
> Hey Amitav! 
> 
> Don't forget that I'm not a developer so I do UI-reviews not code reviews -
> I've switched the flag.
> 
> I think David might be able to do the review?, if not he'll know who can.
> 
> Overall, nice catch. Do you think you can put just a little more padding
> between the circle and the counter? It appears to be overlapping ever so
> slightly still. I'm also going to include Amy from visual design to take a
> look. 
> 
> Amy, feel free to pass to Hung if you don't have time.
> 
> Thanks guys!

Hung had worked on video player refinements so I'm not sure why the player is showing up like this.
Flags: needinfo?(hnguyen)
QA Whiteboard: [QAnalyst-Triage?]
The video player code is actually a duplicate of the music player. I don't see this happening in music so can we ensure the scrubber has the same code also? 

Thanks
Flags: needinfo?(hnguyen)
QA Contact: mclemmons
(In reply to Josh Schmitt from comment #3)
> Correction: 
> 
> Qawanted to check other devices.

This issue does reproduce on Flame 2.1 and Buri 2.0. Following STR from Comment 0, the slider overlaps the video time length.

Environmental Variables:
Device: Flame Master
Build ID: 20140627040205
Gaia: b8f36518696f3191a56e4f33447ee9d6ec820da1
Gecko: 9290d7995f98
Version: 33.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Environmental Variables:
Device: Buri 2.0
Build ID: 20140627000201
Gaia: 8df02268fcd7e80c5fab8c1ec099772e37f8659d
Gecko: 731a5e8831e6
Version: 32.0a2 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
regression but not nomming; seems like a polish-issue. Nice to have but not urgent.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Sorry I dropped the ball here.  Russ, would you take a look at the attached patch and see if it fixes the problem and looks good to you?  (And whether it fixes the problem in both versions of the video player?)
Flags: needinfo?(dflanagan)
Attachment #8442017 - Flags: review?(rnicoletti)
Amitav,

Thanks for this patch. I've asked Russ to review it. Please note that in comment 7, our UX designer has asked you to make some tweaks.  Also, keep in mind that the video app has a separate set of code for playing videos by the view activity, so you might want to check whether a fix is needed for that case as well (or verify that this CSS is shared by both players)
Comment on attachment 8442017 [details] [review]
PR

Regarding whether the patch addresses both use cases, playing videos from the video app and playing videos from other apps (e.g., as an email attachment), the answer is yes, that patch addresses both use cases. 

In addition, I will reiterate what Tiffanie mentioned in comment #7, that the slider still overlaps very slightly with the duration text.

I'm giving r+ with the assumption the css will be tweaked to address the (small) spacing issue that remains.
Attachment #8442017 - Flags: review?(rnicoletti) → review+
Attachment #8442017 - Flags: feedback?(amlee)
Attached patch BUG_1025147.patch (obsolete) — Splinter Review
Hi Russ,

I modified the existing PR to avoid the below things.

1.Slight overlap 
2.Play Head is moving outof progress bar.

So,please review the changes.If the patch is fine i will upload the PR for the same.

Thanks..
Sireesha
Attachment #8491300 - Flags: review?(rnicoletti)
Attachment #8491300 - Flags: review?(rnicoletti) → review+
blocking-b2g: --- → 2.0M+
Duplicate of this bug: 1068497
Assignee: nobody → vsireesha246
Since Sireesha had got r+ and had a great work on this, I change the assignee to her. Thanks Sireesha.
Can you please post a pull request so we can get a test run prior to committing? Thanks!
Flags: needinfo?(vsireesha246)
Keywords: checkin-needed
Attachment #8442017 - Attachment is obsolete: true
I will post PR soon.

Thanks..
Sireesha

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> Can you please post a pull request so we can get a test run prior to
> committing? Thanks!
Flags: needinfo?(vsireesha246)
PR is created for this issue.
Attachment #8491300 - Attachment is obsolete: true
This is a generic bug, shouldn't be 2.0M+ but 2.0+.
blocking-b2g: 2.0M+ → 2.0+
MClemmons or No-Jun,

Could you please test this patch out?

Thanks
Hema
Flags: needinfo?(npark)
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(dharris)
Whiteboard: [2.1-flame-test-run-3]
I verified the patch on master branch, and it looks good, works fine with portrait/landscape mode on short/long videos.
Flags: needinfo?(npark)
Russ,

I'm assigning this to you to land on master and request uplift to 2.1 and 2.0. Please don't land Sireesha's PR directly, however because it changes the executable bit of the CSS file (which means that Sireesha uses Windows, I think).  

Also, you might want to verify that the extra spacing that Tif requested in comment 7 has been put in place.
Assignee: vsireesha246 → rnicoletti
Duplicate of this bug: 1043702
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(dharris)
The duplicate bug (bug 1043702) has a patch that is a bit cleaner than the PR for this bug. I have added some comments to the css of the bug 1043702 patch and have created a new PR [1]. I will land this new PR when the try server run is green.

[1] https://github.com/mozilla-b2g/gaia/pull/24905
Status: NEW → ASSIGNED
Master: https://github.com/mozilla-b2g/gaia/commit/96c1272cd2623616b8fa77237cc48cb82951d953
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8496815 [details]
Pointer to Pull Request.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8496815 - Flags: approval-gaia-v2.0?(bbajaj)
Comment on attachment 8496815 [details]
Pointer to Pull Request.html

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
As far as I can tell, bug has existed for some time.

[User impact] if declined:
Bad user experience, makes app look unprofessional.

[Testing completed]:
Manual testing 

[Risk to taking this patch] (and alternatives if risky):
Low risk, changes were to a single video css file. Changes do not affect translatable text so there is no danger the patch will affect when language is non-english.

[String changes made]:
None
Attachment #8496815 - Flags: approval-gaia-v2.0?(bbajaj)
hi, would you please help to confirm if this patch checked in woodduck branch? Thank you very much!
Hi Woodduck,
The patch approval for landing on 2.0 is request for approval by Russ. Once it approved. the fix will land on 2.0 and merge to 2.0M. 
You will have the fix then and should be before next Friday.
(In reply to Josh Cheng from comment #31)
> Hi Woodduck,
> The patch approval for landing on 2.0 is request for approval by Russ. Once
> it approved. the fix will land on 2.0 and merge to 2.0M. 
> You will have the fix then and should be before next Friday.

Hi Josh, do you mean before 10/17? As I know, our SQC will be end in this week. Thanks!
Attachment #8496815 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.1?
Target Milestone: --- → 2.1 S6 (10oct)
Comment on attachment 8496815 [details]
Pointer to Pull Request.html

looks low risk enough to uplift given this is a 2.0 regression.
Attachment #8496815 - Flags: approval-gaia-v2.1?
Attachment #8496815 - Flags: approval-gaia-v2.1+
Attachment #8496815 - Flags: approval-gaia-v2.0?(bbajaj)
Attachment #8496815 - Flags: approval-gaia-v2.0+
Blocks: Woodduck
Blocks: 1080481
Attached image screenshoot
This issue can't repro on Flame 2.0, 2.1, 2.2
See attachment: Verify_screenshot.png
Reproducing rate: 0/5

Woodduck2.0 build:
Gaia-Rev        60146ec47cd38a8be8ed22e0116902eceb9ac067
Gecko-Rev       cdfbe9866cf0b71b33454926638ce0cd8bb1fb00
Build-ID        20141117050313
Version         32.0
Flame 2.1 build:
Gaia-Rev        81160ad79e5b4c21967418dd63f1a1d08d77924e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3572aa3e6766
Build-ID        20141117001201
Version         34.0

Flame 2.2 build:
Gaia-Rev        ae3a84acaab80a5b35d5542d63e68462273c8a1b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2d0a51ef828d
Build-ID        20141117160200
Version         36.0a
Attached image Verify_woodduck.png
This issue can't repro on Woodduck2.0
See attachment: Verify_woodduck.png
Reproducing rate: 0/5

Woodduck 2.0 biuld:
Gaia-Rev        cc690f8016b672475dc186bc7fd58aef45e684b7
Gecko-Rev       03d3ab62d5b07b915434f2d1d68495ad5915ecd2
Build-ID        20141118184148
Version         32.0
You need to log in before you can comment on or make changes to this bug.