Closed
Bug 947103
Opened 11 years ago
Closed 11 years ago
[Video] Update to refreshed video player
Categories
(Firefox OS Graveyard :: Gaia::Video, defect)
Tracking
(b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)
RESOLVED
FIXED
1.3 C3/1.4 S3(31jan)
People
(Reporter: epang, Assigned: pivanov)
References
Details
(Whiteboard: ux-tracking, visual design, visual-tracking, bokken)
Attachments
(5 files, 3 obsolete files)
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•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8343605 -
Flags: review?(dale)
Comment 4•11 years ago
|
||
Comment on attachment 8343605 [details]
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•11 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)
Comment 6•11 years ago
|
||
Thanks Eric,
I will apply both patches before reviewing.
Flags: needinfo?(johu)
Comment 7•11 years ago
|
||
Comment on attachment 8343605 [details]
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•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #7)
> Comment on attachment 8343605 [details]
> 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•11 years ago
|
Whiteboard: ux-tracking, visual design, visual-tracking, jian → ux-tracking, visual design, visual-tracking, bokken
Reporter | ||
Comment 9•11 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!
Assignee | ||
Comment 10•11 years ago
|
||
Actually ... it's not easy ... but I will try to do it
Flags: needinfo?(pivanov)
Comment 11•11 years ago
|
||
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)
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
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)
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
I've checked the screen on device offline, it's correct! The progress bar should overlap the video.
Flags: needinfo?(hhuang)
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8360194 [details]
update the patch of Pavel to have tablet compatible
Looks OK :) thanks
Attachment #8360194 -
Flags: review?(pivanov) → review+
Comment 22•11 years ago
|
||
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•11 years ago
|
||
merged to master:
https://github.com/mozilla-b2g/gaia/commit/2a698b3ee70d1811822d940c1b740b5adebea1ff
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 24•11 years ago
|
||
Comment on attachment 8360194 [details]
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)
Comment 25•11 years ago
|
||
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]:
Updated•11 years ago
|
Attachment #8360194 -
Flags: approval-gaia-v1.3?(praghunath) → approval-gaia-v1.3+
Comment 26•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(pivanov) → needinfo?(johu)
Comment 27•11 years ago
|
||
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)
Assignee | ||
Comment 28•11 years ago
|
||
Comment on attachment 8377366 [details]
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-
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 8377366 [details]
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-
Comment 30•11 years ago
|
||
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)
Comment 31•11 years ago
|
||
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•11 years ago
|
||
When this is ready to merge to v1.3, just add the checkin-needed keyword to the bug :)
Comment 33•11 years ago
|
||
Comment on attachment 8377366 [details]
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 34•11 years ago
|
||
Comment on attachment 8377429 [details]
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)
Comment 35•11 years ago
|
||
clear the need info because I already created two versions.
Flags: needinfo?(pivanov)
Assignee | ||
Comment 37•11 years ago
|
||
Comment on attachment 8377366 [details]
patch for v1.3
I prefer the css version. Thanks :)
Attachment #8377366 -
Flags: review?(pivanov) → review+
Flags: needinfo?(pivanov)
Comment 38•11 years ago
|
||
Comment on attachment 8377429 [details]
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)
Comment 39•11 years ago
|
||
Ryan,
Thanks for your ping. Please help to land the patch at attachment 8377366 [details].
Keywords: checkin-needed
Comment 40•11 years ago
|
||
v1.3: 8039a5cb7519adfa81677df577f494c6a4de6140
Keywords: checkin-needed
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•