1016255 - Video played from Gallery goes back to gallery view on Pressing left top corner,no back button visible though

Status: RESOLVED FIXED

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

Description

[amitav.anand]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193838

Steps to reproduce:

1. Go to Gallery
2. Start Playing a video
3. Touch on the top left corner of the video


Actual results:

No action should take place. No back button visible.


Expected results:

The video stops and goes back to gallery view.

Comment 1

[Tiffanie Shakespeare [:tif]]

Attachment: video in gallery screenshots

I think you mixed up your expected and actual result comments?

For the expected (what you have as actual), it should actually be the opposite. The issue isn't that the action takes place, it's that the back button isn't visible.

Take a look at the attached screenshots. 
The first shows the video before playback starts - back button is visible.
The second shows the video playing - the playback controls appear at the bottom, but the back button has disappeared.
The third shows all controls hidden after tapping on the screen. Tapping again would bring them back.

What should happen is the back button stays on screen until the user taps to hide all controls - i.e. follow suit with the playback controls.

I did notice that the behaviour also occurs when all the controls are hidden - tapping the upper left returns to the main list. In this instance, tapping there would not return to the previous screen but instead bring back the hidden controls.

Hopefully this makes sense, thanks!

Comment 2

[Stephany Wilkes]

Clearing my flag as Tif more than covered this.

Comment 3

[amitav.anand]

Thanks Tiffanie and Stephany for your comments.

Yes :-) By mistake I had mixed up the actual and expected results.

I saw the attached screen shot. I made a temporary fix by making the back button visible while the video is playing.
Correct me if I am wrong, the expected result should be: the back button + the video control ie the Play/pause button,seek bar should remain on the screen until the user taps on the playback screen ?

I am working on this issue. Will submit the pull request as soon as I am done with this.

Comment 4

[Tiffanie Shakespeare [:tif]]

Yep that's right - the back button and video control are both visible until the user taps and both are hidden. Tapping again brings them both back.

Please UI-reivew? me when you have a patch I should take a look at. Thanks for working on this!

Comment 5

[amitav.anand]

Attachment: PR

On Singletap the back button and video control appears on the playback screen, hiding the play/pause and seek bar. Please review the fix.

Comment 6

[Tiffanie Shakespeare [:tif]]

Attachment: galleryBug1016255.pdf

Hi there! I think there's been a little bit of a miscommunication. The back button in the upper left shouldn't be tied to the toolbar as I see in your patch. 

Rather than risk trying to "verbally" describe again, please check out the UI flow I've attached.

Thanks so much and sorry for not being more clear previously.

Comment 7

[Tiffanie Shakespeare [:tif]]

Let me know if you have any further questions!

Comment 8

[amitav.anand]

Hi Punam,

I have added a new tiny back button in video_player.js where the whole video player UI is created on the go. I am attaching the files here. The fix works fine.

Kindly let me know your views on this.

Comment 9

[amitav.anand]

Attachment: Bug_1016255.patch

Kindly review the patch.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8438205 [details] [diff] [review]
Bug_1016255.patch

Review of attachment 8438205 [details] [diff] [review]:
-----------------------------------------------------------------

You've modified shared code here and added code that is specific to gallery to it.  This patch will completely break video playback in the camera app

::: shared/js/media/video_player.js
@@ +33,4 @@
>    var poster = newelt(container, 'img', 'videoPoster');
>    var player = newelt(container, 'video', 'videoPlayer');
>    var controls = newelt(container, 'div', 'videoPlayerControls');
> +  var videaotinyback = newelt(controls, 'button', 'videoPlayerBackButton hidden');

spelling mistake in the variable name

@@ +217,5 @@
>  
> +  // Hook up the back button
> +  videaotinyback.addEventListener('click', function(e) {
> +    if(!videaotinyback.classList.contains('hidden')) {
> +      setView(LAYOUT_MODE.list);

As the filename suggests, this is shared code, also used by the camera app. But you're adding a call to setView() which is only defined in the gallery app, so obviously this won't work.

@@ +225,5 @@
> +
> +  controls.addEventListener('swipe', function(e) {
> +    if (!self.playerShowing)
> +      videaotinyback.classList.add('hidden');
> +  });

Swipe events are handled at a much higher level, you can't add this here.

Comment 11

[David Flanagan [:djf]]

Amitav,

Thank you for reporting and working on this bug. What Tiffanie is asking for here makes the fix much, much harder because of the way that gallery and camera both use the shared video player code.

Here's what I propose:

1) Amitav: please fix the bug by just adding the pointer-events:none CSS so that the button does not get clicked when it is hidden.  I think this will probably be a simple one-line bug fix.

2) Tif: if you want to have a back button visible while the video controls are visible, please file a new bug for that and let's treat it as a feature request to go along with all the other Gallery refresh UX work we're going to be doing.  There isn't any way you would know this, but it turns out that what you are asking for here will involve some refactoring and will be kind of tricky to get right.

Comment 12

[Tiffanie Shakespeare [:tif]]

Thanks David! I think your comments make sense and it's totally fine then to go with the simple fix. 

Instead of filing a new bug to get the rest of the fix, I think it will be a better use of everyone's time to just wait for the Gallery refresh when we'll re-examine navigation.

Thanks Amitav for your work!

(In reply to David Flanagan [:djf] from comment #11)
> Amitav,
> 
> Thank you for reporting and working on this bug. What Tiffanie is asking for
> here makes the fix much, much harder because of the way that gallery and
> camera both use the shared video player code.
> 
> Here's what I propose:
> 
> 1) Amitav: please fix the bug by just adding the pointer-events:none CSS so
> that the button does not get clicked when it is hidden.  I think this will
> probably be a simple one-line bug fix.
> 
> 2) Tif: if you want to have a back button visible while the video controls
> are visible, please file a new bug for that and let's treat it as a feature
> request to go along with all the other Gallery refresh UX work we're going
> to be doing.  There isn't any way you would know this, but it turns out that
> what you are asking for here will involve some refactoring and will be kind
> of tricky to get right.

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8438205 [details] [diff] [review]
Bug_1016255.patch

Cancelling review as David has reviewed the patch and provided needed feedback.

Comment 14

[amitav.anand]

Attachment: PR

Thanks David for your valuable inputs.
Disabled the pointer-event for back button.

Comment 15

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8438967 [details] [review]
PR

Changing to UI-review?

Just a heads up - review flag is for developers to take a look at your code and UI-review is for designers to take a look at the interactions and UI.

Giving a +, issue seems to be resolved.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8438967 [details] [review]
PR

A trivial one-line change. Looks good, and works correctly.

I will land it now.

Comment 17

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/b8d320ec4e71f9315e875861793b0fb72cb95e07

Comment 18

[David Flanagan [:djf]]

Thank you, Amitav!