951025 - [Video]Fast forward/Rewind option in Video app

Status: RESOLVED FIXED

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

Description

[vsireesha246]

1.There is no forward/Rewind buttons in video app to fast forward and Rewind the current playing video.

Proposed Scenario:Video app should have the forward and rewind buttons like in music app.

2.Like Music app on single touch on FF/Rwd buttons move the next music file and on long touch on FF/Rwd buttons will FF/Rwd the current music file.
Similarly same behavior should be present in Video app.

Please suggest any change related to the above two features.

Comment 1

[vsireesha246]

Would you please provide the feedback for this issue.

Comment 2

[Wayne Chang [:wchang]]

Over to Sri.

However as IIRC we had a discussion on this and given there is the progress bar where you can seek it is not essential to have FWD or RWD buttons.

Comment 3

[vsireesha246]

This issue UX can be implemented in two ways.

1.Similar to Music app(refer the attached screenshot)
2.By reducing the seek-bar size and add the Previous button to left of seek-bar and next button to right.

So please suggest me which UX i need to follow to implement this issue.
As this is the the mandatory item for MADAI,i will upload the patch once you conform me about UX spec.

Comment 4

[vsireesha246]

Attachment: Video_Proposed_UI.png

Comment 5

[Wayne Chang [:wchang]]

cc'ing Mike and Rob

Comment 6

[Dominic Kuo [:dkuo]]

If I recalled correctly the UX of video player is Chris who is already in the cc'ing list.

Comment 7

[GoodMike]

Yes. I just cc Chris.

