[Video] Update to refreshed video player

RESOLVED FIXED in Firefox OS v1.3

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: epang, Assigned: pivanov)

Tracking

unspecified
1.3 C3/1.4 S3(31jan)
x86
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: ux-tracking, visual design, visual-tracking, bokken)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 8343589 [details]
Video Tool bar specs.png
Created attachment 8343605 [details] [review]
patch for Gaia/master
Depends on: 947093
Attachment #8343605 - Flags: review?(dale)
(Reporter)

Updated

5 years ago
Duplicate of this bug: 939138
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)
Attachment #8343605 - Flags: review?(dale) → review?(johu)
(Reporter)

Comment 5

5 years ago
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!
Flags: needinfo?(johu)
Thanks Eric,

I will apply both patches before reviewing.
Flags: needinfo?(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.
Attachment #8343605 - Flags: review?(johu) → review-
(Reporter)

Comment 8

5 years ago
(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!
Flags: needinfo?(pivanov)
(Reporter)

Updated

5 years ago
Whiteboard: ux-tracking, visual design, visual-tracking, jian → ux-tracking, visual design, visual-tracking, bokken
(Reporter)

Comment 9

5 years ago
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!
Actually ... it's not easy ... but I will try to do it
Flags: needinfo?(pivanov)
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.
Flags: needinfo?(pivanov)
Created attachment 8358200 [details]
screenshots of tablet after this patch applied.
Since the discussion within bug 952446 comment 4, I will update the patch to fix those tablet related concerns at comment 11.
Flags: needinfo?(pivanov)
Created attachment 8360194 [details] [review]
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.
Attachment #8343605 - Attachment is obsolete: true
Attachment #8358200 - Attachment is obsolete: true
Attachment #8360194 - Flags: review?(pivanov)
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.
Created attachment 8360195 [details]
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.
Flags: needinfo?(hhuang)
Flags: needinfo?(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.
Flags: needinfo?(frlee)
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.
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.
I've checked the screen on device offline, it's correct! The progress bar should overlap the video.
Flags: needinfo?(hhuang)
Comment on attachment 8360194 [details] [review]
update the patch of Pavel to have tablet compatible

Looks OK :) thanks
Attachment #8360194 - Flags: review?(pivanov) → review+
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.
merged to master:
https://github.com/mozilla-b2g/gaia/commit/2a698b3ee70d1811822d940c1b740b5adebea1ff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 24

5 years ago
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.
Attachment #8360194 - Flags: approval-gaia-v1.3?(praghunath)
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]:
Attachment #8360194 - Flags: approval-gaia-v1.3?(praghunath) → approval-gaia-v1.3+
This doesn't cherry-pick cleanly to v1.3. Please post a branch-specific PR for merging.
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → fixed
Flags: needinfo?(pivanov)
Keywords: branch-patch-needed
Flags: needinfo?(pivanov) → needinfo?(johu)
Created attachment 8377366 [details] [review]
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.
Attachment #8377366 - Flags: review?(pivanov)
Flags: needinfo?(johu)
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
Attachment #8377366 - Flags: review?(pivanov) → review-
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
Attachment #8377366 - Flags: review-
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?
Flags: needinfo?(pivanov)
Created attachment 8377429 [details] [review]
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.
When this is ready to merge to v1.3, just add the checkin-needed keyword to the bug :)
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.
Attachment #8377366 - Flags: review?(pivanov)
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.
Attachment #8377429 - Flags: review?(pivanov)
clear the need info because I already created two versions.
Flags: needinfo?(pivanov)
Review ping?
Flags: needinfo?(pivanov)
Keywords: branch-patch-needed
Comment on attachment 8377366 [details] [review]
patch for v1.3

I prefer the css version. Thanks :)
Attachment #8377366 - Flags: review?(pivanov) → review+
Flags: needinfo?(pivanov)
Comment on attachment 8377429 [details] [review]
PR for v1.3 with play button change

obsolete this version since it is not accepted.
Attachment #8377429 - Attachment is obsolete: true
Attachment #8377429 - Flags: review?(pivanov)
Ryan,

Thanks for your ping. Please help to land the patch at attachment 8377366 [details] [review].
Keywords: checkin-needed
v1.3: 8039a5cb7519adfa81677df577f494c6a4de6140
status-b2g-v1.3: affected → fixed
Keywords: checkin-needed
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
status-b2g-v1.3T: --- → fixed
You need to log in before you can comment on or make changes to this bug.