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

RESOLVED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Josh Schmitt [Joshs], Assigned: russn)

Tracking

({regression})

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
regression
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8440032 [details]
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
(Reporter)

Comment 1

4 years ago
Created attachment 8440039 [details]
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
(Reporter)

Comment 2

4 years ago
Qawanted to confirm and to check other devices and 2.1 Flame
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v1.4: --- → unaffected
Keywords: qawanted, regression
(Reporter)

Comment 3

4 years ago
Correction: 

Qawanted to check other devices.

Comment 4

4 years ago
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.

Comment 5

4 years ago
Created attachment 8441167 [details]
IMG_0001.jpg

Image showing the slider overlaps the video time when playback ends

Comment 6

4 years ago
Created attachment 8442017 [details] [review]
PR

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)

Comment 8

4 years ago


(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)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?]

Comment 9

4 years ago
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?]
status-b2g-v2.1: --- → affected
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)

Updated

4 years ago
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)

Updated

4 years ago
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)
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8442017 - Flags: review?(rnicoletti) → review+

Updated

4 years ago
Attachment #8442017 - Flags: feedback?(amlee)

Comment 15

4 years ago
Created attachment 8491300 [details] [diff] [review]
BUG_1025147.patch

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)
(Assignee)

Updated

4 years ago
Attachment #8491300 - Flags: review?(rnicoletti) → review+

Updated

4 years ago
blocking-b2g: --- → 2.0M+

Updated

4 years ago
Duplicate of this bug: 1068497
Keywords: checkin-needed
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

Comment 19

4 years ago
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)

Comment 20

4 years ago
Created attachment 8496815 [details]
Pointer to Pull Request.html

PR is created for this issue.
Attachment #8491300 - Attachment is obsolete: true

Comment 21

4 years ago
This is a generic bug, shouldn't be 2.0M+ but 2.0+.
blocking-b2g: 2.0M+ → 2.0+
status-b2g-v2.0M: --- → affected

Comment 22

4 years ago
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
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1043702
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(dharris)
(Assignee)

Comment 26

4 years ago
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
(Assignee)

Comment 27

4 years ago
Master: https://github.com/mozilla-b2g/gaia/commit/96c1272cd2623616b8fa77237cc48cb82951d953
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

4 years ago
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)
(Assignee)

Comment 29

4 years ago
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)

Comment 30

4 years ago
hi, would you please help to confirm if this patch checked in woodduck branch? Thank you very much!

Comment 31

4 years ago
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.

Comment 32

4 years ago
(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?
status-b2g-v2.2: --- → fixed
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+

Updated

4 years ago
Blocks: 1054172

Updated

4 years ago
Blocks: 1080481
Created attachment 8524457 [details]
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Created attachment 8525120 [details]
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
status-b2g-v2.0M: fixed → verified
You need to log in before you can comment on or make changes to this bug.