My comment is that it is good to have FF/REW in video app.
1.Same behavior as Music.
2.Easy for navigation. ( In current video, user always has to go back to video list to select next or another clip.
p.s. But for the video player in Gallery, we shall keep current implementation.

Thanks.

Comment 8

[vsireesha246]

Attachment: Bug_951025.patch

Would you please review the attached Work in Progress patch and provide me the review comments.

Thanks,
Sireesha

Comment 9

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

It's an UI change of video player. Let's ask Chris about the proposed UI first.

Chris, may you take a look at the attached UI at comment 4 and give us some input about the changes of this feature?

Comment 10

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

BTW,

I can't make sure the different between this bug and bug 948260. Are they duplicated??

Comment 11

[vsireesha246]

This bug and bug-948260 are almost similar.
But in this bug,i added one more feature,like on long press on FF/Rwd buttons we can do seeking also.

Comment 12

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

IMHO, the bug 948260 is for previous/next video navigation and this one is for fast forwarding or rewinding within a video. So, we should still view them as two separate features.

Comment 13

[GoodMike]

UX did the review on the patch (using Hamachi) and here is the feedback : 
In the Video, the FWD and REW icons are not visible but it works when I touch on the right and left side of the Play button.

Issue :
In the patch version, if you long-press FWD, it fastforward to the end and start from beginning and keep fast forwarding as a loop.

Proposed Scenario :
Long press FWD - It should fastforward action should stop at the end of the timeline. Just like Music app.

When long press REW, it rewinds all the way back to the beginning and stops there. This is correct, the same as Nusic app.

Thanks.

Mike

Comment 14

[vsireesha246]

Attachment: Bug_951025.zip

I modified the code as per the review comments and attached the png files in the attachment.

Comment 15

[Dominic Kuo [:dkuo]]

I thought we should have the wireframes and visual design first then implement this? and I agree with John we should treat bug 948260 and this bug as two separate features, or it will become a huge patch which is not easy to be uplifted.

Comment 16

[GoodMike]

Feedback after reviewing the patch again :
The long-press FWD issue has not be solved yet. It needs to align with Music app FWD long-press behavior.
Ni? Chris and Rob who are in charge of the spec for advice.
Please take over this issue since you have come back from vacation just now. 
Thank you.

Comment 17

[GoodMike]

Comment on attachment 8356997 [details]
Bug_951025.zip

Feedback after reviewing the patch again :
The long-press FWD issue has not be solved yet. It needs to align with Music app FWD long-press behavior.
Ni? Chris and Rob who are in charge of the spec for advice.
Please take over this issue since you have come back from vacation just now. 
Thank you.

Comment 18

[Dominic Kuo [:dkuo]]

Comment on attachment 8356997 [details]
Bug_951025.zip

As I commented in comment 15, let's wait for Chris or Rob to give us some inputs, and if [e-mail] wants to get the patch into the review process, it should go to ui-review first, not the regular review for devs, thanks.

Comment 19

[Hema Koka [:hema]]

vsireesha246: this feature is currently in the plan for our upcoming release (1.4). mozilla ui folks are working on coming up with the right ux proposal. once we have the recommendation from ux you can certainly help with the implementation (since you have already started on it). 

thanks
hema

Comment 20

[Rob MacDonald [:robmac]]

Hi everyone...

Jacqueline and I are working on a proposal and will post the results to this bug on Thursday. 

Thanks!

Comment 21

[Jacqueline Savory [:jsavory] UX]

Attachment: Video_Jan2014.pdf

Hello,

I've attached the proposal for this feature, please let me know if anyone has any questions. Thanks!

Comment 22

[Sri Kasetti]

Clearing the NI

Comment 23

[Jacqueline Savory [:jsavory] UX]

NI for Peter to provide any visuals for this proposal.

Comment 24

[Blake Wu [:bwu][:blakewu]]

(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
> 
> Hello,
> 
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!

I have some opinions as below. 
1. 10 seconds jump may be too big. 
User can simply use seek to do that. Besides normally the duration of videos recorded by phone is not long. Probably within several ten seconds. In this cases, only several frames would be displayed. One way to have a smooth fast-forward or rewind is to play video key-frames only.   
2. Long press FWD/RWD
Maybe it is not necessary to be consistent with music app. As I found some video players, like Apple QuickTime that if you want to play it in FWD, just press once. And you can go back to normal speed by pressing play/pause button. Would it be much easier for user?

Comment 25

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

Hi all,

About swipe to navigate, it may not easy to implement. Because we only have one video decoder in our devices. We may need to use a thumbnail image for another video or use two thumbnail images for both video files. The video player in video app is different than gallery app. We use video element to load the video file directly instead of loading thumbnail image first. While swiping, gallery app uses two images to do the transition. But in video app, we may need to pause the video element and load another image for swiping. There may be two issues here:

1. which image do we use and the image quality
    The only thumbnail image we current have is the image for list view. That image is created at a fixed resolution, 210x120. The algorithm of thumbnail image is to fit the video file into a 210x120 rectangle. When the video's ratio is not the same as 210x120, it fills black at the other place. So, a portrait video will be scaled down to fit into 120px in its hight, and the scale ratio will be high. If we use a portrait video in landscape layout, the quality of this image may be bad. In gallery app, it respects video resolution ratio to create thumbnail image and use css to place the thumbnail image into the list view. If we use the same algorithm here, it may give us an acceptable image quality in fullscreen view and may have a manageable scaling ratio.

2. loading performance
    If we use one image and one video for swiping, we may need to handle the time of video loaded and first frame decoded while swiping quickly. To compare the loading time of image and video, I may prefer to use two images for swipe transition. After the transition is finished, we use video element to load the video file but not auto-play. The timing of video loading may be an issue here.. If we use two images for transition, the timing to load the video file may be critical when user swipe quickly. In gallery app, we load the video when user tap the play button. We may use the same policy to do so.



(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
> 
> Hello,
> 
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!

Comment 26

[vsireesha246]

Attachment: Pointer to Pull Request.html

Hi John,

Would you please help me to get the approval for resources and code review.
As of now i attached PR for work in progress patch.

Seeking having some issues like in Comment#24.

Thanks..
Sireesha

Comment 27

[vsireesha246]

This PR contains know issues like

1.FF/RWD buttons alignment
2.More options screen(Share,Delete,Info) don't include Icons
3.Seeking behaviour is too fast.


 (In reply to vsireesha246 from comment #26)
> Created attachment 8371446 [details]
> Pointer to Pull Request.html
> 
> Hi John,
> 
> Would you please help me to get the approval for resources and code review.
> As of now i attached PR for work in progress patch.
> 
> Seeking having some issues like in Comment#24.
> 
> Thanks..
> Sireesha

Comment 28

[vsireesha246]

Navigating between Previous and Next Video(Bug 948260)implementation needs the below changes.

1.Like Gallery app,Video app needs to maintain an global array and this array is initially filled when we enumerate video database, and has elements added and removed when we receive create and delete events from the media databases.

2.Like Gallery app,Video app UI need to maintain 3 <div> elements for prev,current and next Videos be to displayed.But current video app UI is using single <video> element.

3.Video app need to include transition related css similar to Gallery app.

Along with the below points mentioned by John,prev/next video navigation of UI spec is little complex and need more code changes in Video app.


(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #25)
> Hi all,
> 
> About swipe to navigate, it may not easy to implement. Because we only have
> one video decoder in our devices. We may need to use a thumbnail image for
> another video or use two thumbnail images for both video files. The video
> player in video app is different than gallery app. We use video element to
> load the video file directly instead of loading thumbnail image first. While
> swiping, gallery app uses two images to do the transition. But in video app,
> we may need to pause the video element and load another image for swiping.
> There may be two issues here:
> 
> 1. which image do we use and the image quality
>     The only thumbnail image we current have is the image for list view.
> That image is created at a fixed resolution, 210x120. The algorithm of
> thumbnail image is to fit the video file into a 210x120 rectangle. When the
> video's ratio is not the same as 210x120, it fills black at the other place.
> So, a portrait video will be scaled down to fit into 120px in its hight, and
> the scale ratio will be high. If we use a portrait video in landscape
> layout, the quality of this image may be bad. In gallery app, it respects
> video resolution ratio to create thumbnail image and use css to place the
> thumbnail image into the list view. If we use the same algorithm here, it
> may give us an acceptable image quality in fullscreen view and may have a
> manageable scaling ratio.
> 
> 2. loading performance
>     If we use one image and one video for swiping, we may need to handle the
> time of video loaded and first frame decoded while swiping quickly. To
> compare the loading time of image and video, I may prefer to use two images
> for swipe transition. After the transition is finished, we use video element
> to load the video file but not auto-play. The timing of video loading may be
> an issue here.. If we use two images for transition, the timing to load the
> video file may be critical when user swipe quickly. In gallery app, we load
> the video when user tap the play button. We may use the same policy to do so.
> 
> 
> 
> (In reply to jsavory from comment #21)
> > Created attachment 8368359 [details]
> > Video_Jan2014.pdf
> > 
> > Hello,
> > 
> > I've attached the proposal for this feature, please let me know if anyone
> > has any questions. Thanks!

Comment 29

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

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

This patch works. But I feel it need to be refactored to have a separate file to handle this feature. And there are some few css nits that I already marked in PR.

As you mentioned, the forward/rewind step seems incorrect when we long press it. Maybe, we need to change to use seek and seekdone event instead of setInterval.

Comment 30

[Blake Wu [:bwu][:blakewu]]

Moreover, the performance of each seek should not be consistent. This is because now seek is a precise seeking which means if the seeking time is not close to key-frame, it takes some time to decode from the nearest key-frame to the seeking time. This performance problem could be observable easily (ex. the playback speed of long-press FWD should be stable and smooth), unless bug 778077 is fixed or Gaia can find the time of key-frame and seek to that time.

Comment 31

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

Thanks Blake,

I can understand the performance issue. We should need info Jacqueline to give us her thought.

Hi Jacqueline,

Please find the comment 24 and comment 30 to understand the concern of blake. And I feel we should discuss the previous/next navigation at bug 948260, since Sireesha has some concerns about the implementation. I will also move my comment 25 to there, too.

Comment 32

[vsireesha246]

Hi John,

I squashed the review comments for js changes.
I will do css changes once i will get UX input.

So please review the js changes and let me know your comments.

Thanks...
Sireesha

Comment 33

[vsireesha246]

Hi Eric,

Would you please help me for Visual spec(resources) for this bug.
I have some review comments for this bug related to css at https://github.com/mozilla-b2g/gaia/pull/16035/files.


Thanks..
Sireesha

Comment 34

[Sri Kasetti]

Also adding Rob since Jacqueline is away this week

Comment 35

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

We had discussed at IRC and Github. Clear the NI.

(In reply to vsireesha246 from comment #32)
> Hi John,
> 
> I squashed the review comments for js changes.
> I will do css changes once i will get UX input.
> 
> So please review the js changes and let me know your comments.
> 
> Thanks...
> Sireesha

Comment 36

[vsireesha246]

Hi John,

Would you please review the css & js part for this bug.
I added some of your review changes and action menu with icon changes.

Thanks..
Sireesha.

Comment 37

[Eric Pang [:epang] UX]

clearing need info on my since Peter has already been flagged.

Comment 38

[Peter La]

Hi guys,

Sorry for the delay in chiming in here.  I am currently trying to find a designer to take this on, we will be addressing this shortly and posting specs asap.

Peter.

Comment 39

[Peter La]

Just a quick update - we have Peko working on this today, and Hung will refine and finish it off by EOD Wednesday, at which point we'll post the specs.

These specs shouldn't hold up development though - the functional things can still be coded, but once the specs are up, you'll know exactly where to place things, have final graphics assets, etc.

Peter.

Comment 40

[vsireesha246]

Hi Peter,

I am already working on it and uploaded the PR for the same.I got the Review comments related to UX alignment along with some JS changes.I done the JS Review changes and i am waiting for UX input from you and the squash review changes need to be reviewed by Johnhu.

Hi John,

Currently I am working on Unit testcases.Would you please review the JS changes,so that i can add the some more Unit testcase for this bug.




(In reply to Peter La from comment #39)
> Just a quick update - we have Peko working on this today, and Hung will
> refine and finish it off by EOD Wednesday, at which point we'll post the
> specs.
> 
> These specs shouldn't hold up development though - the functional things can
> still be coded, but once the specs are up, you'll know exactly where to
> place things, have final graphics assets, etc.
> 
> Peter.

Comment 41

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

Hi Sireesha,

I had read your code with the PR here. This version is better. But I found the following issues:

1. We may need to have the same feature at view.js (open activity).
2. Show/hide options menu should be moved to video.js
3. Since we already give seekForward and seekRewind to controller, we may add event listeners inside of controller.

BTW, I still feel we need to wait for Peter's input to have correct image and layout.

Comment 42

[vsireesha246]

Hi John,

Thank You for your review comments.
Sorry i missed to add in view.js.

I squash the below review changes.
Would you please review it once again.

Thanks..
Sireesha


(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #41)
> Hi Sireesha,
> 
> I had read your code with the PR here. This version is better. But I found
> the following issues:
> 
> 1. We may need to have the same feature at view.js (open activity).
> 2. Show/hide options menu should be moved to video.js
> 3. Since we already give seekForward and seekRewind to controller, we may
> add event listeners inside of controller.
> 
> BTW, I still feel we need to wait for Peter's input to have correct image
> and layout.

Comment 43

[vsireesha246]

Hi John,

I added 2 Unit testcases for OptionView(newly added screen).
Is it fine or i need to added any other Unit testcases.
I think for FF/Rwd instead of Unit testcases,Gaia-UI testcases(Marionette) we need to write.

Comment 44

[Peter La]

Attachment: VisualSpec+IconAssets.zip

Hi Sireesha,

Apologies for the delay.  Attached please find the visual specs and icon assets for the video controls @1, @1.5 and @2 times resolutions.

We will load the patch and check the implementation on Monday.

Peter

Comment 45

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

Hi Sireesha,

Please update the PR to have all information from Peter La. I had checked the PR and Spec carefully again. I found:

1. We only implement the press and hold case but not tap case. We need to add them back.
2. The implementation of press and hold may be not exactly the same as spec. We still need to improve that.
3. As to unit tests, you are correct, we need to add more cases for Forward/Rewind controller.

Comment 46

[vsireesha246]

Hi John,

As discussed in IRC i tried to use playBackrate and its not working for me.Same i updated in github.So i used 1000ms instead of 15ms.
Remaing review comments are modified and squashed.

If you are ok with the js part i will start doing the css part given by Peter La.

So,please review it.

Thanks...
Sireesha

Comment 47

[vsireesha246]

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

Hi Peter La,

Would you please review the UI-Changes and let me know your review comments.

Hi John,

Would you please let me know your review commets for the functionality and css.

Thanks..
Sireesha

Comment 48

[vsireesha246]

Hi,

I have one doubt in page 5 Fat Farward and Rewind.

In case of Hold: according to screenshot 3 and 4, if user hold for 3secs then the seeking should increase 3sec*10 times?

Would you please clarify Hold case...



(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
> 
> Hello,
> 
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!

Comment 49

[Peter La]

Hi Sireesha,

I just reviewed your patch and noticed there are some layout issues.  We also discussed within UX that we'll need a small IxD change.

Both Jacqueline and I will be posting updates to our specs tomorrow, and I will also post a document outlining the layout issues that I've seen.

Stay tuned.
Peter.

Comment 50

[Jacqueline Savory [:jsavory] UX]

Attachment: Video_Mar2014.pdf

Hi Sireesha,

I've attached an updated spec that has changed the layout issue mentioned by Peter and removed the previous/next case. 

Regarding your comment, the fast forward and rewind speed will be x10 the regular speed of play. Let me know if this is considered too fast or slow and we can discuss another rate.

Comment 51

[vsireesha246]

Hi Peter La,

I modified the UX according to Video_Mar2014.pdf.
Would you please review the UX changes once again and let me know your comments.

Thanks..
Sireesha


(In reply to Peter La from comment #49)
> Hi Sireesha,
> 
> I just reviewed your patch and noticed there are some layout issues.  We
> also discussed within UX that we'll need a small IxD change.
> 
> Both Jacqueline and I will be posting updates to our specs tomorrow, and I
> will also post a document outlining the layout issues that I've seen.
> 
> Stay tuned.
> Peter.

Comment 52

[vsireesha246]

Hi,

Case1:The normal playBackRate=1.You mean to say in Hold case we need to increase playBackRate=10?
Case2:But according to the UX spec given by you it says..user holds for 3secs,it forwarded to 3*10=30sec.

So i need to consider case1 or case2?
If case2 is correct,then what about 3sec holded duration by user..means in this 3sec holding time video should play or pause? and we need to forward the play at a time..means suppose user holds for 3sec then 3*10=30sec at a time we need to forward/rwd?


(In reply to jsavory from comment #50)
> Created attachment 8385749 [details]
> Video_Mar2014.pdf
> 
> Hi Sireesha,
> 
> I've attached an updated spec that has changed the layout issue mentioned by
> Peter and removed the previous/next case. 
> 
> Regarding your comment, the fast forward and rewind speed will be x10 the
> regular speed of play. Let me know if this is considered too fast or slow
> and we can discuss another rate.

Comment 53

[vsireesha246]

(In reply to vsireesha246 from comment #52)
> Hi,
> 
> Case1:The normal playBackRate=1.You mean to say in Hold case we need to
> increase playBackRate=10?
> Case2:But according to the UX spec given by you it says..user holds for
> 3secs,it forwarded to 3*10=30sec.
> 
> So i need to consider case1 or case2?
> If case2 is correct,then what about 3sec holded duration by user..means in
> this 3sec holding time video should play or pause? and we need to forward
> the play at a time..means suppose user holds for 3sec then 3*10=30sec at a
> time we need to forward/rwd?
> 
> 
> (In reply to jsavory from comment #50)
> > Created attachment 8385749 [details]
> > Video_Mar2014.pdf
> > 
> > Hi Sireesha,
> > 
> > I've attached an updated spec that has changed the layout issue mentioned by
> > Peter and removed the previous/next case. 
> > 
> > Regarding your comment, the fast forward and rewind speed will be x10 the
> > regular speed of play. Let me know if this is considered too fast or slow
> > and we can discuss another rate.

Comment 54

[Peter La]

Attachment: Updated Visual Spec (Mar 07)

Hi Sireesha,

I'm uploading a slightly modified Visual Spec to account for the change in the menu in the header, as well as fix a couple of small issues from before.

Please refer to this spec for your implementation.  In particular:

1) The header bar should be black with 85% opacity, rather than the current dark grey, fully opaque header that is in the implementation.

2) The bottom toolbar that houses the playback controls should be 45px high.  It is currently 40px in your patch that I reviewed.

3) The play controls in your patch are slightly off center (skewed to the right) and slightly low compared to the spec.  Please check this and align with the spec.

4) The play button has a lighter rectangle behind it.  This is not as spec'd.  Please remove this.

Comment 55

[Peter La]

Attachment: Outline of problems with latest patch (illustration)

Hi Sireesha,

Please refer to this image.  It outlines the problems I have found when reviewing your patch.  Please contact me if you need further clarification or assistance!

Thanks!
Peter

Comment 56

[Peter La]

Attachment: Outline of problems with latest patch (illustration) - Updated

Updated with a couple more observations.

Comment 57

[vsireesha246]

Attachment: Bug_951025_Screenshots.7z

Hi Peter La,

I updated the PR with your review comments.
But still alignment problem is present,may be due to play/pause images.
Would you please recheck and let me know your review comments(Bug_951025_Screenshots.7z).
I attached the screen shots with the review comments.

Thanks..
Sireesha

Comment 58

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

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

Sorry for late review. This version is far more better. I find some nits and questions in the PR. Please find them.

I clear the PR because the visual seems incorrect at the landscape mode. I will upload the image.

Thanks for this patch.

Comment 59

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

Attachment: control overlaps

The time slider and buttons are overlapping.

Comment 60

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

Attachment: tablet UI broken

This patch breaks the tablet UI. We had met similar issue here. The minimum requirement of this kind issue is that we still have a way to use it. For tablet, we may hide the forward and rewind feature but we still need to have the way to play it. With the current patch, we cannot tap the play button at all.

Comment 61

[vsireesha246]

Hi John,

I updated the PR with your review comments.
I have two small queries and same i mentioned in github.

So,review and let me know your review comments.

Thanks..
Sireesha



(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #59)
> Created attachment 8389591 [details]
> control overlaps
> 
> The time slider and buttons are overlapping.

Comment 62

[vsireesha246]

Hi John,

Its better to added followup bug for all the issues related to fix patch.
Please let me know if your fine adding new for for the tablet.

Thanks..
Sireesha

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #60)
> Created attachment 8389598 [details]
> tablet UI broken
> 
> This patch breaks the tablet UI. We had met similar issue here. The minimum
> requirement of this kind issue is that we still have a way to use it. For
> tablet, we may hide the forward and rewind feature but we still need to have
> the way to play it. With the current patch, we cannot tap the play button at
> all.

Comment 63

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

For tablet case, we should not generate any regression. That's our policy.

We should do minimum efforts to keep that working. So, I think the only requirement is to
1. make "play" button work as usual,
2. turn off forward and rewind feature in tablet, just hiding the buttons,
3. file a bug to turn on forward and rewind feature in tablet.

Comment 64

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

I still find some nits and put them at PR. Please update it.

For tablet, please find the comment 63.

Comment 65

[vsireesha246]

Hi John,

I updated the PR please check,i added hidden for FF/Rwd buttons for tablet.
But i am unable to verify it in simulator.
Simulator is showing upto "Add videos to get started screen".

If it possible to verify in simulator please let me know.

Thanks..
Sireesha

Comment 66

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

You may put ogv and webm files under your Movies folder under your account. So, the video app can list videos in its list view.

Comment 67

[vsireesha246]

Hi John,

For Tablet with this patch Play,FF/Rwd buttons are not working and positions are not proper.
As per comment#c63 to make play button to work properly i need to know the proper resolutions in simulator and my doubt is due to this patch(code changes) or due to resources this is it stopped working.

So please let me know how can i proceed to make it work for tablet and to fix this bug completely.

Thanks..
Sireesha

Comment 68

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

It dues to the html and css change.

Comment 69

[Jacqueline Savory [:jsavory] UX]

Hi Sireesha,

Sorry for the confusing example! From your comment 52, I believe case 1 is what I'm trying to convey with my spec document. Thanks!

Comment 70

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

Hi Jacqueline,

About comment 52, what's the difference between 1. we tap ff/rwd button once and 2. hold them for 1 sec? For both case, they all jump 10 seconds.

With the implementation of Sireesha, it is more easy to tap it multiple times to jump 10 * n seconds then hold them for n seconds to have the same result.

Comment 71

[vsireesha246]

Hi John,

I fixed the Tablet UI crash and now play,FF/Rwd buttons are working.
Play button alignment is still in Tablet also.

So,Please let me know your review comments for the PR.

Hi Peter La,

Can you please provide me the Ui-Review for Tablet also with this PR.

Thanks..
Sireesha

Comment 72

[Jacqueline Savory [:jsavory] UX]

Hi John,
The idea behind having the two different functions is if the user quickly wants to jump ahead a bit they can tap the fast forward button, but if they want to fast forward through a larger section they can hold down the button for an extended period of time. 

I don't necessarily think its easier to tap too many times to get through a larger section. However, through testing if there are recommendations in changing these rates, I am certainly open to hear them.

Comment 73

[Wayne Chang [:wchang]]

Removing dependency as Prev/Next track requirement was dropped.

Comment 74

[vsireesha246]

Attachment: Pointer to Pull Request.html

HI John,

I created new PR for this issue and please review it.

Thanks,
Sireesha

Comment 75

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

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

Sireesha,

Thanks for this patch. It is almost finished. I clear the review flag because:
1. According to UX spec, we should have option buttons shown at header bar when device is at landscape mode. IIRC, we have it at last patch but it is missing is this patch. It should be easy to pull that back to this patch.
2. Thanks for your unit tests. But we break one video unit test. We need to fix them.
3. This patch breaks fullscreen mode of tablet version. The play button cannot be used and please hide the ff and rwd button in tablet.
4. The line-feed(\n) is leaded by a carriage-return(\r) at forwardrewind_controller.js. We may need to use dos2unix to fix that.

Comment 76

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

Comment on attachment 8368359 [details]
Video_Jan2014.pdf

Sorry, I found I read wrong spec. The review result item 1 is invalid. It exists in old spec but not in new spec. So, I make the old spec obsoleted.

Comment 77

[vsireesha246]

Hi John,

All your review commented are squashed.
But for Tablet Full screen case,i cant check it in simulator because my fullscreen button is not working.
So i modified or removed the videoToolBar from hiding.
Can you please check now it is working fine in Tablet.

Thanks..
Sireesha

Comment 78

[Blake Wu [:bwu][:blakewu]]

Bug 778077 has been fixed, so using fastSeek API instead of using currentTime will have a better performance.

Comment 79

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

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

Sireesha,

I feel the patch is ready to master. But the tablet is still broken. I cannot touch any button anymore and no way to enter fullscreen mode. We need to fix it before landing to master. And we also need ui-review, of course.

Comment 80

[vsireesha246]

Hi John,

I corrected Tablet UI errors and now iam able to use the buttons in normal and fullscreen mode.
Would you please recheck and let me know in Tablet UI is fine or not.

Thanks,
Sireesha

Comment 81

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

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

There are few suggestions here:

1. Please remove fullscreen-tool-bar and its descendants, including code in html, css, and js.
2. Please use "dos2unix" tool to convert the "\r\n" to "\n" for new line.

Comment 82

[vsireesha246]

Hi John,

I squashed the review changes.
Please check and let me know your review comments.

Thanks..
Sireesha

Comment 83

[vsireesha246]

Hi John,

I corrected missing delete and share buttons in select view screen.
So,please check and let me know your comments.

Thanks..
Sireesha

Comment 84

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

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

Sireesha,

This is good patch. But the portrait mode of tablet version cannot be used because of play button cannot be tapped. The same css changes at the landscape part of "video_tablet.css" should also be played at portrait part. They are similar but not exactly the same.

Comment 85

[vsireesha246]

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

Hi John,

I did 2 changes in video_tablet.css for Portrait mode same as landscape.
So please check and let me the review comments.

Thanks..
Sireesha

Comment 86

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

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

Thanks for your patient for revising so many times. I think this version is better now.

Nice job!

Comment 87

[vsireesha246]

Thank You John for your quick reviews and suggestions.

Comment 88

[vsireesha246]

Hi Wyane,

Would you please help me to do the UI-Review for this issue from UX.

Thanks..
Sireesha

Comment 89

[Wayne Chang [:wchang]]

Rob, are you still looking after video?

Comment 90

[Rob MacDonald [:robmac]]

(In reply to Wayne Chang [:wchang] from comment #89)
> Rob, are you still looking after video?

Tiffanie is now on video, needinfo'd. Please flag her for any UI reviews. Thanks!

Comment 91

[Tiffanie Shakespeare [:tif]]

I'm having issues reviewing the patch on my device.  Please attach video/photos if you need me to ui-review.
Thanks!

Comment 92

[vsireesha246]

Attachment: Bug_951025_Screenshots.rar

Hi,

I attached the screenshots(Bug_951025_Screenshots.rar) for Ui-review.
Please let me know your review comments.

Thanks..
Sireesha

Comment 93

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar

Adding Amy for visual design feedback.

Comment 94

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar

Adding Amy for visual design feedback.

Comment 95

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar

In the action menu, there shouldn't be icons - this was a change we made to camera's preview and I want to be consistent. Amy/Hung can comment more if needed.

Also, I was finally able to flash the patch - is this the latest? I noticed a few issues with it.
- the progression of the circle on the playback control isn't as smooth as it is in music
- the rewind/fast forward buttons didn't seem to do anything (both for tap and press&hold)
- when tapping "more" options icon, followed by "delete":
* the main button should be red and say "Delete" (rather than blue ok)
* let's remove the title "Video" and replace message with "Delete video?" This will make it consistent with gallery and camera's preview delete confirmation.

Thanks!

Comment 96

[Hung Nguyen]

Attachment: Explanation_PlayIconUpdate.zip

Threw together an explanation of what is wrong with the current implementation in terms of visuals. 

The explanation includes:

1. Comparison of the current vs intended design.
2. Alignment guide. 
3. Redline of the UI. 

I've also updated the play icon so its center aligned. The assets are included in the zip file. 

Please let me know if you have any questions.

Comment 97

[Hung Nguyen]

Comment on attachment 8418086 [details]
Explanation_PlayIconUpdate.zip

Please have a look. 

Thanks!

Comment 98

[vsireesha246]

Attachment: Bug_951025_UIReview.rar

Hi,

I have some aligment problem with play and pause buttons with your new UI resource and values.
Please check the screenshots Bug_951025_UIReview.rar

Thanks..
Sireesha

Comment 99

[Hung Nguyen]

Attachment: 951025_pixelmovement.png

You're so close! Added a quick guide of how much more you have to move the elements. 

Please update and send over a screencap when ready. 

Thanks Sireesha

Comment 100

[Tiffanie Shakespeare [:tif]]

Also, can you please post an updated patch so I can check out the fixes for the interaction issues I mentioned in comment 95?

Thanks Sireesha! :)

Comment 101

[vsireesha246]

Hi,

I updated the PR with some changes.
Please check and let me know your review comments.

Thanks..
Sireesha

(In reply to Tiffanie Shakespeare from comment #100)
> Also, can you please post an updated patch so I can check out the fixes for
> the interaction issues I mentioned in comment 95?
> 
> Thanks Sireesha! :)

Comment 102

[vsireesha246]

Attachment: Bug_951025_May08.rar

Hi,

Would you please check this screenshots(Bug_951025_May08.rar).
This is my current status.

As your previous review review says "Move 8px left".Do you mean i need to maintain 7.1rem to 7.1-0.8=6.3 rem?

Thanks..
Sireesha

Comment 103

[Tiffanie Shakespeare [:tif]]

Hey Sireesha - I checked out your patch (branch - Bug_951025_Video_FF/Rwd) and my comments (below) seem to still apply. Also, can you double check the screenshots you posted today? The folder was empty for me. 

Thanks!!

(In reply to Tiffanie Shakespeare from comment #95)
> Comment on attachment 8417199 [details]
> Bug_951025_Screenshots.rar
> 
> In the action menu, there shouldn't be icons - this was a change we made to
> camera's preview and I want to be consistent. Amy/Hung can comment more if
> needed.
> 
> Also, I was finally able to flash the patch - is this the latest? I noticed
> a few issues with it.
> - the progression of the circle on the playback control isn't as smooth as
> it is in music
> - the rewind/fast forward buttons didn't seem to do anything (both for tap
> and press&hold)
> - when tapping "more" options icon, followed by "delete":
> * the main button should be red and say "Delete" (rather than blue ok)
> * let's remove the title "Video" and replace message with "Delete video?"
> This will make it consistent with gallery and camera's preview delete
> confirmation.
> 
> Thanks!

Comment 104

[vsireesha246]

Hi Tiffanie,

Please find the below comments.

1.In the action menu, there shouldn't be icons - this was a change we made to
> camera's preview and I want to be consistent. Amy/Hung can comment more if
> needed.
Ans:I modified same in PR today.

2.the progression of the circle on the playback control isn't as smooth as
it is in music
Ans:This is existing bug in video app.We can raise follow up bug for this.In this patch i didn't modify anything related to progression of the circle on the playback control.If it really my mistake i will correct it.

3.the rewind/fast forward buttons didn't seem to do anything (both for tap
and press&hold)
Ans:Today i rebased with latest code,Can you please check once again.Its working for me.

4.when tapping "more" options icon, followed by "delete":
* the main button should be red and say "Delete" (rather than blue ok)
* let's remove the title "Video" and replace message with "Delete video?"
This will make it consistent with gallery and camera's preview delete
confirmation.
Ans:Same here as this is existing bug in Video app,we can raise the followup bug for this.

So,please let me know your comments for this.

Comment 105

[vsireesha246]

Attachment: Bug_951025_May09.rar

Hi,

I attached the current status of of this PR screenshots.
Still i need to make some alignment proper.

Thanks..
Sireesha

Comment 106

[vsireesha246]

Hi Tiffanie & Hnguyen,

With the current UI spec i am facing the FF/Rwd and Play buttons Touch area is very less.
So i modified the UI similar to Music app(Increasing buttons size) and the Touch area on FF/Rwd and play buttons got increased.(Refer Video1.png & Video2.png attached)

We need to increase the button size for increasing the Touch or hit area on buttons.

So would you please let me know your opinion about this.
If you are fine with this,please let me know the modified alignment values for the both landscape and portrait modes.

Thanks..
Sireesha

Comment 107

[vsireesha246]

Attachment: Video.rar

Comment 108

[Tiffanie Shakespeare [:tif]]

Hi Sireesha!

Checked out the patch and overall seems good. I've made a note to file those other issues as bugs, thanks for clarifying. 

I was able to get the fast forward and rewind to work, though it seemed a little buggy. It wasn't always registering my tap or doing the action; I would say about 40% of the time.

For the control placement, I don't have a problem copying the layout of the music app - more space between controls definitely improves usability. I'll let Hung weigh in and work with you on alignment if he agrees.

Thanks! :)

Comment 109

[Tiffanie Shakespeare [:tif]]

One more question to...someone.

I noticed that the photo app also has videos within it (taken from the camera app). Do we have plans to copy this UI into that app? It would be weird if the photo and video app behaved differently.

Enjoy!

Comment 110

[vsireesha246]

Comment on attachment 8418747 [details]
951025_pixelmovement.png

As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=951025#c108 we may need to follow the Music app UI.

So clearing the review.

Hung would you please provide me the Visual similar to Music app in both portrait and landscape mode(Music app contains only Portrait mode).

Thanks..
Sireesha

Comment 111

[Tiffanie Shakespeare [:tif]]

Hey Hung! See my comment #108 and Sireesha's comment #110. Do you agree that we should make the alignment as the Music app? If so can you provide that info please? Thanks so much!

Comment 112

[Hung Nguyen]

Hey Guys 

I agree we should replicate the music controls for consistency. By the looks of it, this approach should be more scalable across a variety of screen widths.  

Sireesha - can you implement it on the 320x480 resolution and forward me a screenshot? Since its a copy and paste exercise, I believe you won't need any further specification but let me know otherwise. 

Thanks

Comment 113

[vsireesha246]

Attachment: Bug_951025_VideoUI_As_Music.rar

Hi Hung,

Please find the attached Bug_951025_VideoUI_As_Music.rar.
I modified the play,FF/rwd controls as per Music app and I didnt modified Timeslider progress bar.

Please let me know if any further modifications and landscape mode changes if required because Music app doesn't have landscape mode.

Comment 114

[Hung Nguyen]

Attachment: 951025_AdjustmentNotes.zip

Thanks for making the update Sireesha. 

We're almost there. Please take a look at my attachments for the finishing touches. 

Cheers

Comment 115

[vsireesha246]

Attachment: Bug_951025.rar

Hi Hung,

Please find the updated attached Screenshots(Bug_951025.rar).
I also updated PR with the latest changes.
Please review and let me know your comments.

Thanks..
Sireesha

Comment 116

[vsireesha246]

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

Hi John,

I modified some code (index.html,video.css and forwardrewind_controller.js) as per the UI-Review comments.

Would you please review it and let me know your review comments.

Thanks..
Sireesha

Comment 117

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

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

Hi Russ,

Since you had became a peer of video app, you may also want to know this bug. It's an important one, I think. I will do the review. Please review this patch, too.

Comment 118

[Tiffanie Shakespeare [:tif]]

Patch looks good - does anyone else notice that the controls fade away super fast? I compared this with mozilla master and it's the same. Am I crazy or did they previously stay until you tapped?

Comment 119

[Russ Nicoletti [:russn]]

I have updated the PR with some comments, however I haven't finished my review so I'm leaving r? for now.

One question I have is the behavior of the forward/rewind functionality when the player is paused. From testing and from looking at the code, I see the feature is coded to function only while the player is playing. My reading of the UX spec, and what I think it most intuitive, is that the forward/rewind functionality would be active while playing or paused; and either way, after forwarding or rewinding the player would retain its state -- if the player was playing before forwarding/rewinding, it would play after forwarding/rewinding; if the player was paused before forwarding/rewinding, it would be paused after forwarding/rewinding. 

NI for Jacqueline to comment on whether forward/rewind should work when the player is paused.

Comment 120

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

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

The patch looks good. But we found some nits here. I clear the review flag because it breaks the tablet's UI again.

Comment 121

[vsireesha246]

Hi Russ,

I updated the PR with your review comments.
I enabled FF/Rwd functionality even in pause state(Music app is doing the same).
So,please let me know your review comments if any.

Thanks..
Sireesha

(In reply to Russ Nicoletti [:russn] from comment #119)
> I have updated the PR with some comments, however I haven't finished my
> review so I'm leaving r? for now.
> 
> One question I have is the behavior of the forward/rewind functionality when
> the player is paused. From testing and from looking at the code, I see the
> feature is coded to function only while the player is playing. My reading of
> the UX spec, and what I think it most intuitive, is that the forward/rewind
> functionality would be active while playing or paused; and either way, after
> forwarding or rewinding the player would retain its state -- if the player
> was playing before forwarding/rewinding, it would play after
> forwarding/rewinding; if the player was paused before forwarding/rewinding,
> it would be paused after forwarding/rewinding. 
> 
> NI for Jacqueline to comment on whether forward/rewind should work when the
> player is paused.

Comment 122

[Hung Nguyen]

Comment on attachment 8424610 [details]
Bug_951025.rar

From a visual perspective, we're good to go. 

Thanks for all your hard work Sireesha.

Comment 123

[vsireesha246]

Hi John,

I updated the Tablet UI crash fix and some of your review comments.
I have some queries related to unit test and same i asked in PR.

So,please let me know your comments for this.

Thanks..
Sireesha

Comment 124

[Jacqueline Savory [:jsavory] UX]

Hi Russ, thanks for catching this. 
Yes, I agree that the fast forward and rewind buttons should still work during the paused state.

Comment 125

[Tiffanie Shakespeare [:tif]]

Hey guys - I just installed the updated code and it looks like the forward/rewind buttons have stopped working. I can't get them to work at all during playing and paused.

Comment 126

[Russ Nicoletti [:russn]]

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

Sireesha, I added a few more comments/questions to the PR. Although r- for now, I anticipate approval after the next iteration. Good work.

One other comment, I see there are few new images that aren't used ('actionicon_media_*.png') -- it seems these are obsolete in the presence of IconAction_Media_*.png images. If so, can you delete the obsolete image files? Thanks.

Comment 127

[vsireesha246]

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

Hi Russ,

I updated all your review changes and for one review i added my comment in PR.
So please review and let me know your comments.

Thanks..
Sireesha

Comment 128

[Tiffanie Shakespeare [:tif]]

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

Patch looks good from the interaction side. It also looks like Hung approved the visuals in comment #122.

Thanks Sireesha

Comment 129

[Russ Nicoletti [:russn]]

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

Looks good, r+. There are a few nits I added to the PR about missing spaces and the name of the new unit test js file. If you can fix those that would be good.

One last thing, I noticed the travis build failed due to a linter error in forward_rewind_controller.js (you need to declare the export of 'ForwardRewindController') -- I also mentioned another linter issue in that file in the PR. 

This is a nice addition to the video app.

Comment 130

[vsireesha246]

Hi Russ,

I fixed the nits related to space and renamed the file and added export for linter error.
For remaining this i added comments in PR.

Thanks for your quick review.

(In reply to Russ Nicoletti [:russn] from comment #129)
> Comment on attachment 8406086 [details]
> Pointer to Pull Request.html
> 
> Looks good, r+. There are a few nits I added to the PR about missing spaces
> and the name of the new unit test js file. If you can fix those that would
> be good.
> 
> One last thing, I noticed the travis build failed due to a linter error in
> forward_rewind_controller.js (you need to declare the export of
> 'ForwardRewindController') -- I also mentioned another linter issue in that
> file in the PR. 
> 
> This is a nice addition to the video app.

Comment 131

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

Sireesha,

It works fine but has a wired animation. That doesn't affect the usage of tablet. Thanks for taking care of this.

Comment 132

[vsireesha246]

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

Hi Tiffanie,

Would you please review the UI for this PR.(Normal Video playback,Pick activity(from SMS) and through Bluetooth transfer etc)

Thanks..
Sireesha

Comment 133

[Tiffanie Shakespeare [:tif]]

Hey Sireesha - patch looks good. Nice work!

I did run across a bug where the image would freeze during playback but you could hear the audio continuing in the background.

The STR are pretty much tapping the play, forward/back buttons and throwing a pause tap and doing the same. I didn't notice a specific sequence, but I am able to repro pretty easily. When this happens I have to force quit the video app to be able to use it again.

Comment 134

[Tiffanie Shakespeare [:tif]]

Hey John - I don't have a tablet. What sort of weird animation is happening? Even though the UI is still usable, we should still ensure the animations are correct and working.

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #131)
> Sireesha,
> 
> It works fine but has a wired animation. That doesn't affect the usage of
> tablet. Thanks for taking care of this.

Comment 135

[Russ Nicoletti [:russn]]

(In reply to vsireesha246 from comment #130)
> Hi Russ,
> 
> I fixed the nits related to space and renamed the file and added export for
> linter error.
> For remaining this i added comments in PR.
> 
> Thanks for your quick review.
> 
> (In reply to Russ Nicoletti [:russn] from comment #129)
> > Comment on attachment 8406086 [details]
> > Pointer to Pull Request.html
> > 
> > Looks good, r+. There are a few nits I added to the PR about missing spaces
> > and the name of the new unit test js file. If you can fix those that would
> > be good.
> > 
> > One last thing, I noticed the travis build failed due to a linter error in
> > forward_rewind_controller.js (you need to declare the export of
> > 'ForwardRewindController') -- I also mentioned another linter issue in that
> > file in the PR. 
> > 
> > This is a nice addition to the video app.

Hi Sireesha, thanks for your quick updates per the review comments.

I updated the PR regarding the ForwardRewindController 'init' function in the unit test context. I also realized the unit tests could fairly easily provide more test coverage which I also mentioned in the PR. Thanks again for the updates.

Comment 136

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

Attachment: tablet-wired-animation.mp4

(In reply to Tiffanie Shakespeare from comment #134)
> Hey John - I don't have a tablet. What sort of weird animation is happening?
> Even though the UI is still usable, we should still ensure the animations
> are correct and working.

Hi Tiffanie,

Thanks for asking this. Please find the attachment which contains the animation. A flying animation happens to the "play" or "pause" button when I press it. So, please focus your eye at the bottom of the video. It may be caused by few misconfigured CSS properties.

Comment 137

[vsireesha246]

Hi Tiffanie,

I am unable to reproduce this issue.
Would you please let me know the Gaia and Gecko version.

Thanks..
Sireesha


(In reply to Tiffanie Shakespeare from comment #133)
> Hey Sireesha - patch looks good. Nice work!
> 
> I did run across a bug where the image would freeze during playback but you
> could hear the audio continuing in the background.
> 
> The STR are pretty much tapping the play, forward/back buttons and throwing
> a pause tap and doing the same. I didn't notice a specific sequence, but I
> am able to repro pretty easily. When this happens I have to force quit the
> video app to be able to use it again.

Comment 138

[Tiffanie Shakespeare [:tif]]

Hey Sireesha - must have been something wrong with the Build I had installed yesterday and I have since updated it. Believe me I tried, but I cant repro the issue. Easy fix :)

Comment 139

[vsireesha246]

Hi Russ,

I modified the Unit test cases.
Would you please review and let me know the comments.

Thanks..
Sireesha

Comment 140

[vsireesha246]

Hi John,

This i can't able to reproduce in simulator.
Can you please conform..is it reproduced in simulator?

Thanks..
Sireesha

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> Created attachment 8427461 [details]
> tablet-wired-animation.mp4
> 
> (In reply to Tiffanie Shakespeare from comment #134)
> > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > Even though the UI is still usable, we should still ensure the animations
> > are correct and working.
> 
> Hi Tiffanie,
> 
> Thanks for asking this. Please find the attachment which contains the
> animation. A flying animation happens to the "play" or "pause" button when I
> press it. So, please focus your eye at the bottom of the video. It may be
> caused by few misconfigured CSS properties.

Comment 141

[Tiffanie Shakespeare [:tif]]

Yes! I do see that. 

Sireesha - can you double check the CSS and see if we can resolve this issue? I assume that why you're asking John about trying to repro the issue in the simulator.

Thanks!

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> Created attachment 8427461 [details]
> tablet-wired-animation.mp4
> 
> (In reply to Tiffanie Shakespeare from comment #134)
> > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > Even though the UI is still usable, we should still ensure the animations
> > are correct and working.
> 
> Hi Tiffanie,
> 
> Thanks for asking this. Please find the attachment which contains the
> animation. A flying animation happens to the "play" or "pause" button when I
> press it. So, please focus your eye at the bottom of the video. It may be
> caused by few misconfigured CSS properties.

Comment 142

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

Sorry for late replying.

The simulator doesn't have the same issue with the latest version. But it still has the same issue in tablet.

Comment 143

[vsireesha246]

Hi Tiffanie,

I don't have Tablet with me and i need to fix this issue with simulator only.
But i checked this issue, is not reproduced in simulator.Please check the John comment 142 for the same.


Thanks..
Sireehsa

(In reply to Tiffanie Shakespeare from comment #141)
> Yes! I do see that. 
> 
> Sireesha - can you double check the CSS and see if we can resolve this
> issue? I assume that why you're asking John about trying to repro the issue
> in the simulator.
> 
> Thanks!
> 
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> > Created attachment 8427461 [details]
> > tablet-wired-animation.mp4
> > 
> > (In reply to Tiffanie Shakespeare from comment #134)
> > > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > > Even though the UI is still usable, we should still ensure the animations
> > > are correct and working.
> > 
> > Hi Tiffanie,
> > 
> > Thanks for asking this. Please find the attachment which contains the
> > animation. A flying animation happens to the "play" or "pause" button when I
> > press it. So, please focus your eye at the bottom of the video. It may be
> > caused by few misconfigured CSS properties.

Comment 144

[vsireesha246]

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

Hi John,

I fixed the "Blue color Highlighting issue on the Play,FF/Rwd buttons" if the user swipes or clicks the button edges.

So would you please review the changes and also let me know the Tablet issue is still exists or not.

Thanks..
Sireeha

Comment 145

[Tiffanie Shakespeare [:tif]]

Is there perhaps someone with a tablet we can ping to ask for help? I totally get how impossible it would be to try to fix something without being able to reproduce.

Comment 146

[Russ Nicoletti [:russn]]

Hi Sireeha, I added some comments to the PR regarding the unit tests (one minor issue).

Comment 147

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

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

I had tested on phone and tablet. I think the everything is fine. Don't forget the check russ's comments and my comment about tablet animation issue.

Thanks for this patch. Nice job!!

Comment 148

[vsireesha246]

Hi John,

I updated the PR.Can you please check the Tablet issue is solved and i also updated Russ comments.

Thanks..
Sireesha

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] OOO from 6/2~6/6 from comment #147)
> Comment on attachment 8406086 [details]
> Pointer to Pull Request.html
> 
> I had tested on phone and tablet. I think the everything is fine. Don't
> forget the check russ's comments and my comment about tablet animation issue.
> 
> Thanks for this patch. Nice job!!

Comment 149

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

Cool. This version fixes the issue. That's really caused by transition css.

Comment 150

[vsireesha246]

Hi Russ,

I updated your review comments.Please check.

Thanks..
Sireesha

(In reply to Russ Nicoletti [:russn] from comment #146)
> Hi Sireeha, I added some comments to the PR regarding the unit tests (one
> minor issue).

Comment 151

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

Comment on attachment 8427461 [details]
tablet-wired-animation.mp4

Obsolete this attachment since it is not an issue anymore.

Comment 152

[vsireesha246]

Hi Russ,

We can land this PR to master,if your review is done?
From my side if anything is pending please let me know.

Thanks..
Sireesha

Comment 153

[Russ Nicoletti [:russn]]

It looks good to land. The travis failures appear to be unrelated to your changes. Although I'm finding the patch doesn't apply cleanly in master. You may need to correct that before landing.

Comment 154

[vsireesha246]

Hi Russ,

I am able to apply the patch cleanly on to the Master.
Would you please check it once and please land it on to the master,because i don't have permissions to merge.

Thanks..
Sireesha

(In reply to Russ Nicoletti [:russn] from comment #153)
> It looks good to land. The travis failures appear to be unrelated to your
> changes. Although I'm finding the patch doesn't apply cleanly in master. You
> may need to correct that before landing.

Comment 155

[Russ Nicoletti [:russn]]

I verified the patch applies cleanly against latest master. Landed just now.

Comment 156

[Russ Nicoletti [:russn]]

Master --> https://github.com/mozilla-b2g/gaia/commit/8341cddb04773957069ac1334692a888b8bb1091

Comment 157

[Hema Koka [:hema]]

Added 2.0 feature flag since it made it into 2.0 before branch cut