There is intermittent confusion in the full screen control representation.

mediapack-me commented 6 years ago

PLEASE FILL THIS OUT IF YOU WANT HELP WITH AN ISSUE!

Bug, feature request, or proposal:

Bug

What is the current behavior?

There is intermittent confusion in the full screen control representation. The control can display the wrong state. There is an image attached that shows the #mat-video at less than full screen. The icon should be the one that takes the user to full-screen. The actual icon is the one that takes the user back to not-full-screen. full-size-control-issue

What is the expected behavior?

The control should show the "Go to Full Screen" Icon when NOT in Full screen mode.

What are the steps to reproduce?

Providing a StackBlitz reproduction is the best way to share your issue.
StackBlitz starter: https://goo.gl/wwnhMV
The behavior is intermittent. It is possible to make this behavior impossible. Doing so would probably take much less time than trying to locate the intermittent cause.

What is the use-case or motivation for changing an existing behavior?

It's incorrect. ;-)

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Angular 6/ Material 6 / Typescript 2.7.2 / Screen shot detail is from latest FireFox.

Is there anything else I should know?

The behavior is intermittent so it might be difficult to reproduce.

In my opinion, the best way to proceed is to restrict the decision to a single choke point in the code that controls this underlying variable in both direction.

It also probably a good idea to ensure that the default is set to not-full-screen. Then ensure that the only way this can change is by an action taken by the user on the control.

Note* I can't think of any web use cases where the starting state would be full-screen. But it's probably still a good idea to let the composing developer control the default.

mediapack-me commented 6 years ago

Here's a little more detail that might help to sort this out.
The cause is probably a little more complex and a little less intermittent than we originally thought.
This control's "confusion" seems to takes place when the user has the browser set to full screen.

That means, the controlling variable needs to "andded" ;-) with the browser's fullscreen condition.
The video fullscreen state must consider the browser's fullscreen state when examining its own.

nkoehler commented 6 years ago

Fullscreen is a tricky topic because most browsers allow users to press Esc to exit fullscreen, among other non standard options. Limiting the control to only button presses is not a viable solution because of this unfortunately.

Getting the browsers fullscreen condition is something that needs investigating first. Considering the lack of widespread support for the fullscreen api, I'm doubtful that all/any browsers will actually expose this value in a way that can trigger an event.

And if this incident is isolated to the browsers fullscreen option, and not the video player itself, can we really consider it a bug?

mediapack-me commented 6 years ago

There are a couple of things. 1) Screenfull.js (find on github, npm, yarn) gets downloaded about 130,000 times a month. It enables the composing developer to track browser full screen state. So, it's not that isolated. We're able to track it across all the browsers #mat-video# supports.

2) It's perfectly fine to choose to ignore a bug. But I'm not sure it's ok to stare at one and call it a frog. ;-)

nkoehler commented 6 years ago

Hmm... well I already track the fullscreen state in fullscreen.service.ts using events, so I will take a look at how Screenfull.js is doing in comparison.

nkoehler commented 6 years ago

After poking through Screenfull.js and my own implementation in fullscreen.service.ts, they are virtually doing the same thing and listening for the same events.

Can you confirm that this issue is only occurring when using the browsers fullscreen option when mat-video is filling the entire page? (ex: no parent div)

mediapack-me commented 6 years ago

The condition occurs when:

The browser is at fullscreen. AND when
mat-video# is NOT filling the entire page.

In the example screen shot, #mat-video# is just part of the "content" in a sidenav container and that sidenav is within the app container. So, it's FAR from filling the entire page.

In a nutshell, the component needs to be aware of the fullscreen state of the browser.

This should be as easy as tracking two booleans:

   isBrowserFullscreen: boolean;
   isMatVideoFullscreen: boolean;

Set up a couple of listeners. I'd suggest using BehaviorSubjects. Then you can just call next whenever either tracked fullscreen state changes. You don't even need to think about whether they're true or false anywhere but at the place where you set the icons.

   isBrowserFullscreen$: BehaviorSubject<boolean>;
   isMatVideoFullscreen$: new BehaviorSubject<boolean>;

   isBrowserFullscreen$ = new BehaviorSubject(isBrowserFullscreen)
   isMatVideoFullscreen$ = new BehaviorSubject(isMatVideoFullscreen)

HERE'S THE PROBLEM The fullscreen starting state for the browser is unknown. You're assuming it is NOT fullscreen. That's not always a valid assumption

   isBrowserFullscreen = false;  // starting state: this is where you need to focus
                                 // because a user might be in fullscreen before navigating 
                                 // to  a MatVideo component within an app.  Right?

