Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Revert "Exit full screen on tab close" (fixes #2618) #2631

Merged
merged 2 commits into from
Jul 23, 2016

Conversation

benmosher
Copy link
Contributor

Sorry, better PR. Found others that were based against dev-channel, that seems to be the right target.

Fixes #2618 by reverting commit 8cded9c.

@bbondy
Copy link
Member

bbondy commented Jul 21, 2016

I think there is a bug relating to html5 video full screen. Could you try your patch with that too?
Pls compare to what Chrome does for it.

@bbondy
Copy link
Member

bbondy commented Jul 22, 2016

You're still tweaking this one right?

@benmosher
Copy link
Contributor Author

I haven't had a chance to take a second look yet, no. I will need to get more familiar with the code to make an actual change, this is just a revert of your original commit for #2404.

@benmosher
Copy link
Contributor Author

Looking at this now. I think I see what needs to be done to satisfy #2404 and #2618.

@benmosher
Copy link
Contributor Author

benmosher commented Jul 22, 2016

@bbondy I'm not in love with how I solved this, but it does seem to work.

I couldn't figure out how to do this from inside windowStore, as it seemed to be necessary to run JS in the frame.webview before closing it. Only other thing might be to handle it in componentWillUnmount?

edit: reset to 1001033 and reimplemented via componentWillUnmount in frame.js (0af85e5). Much cleaner and more complete. Appears to appease #2404 and #2618 and match Chrome behavior.

@bbondy
Copy link
Member

bbondy commented Jul 22, 2016

This is a good idea but it's only working about half the time for me (race condition).
Could you add something to the js/actions/webviewActions.js w/ a callback and then call it from the windowStore close frame? Moving the rest of the window action close frame code inside the callback that you pass. I think that would solve it consistently .

@bbondy
Copy link
Member

bbondy commented Jul 22, 2016

Note also that if it is not full screen to call the callback right away.

@benmosher
Copy link
Contributor Author

Ah, makes sense, didn't realize there were actions with access to the current webview. I'll take a look.

@benmosher
Copy link
Contributor Author

benmosher commented Jul 22, 2016

Updated. Seems to work as well. Removed from componentWillMount.

Composed action in windowActions.js's closeFrame, I'm skittish about importing *Actions inside a store, even if they don't dispatch anything... ??

[--- Commented from Asana.com
#commenter brad richter
---[aa]

@bbondy
Copy link
Member

bbondy commented Jul 23, 2016

Excellent, thanks for working through this!

@bbondy bbondy merged commit 74f254f into brave:dev-channel Jul 23, 2016
@luixxiul luixxiul added this to the 0.11.3dev milestone Jul 24, 2016
@bbondy bbondy modified the milestones: 0.11.2dev, 0.11.3dev Jul 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants