947103 - [Video] Update to refreshed video player

Status: RESOLVED FIXED

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

Description

[Eric Pang [:epang] UX]

Attachment: Video_Specs.png

This bug includes 2 updates:
1. New updated visuals for the video player
2. Update to the tool bar (note highlights are not needed in the actual video player screen do to the fade out - it ends up looking broken)

Comment 1

[Eric Pang [:epang] UX]

Attachment: Video Tool bar specs.png

Comment 2

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8343605 [details] [review]
patch for Gaia/master

Redirecting to John as I dont currently work on Video (also video is crashing on the peak right now which is the only device I have to test)

Comment 5

[Eric Pang [:epang] UX]

Thanks for redirecting Dale!  

Hey John, I just wanted to note that the patch from this bug needs is dependent with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=947093

They will land together along with all other toolbar updates to remain consistent. Thanks!

Comment 6

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

Thanks Eric,

I will apply both patches before reviewing.

Comment 7

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

Comment on attachment 8343605 [details] [review]
patch for Gaia/master

Most of codes looks good in this patch. But I give this patch r- because it doesn't take 1.5x devices, like helix, into account. It removes the all background-size of icons. That may make image overflow in 1.5x devices, like helix. We need to put them back and use correct background-size settings if the size of image is changed. The second thing is that this patch moves some elements to another place in view.html but not in video.html. We should keep them sync.

BTW, I cannot test it because of conflict while applying this patch with the latest master. Please rebase to master.

Comment 8

[Eric Pang [:epang] UX]

(In reply to John Hu [:johnhu] from comment #7)
> Comment on attachment 8343605 [details] [review]
> patch for Gaia/master
> 
> Most of codes looks good in this patch. But I give this patch r- because it
> doesn't take 1.5x devices, like helix, into account. It removes the all
> background-size of icons. That may make image overflow in 1.5x devices, like
> helix. We need to put them back and use correct background-size settings if
> the size of image is changed. The second thing is that this patch moves some
> elements to another place in view.html but not in video.html. We should keep
> them sync.
> 
> BTW, I cannot test it because of conflict while applying this patch with the
> latest master. Please rebase to master.

Thanks John, Pavel can you look into John's comments?  Let me know if there's anything I can help with.  If not, when ready please reflag John.  Thanks!

Comment 9

[Eric Pang [:epang] UX]

Hi Pavel,

Since we are planning to land the updated tool bars for video later on can you cherry pick the changes for the refreshed video player and land this first?  Let me know, thanks!

Comment 10

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Actually ... it's not easy ... but I will try to do it

Comment 11

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

Pavel,

Sorry for late review. I didn't notice the update of PR until Eric's email sent today.

Two main concerns:

1. This patch had moved the elapsed-text and duration-text element out of timeSlider in "view.html" but remains the origin structure in "index.html". Although the css works, we may want to see that they are synced. Is there any reason to have this kind of change??

2. This patch breaks the tablet layout. I will upload the screenshot to this bug and cc other guys here to know about this.

Comment 12

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

Attachment: screenshots of tablet after this patch applied.

Comment 13

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

Since the discussion within bug 952446 comment 4, I will update the patch to fix those tablet related concerns at comment 11.

Comment 14

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

Attachment: update the patch of Pavel to have tablet compatible

Pavel,

I had update your patch to have tablet compatible. According to the priority of tablet,  this patch is trying to use minimum resource to make the tablet version workable but not to make the tablet version to be original version or to have the consistent visual with phone version.

I will upload few screenshots to show the change of tablet to Taipei visual and TAM.

Comment 15

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

Pavel,

One more thing: I don't change anything in video.css. So, it should be the same as your previous version. Please also help me to check that.

Comment 16

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

Attachment: tablet visual change list

Hi Helen and Francis

The followings are the changes of tablet visual. Hope that's ok to you.

1. the touched style of player head of progress bar had changed to be the phone's style
2. the touched style of play button and fullscreen button had been removed.

Comment 17

[Francis Lee [:frlee]]

hi John,

i have no concern with the style, but if you refer to the first 2 screen shots of your attachment, is it normal that the progress bar overlap the playing video? it doesn't seem desirable. i will let Helen to comment on this.

Comment 18

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

The second one is normal because it is in fullscreen view and the progress bar can be hide by tap the screen. The first one is incorrect. I will update my patch the have it shown as expected.

Comment 19

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

Sorry, I think I am wrong about the first screenshot. According to the visual spec of tablet, it is normal. And the progress bar is transparent so that user may see through to it. The update of patch is no need.

Comment 20

[Helen Huang [:HHuang]]

I've checked the screen on device offline, it's correct! The progress bar should overlap the video.

Comment 21

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8360194 [details] [review]
update the patch of Pavel to have tablet compatible

Looks OK :) thanks