Once you've set up the correct initial state. You can just listen for browser changes and whenever the browser fullscreen state changes:

   isBrowserFullscreen$.next(isBrowserFullScreen);

The starting state for MatVideo is a known quantity:

   isMatVideoFullscreen = false; //

Whenever the MatVideo fullscreen state changes:


   isMatVideoFullscreen$.next(isMatVideoFullscreen);

As long as you're tracking both, then you can be sure that you have all the information you need to make your decision.

Here's a starting point for a "truth table"

If MatVideo AND the Browser are both fullscreen -- Then MatVideo needs to display the exit full screen icon (this is where the issue occurs now)
If MatVideo is fullscreen AND the Browser is NOT full screen -- Then MatVideo needs to display the exit full screen icon (this is where the issue occurs now)
If the Browser is NOT full screen and MatVideo is NOT full screen -- Then MatVidoe needs to display the go to full screen icon (this is covered in th original assumption)

NOTE We have to address the sources for the videos this week. The sources (captions and audio) are HUGE for our requirments, so I agreed to tackle that problem this weekend. Once I get through those, and I've had a chance to get someone to test them, I'll send you suggested instructions for the implementation. If you're still not through this, then I'll send you an implementation to address the fullscreen icon issue, but I have to get the sources working ;-)

nkoehler commented 6 years ago

Have you only been able to reproduce this issue in Firefox? Because mat-video is working as desired in Chrome.

Screenshots: chrome browser and video fullscreen chrome browser fullscreen not video

I went through your truth table and every case worked as expected in Chrome.

And I will comment on the other issue for my thoughts on sources/tracks.

mediapack-me commented 6 years ago

Did you fix it?

So you are letting me know that after your fix it works in Chrome?

Or are you saying you are NOT going to fix it because you can't reproduce it in Chrome?

Let me know so I can schedule time to address it if you haven't fixed it.

Thanks

On May 12, 2018 at 4:12 PM nkoehler notifications@github.com wrote:

Have you only been able to reproduce this issue in Firefox? Because mat-video is working as desired in Chrome.

Screenshots:
[chrome browser and video fullscreen] https://user-images.githubusercontent.com/10172004/39961117-5ba071c8-55fd-11e8-855a-efb5152b0e88.png
[chrome browser fullscreen not video] https://user-images.githubusercontent.com/10172004/39961118-5bb0c4b0-55fd-11e8-88dc-1c751bfe94d8.png

I went through your truth table and every case worked as expected in Chrome.

And I will comment on the other issue for my thoughts on sources/tracks.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/nkoehler/mat-video/issues/2#issuecomment-388580016 , or mute the thread https://github.com/notifications/unsubscribe-auth/Ajb_mnOnY69T_1ltGKrztLNdbUmxKviGks5tx0JEgaJpZM4T4dk3 .

nkoehler commented 6 years ago

I did not make any changes or fixes. The latest release works as expected in Chrome. I am just wondering if this issue is specific to Firefox because I need to know where to spend my time trying to reproduce it.

mediapack-me commented 6 years ago

See last comment.

On May 12, 2018 at 4:20 PM nkoehler notifications@github.com wrote:

I did not make any changes or fixes. The latest release works as expected in Chrome. I am just wondering if this issue is specific to Firefox because I need to know where to spend my time trying to reproduce it.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/nkoehler/mat-video/issues/2#issuecomment-388580417 , or mute the thread https://github.com/notifications/unsubscribe-auth/Ajb_mrkAcsS8ts0T8BQzlZueMWC82ay0ks5tx0QFgaJpZM4T4dk3 .

nkoehler commented 6 years ago

Provide a StackBlitz with routes for the issue or I will not fix it.

mediapack-me commented 6 years ago

No worries.

On May 12, 2018 at 4:40 PM nkoehler notifications@github.com wrote:

Provide a StackBlitz with routes for the issue or I will not fix it.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/nkoehler/mat-video/issues/2#issuecomment-388581503 , or mute the thread https://github.com/notifications/unsubscribe-auth/Ajb_mq56LWhp_fo78RGotmyD4uum8xYvks5tx0i1gaJpZM4T4dk3 .

nkoehler commented 6 years ago

Unable to reproduce the issue with the given information. Since no further information has been provided, this issue will be closed.

nkoehler / mat-video