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

Status: RESOLVED FIXED

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

Description

[Josh Schmitt [Joshs]]

Attachment: 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

Comment 1

[Josh Schmitt [Joshs]]

Attachment: 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

Comment 2

[Josh Schmitt [Joshs]]

Qawanted to confirm and to check other devices and 2.1 Flame

Comment 3

[Josh Schmitt [Joshs]]

Correction: 

Qawanted to check other devices.

Comment 4

[amitav.anand]

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

[amitav.anand]

Attachment: IMG_0001.jpg

Image showing the slider overlaps the video time when playback ends

Comment 6

[amitav.anand]

Attachment: PR

Kindly review the PR.

Comment 7

[Tiffanie Shakespeare [:tif]]

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!

Comment 8

[Amy Lee [:amylee] UX]

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

Comment 9

[Hung Nguyen]

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

Comment 10

[Montoya Clemmons (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

Comment 11

[Joshua Mitchell (Inactive)]

regression but not nomming; seems like a polish-issue. Nice to have but not urgent.

Comment 12

[David Flanagan [:djf]]

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

Comment 13

[David Flanagan [:djf]]

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 14

[Russ Nicoletti [:russn]]

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.

Comment 15

[vsireesha246]

Attachment: 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

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Since Sireesha had got r+ and had a great work on this, I change the assignee to her. Thanks Sireesha.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Can you please post a pull request so we can get a test run prior to committing? Thanks!

Comment 19

[vsireesha246]

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!

Comment 20

[vsireesha246]

Attachment: Pointer to Pull Request.html

PR is created for this issue.

Comment 21

[Evelyn Hung]

This is a generic bug, shouldn't be 2.0M+ but 2.0+.

Comment 22

[Hema Koka [:hema]]

MClemmons or No-Jun,

Could you please test this patch out?

Thanks
Hema

Comment 23

[No-Jun Park [:njpark]]

I verified the patch on master branch, and it looks good, works fine with portrait/landscape mode on short/long videos.

Comment 24

[David Flanagan [:djf]]

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.

Comment 26

[Russ Nicoletti [:russn]]

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

Comment 27

[Russ Nicoletti [:russn]]

Master: https://github.com/mozilla-b2g/gaia/commit/96c1272cd2623616b8fa77237cc48cb82951d953

Comment 28

[Russ Nicoletti [:russn]]

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]:

Comment 29

[Russ Nicoletti [:russn]]

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

Comment 30

[woodduck]

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

Comment 31

[Josh Cheng [:josh]]

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

[woodduck]

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

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8496815 [details]
Pointer to Pull Request.html

looks low risk enough to uplift given this is a 2.0 regression.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/bc8eb493311c58f1f311a56b8b645b52bfbd2f71
v2.0: https://github.com/mozilla-b2g/gaia/commit/9791940d5093c37336dcd85f7e66e9a8445a218e

Comment 35

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/9791940d5093c37336dcd85f7e66e9a8445a218e

Comment 36

[Shine(Leave from Mozilla)]

Attachment: 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

Comment 37

[Shine(Leave from Mozilla)]

Attachment: 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