Comment 22

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

Oh, I find that we need to remove useless image files. I will update the patch and retest that.

Another thing we lacked is that we need to use ./tools/png_recompress.sh to compress png files. I learned that from bug 869289.

Comment 23

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

merged to master:
https://github.com/mozilla-b2g/gaia/commit/2a698b3ee70d1811822d940c1b740b5adebea1ff

Comment 24

[Stephany Wilkes]

Comment on attachment 8360194 [details] [review]
update the patch of Pavel to have tablet compatible

[Approval Request Comment]
Noting for 1.3 uplift as this is the last week for 1.3 approvals.

Comment 25

[Preeti Raghunath(:Preeti)]

Unable to review because of lack of form:

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: high, incorrect visuals
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): no risk
[String changes made]:

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This doesn't cherry-pick cleanly to v1.3. Please post a branch-specific PR for merging.

Comment 27

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

Attachment: patch for v1.3

Hi Pavel,

This patch is almost exactly the same as your previous version but including few file deletions. For landing to 1.3, please review help to review this patch. Thanks.

Comment 28

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8377366 [details] [review]
patch for v1.3

I put some comments on the PR ... there are some problems with the play button active state and the #player:before selector too. Can you check and ping me back

Comment 29

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8377366 [details] [review]
patch for v1.3

I put some comments on the PR ... there are some problems with the play button active state and the #player:before selector too. Can you check and ping me back

Comment 30

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

Pavel,

Thanks for your reviewing. The #player:before is a typo. It should be #play:before. I had changed the PR to that.

As to the active state of play button, we use touch-start event to change the state of play button. So, the active state is hard to find in v1.3. But in master, we had changed the code to use click event. I can do similar thing here. But that may need to change the js code. How's your thought about that?

Comment 31

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

Attachment: PR for v1.3 with play button change

Hi Pavel,

Please also try this version. I had do the minimum code change to separate the event handler of play button from the bundle of video controls. That gives us the active state of play button.

In v1.4, we have a tablet patch which changes the event handlers of all video controls. That's the reason we don't get this at v1.4.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

When this is ready to merge to v1.3, just add the checkin-needed keyword to the bug :)

Comment 33

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

Comment on attachment 8377366 [details] [review]
patch for v1.3

I had put two version here. Please review them.

This is the original version only with CSS changes. In this version, the active state of play button is hard to find.

Comment 34

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

Comment on attachment 8377429 [details] [review]
PR for v1.3 with play button change

I had put two version here. Please review them.

This version had changed few lines of JS and the CSS changes. The active state of play button is easy to find.

Thanks for your reviewing.

Comment 35

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

clear the need info because I already created two versions.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Review ping?

Comment 37

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8377366 [details] [review]
patch for v1.3

I prefer the css version. Thanks :)

Comment 38

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

Comment on attachment 8377429 [details] [review]
PR for v1.3 with play button change

obsolete this version since it is not accepted.

Comment 39

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

Ryan,

Thanks for your ping. Please help to land the patch at attachment 8377366 [details] [review].

Comment 40

[Ryan VanderMeulen [:RyanVM]]

v1.3: 8039a5cb7519adfa81677df577f494c6a4de6140