Windows are restored with the correct z-index #10580

NejcZdovc · 2017-08-18T10:32:33Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Resolves #10473

Test Plan:
See #10473

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-08-18T10:42:47Z

Codecov Report

Merging #10580 into master will decrease coverage by 0.01%.
The diff coverage is 7.69%.

@@            Coverage Diff             @@
##           master   #10580      +/-   ##
==========================================
- Coverage   54.66%   54.64%   -0.02%     
==========================================
  Files         249      249              
  Lines       21932    21943      +11     
  Branches     3429     3431       +2     
==========================================
+ Hits        11989    11991       +2     
- Misses       9943     9952       +9

Flag	Coverage Δ
#unittest	`54.64% <7.69%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
app/browser/windows.js	`24.31% <ø> (+0.09%)`	⬆️
js/actions/appActions.js	`13.28% <ø> (ø)`	⬆️
app/browser/reducers/windowsReducer.js	`18% <0%> (-0.14%)`	⬇️
js/stores/windowStore.js	`29.31% <11.11%> (-0.15%)`	⬇️

NejcZdovc · 2017-08-26T16:54:55Z

this PR is blocked by #10458

cezaraugusto · 2017-09-05T00:55:57Z

@NejcZdovc please confirm if this is ready for review and tag reviewers to check. thanks!

NejcZdovc · 2017-09-06T10:14:13Z

@cezaraugusto it's ready for a review

bbondy · 2017-09-15T14:20:10Z

js/actions/windowActions.js

@@ -1211,6 +1211,16 @@ const windowActions = {
      },
      windowValue
    })
+  },
+
+  onWindowFocus: function (windowId, windowValue) {


Can you move this to appAction?

bbondy · 2017-09-15T14:21:33Z

app/browser/reducers/windowsReducer.js

@@ -332,6 +333,7 @@ const windowsReducer = (state, action, immutableAction) => {
      break
    case appConstants.APP_WINDOW_CREATED:
      state = windowState.maybeCreateWindow(state, action)
+      windowActions.onWindowFocus(action.getIn(['windowValue', 'windowId']), action.get('windowValue'))


I don't think we can assume that app window created means always focused. You could maybe check the window here and if focused do that if for some reason the focus event isn't firing.

bbondy

See inline comments

cezaraugusto · 2017-09-15T20:05:23Z

I'm not sure about the test plan but I did the following:

3 tabs with a, b, c websites
tear-off each tab in that order. Have 3 windows with a, b, c (current visible window)
Shut down, open again
Window with tab c should show

is that an accurate test plan?

cezaraugusto · 2017-09-15T20:07:01Z

for the above windows opened in the reverse order so a, b, c instead of c, b, a. Not sure if that's the expected behavior.

NejcZdovc · 2017-09-18T06:31:59Z

@cezaraugusto yeah I didn't try this scenario, was using keyboard shortcuts only. Nice scenario. This use case should be fixed now. Can you please re-review?

@bbondy I rewrote this PR a little bit, so we don't need focus action anymore and detection rate is a lot better

NejcZdovc · 2017-09-18T06:32:26Z

I will squash commits before merging

bsclifton · 2017-09-18T16:20:11Z

app/index.js

-      loadedPerWindowImmutableState.forEach((wndState) => {
-        appActions.newWindow(undefined, undefined, wndState)
-      })
+      loadedPerWindowImmutableState


Maybe as a follow up, it would be good to pull out the sorting into something which can easily be unit tested. Maybe a new method determineZindex or similar (maybe in app/commmon/state/windowState)

then the below would be like:

const sortedList = windowState.determineZindex(loadedPerWindowImmutableStore) sortedList.forEach((wndState) => { appActions.newWindow(undefined, undefined, wndState) })

bsclifton · 2017-09-18T16:23:31Z

js/stores/windowStore.js

@@ -788,8 +790,17 @@ const doAction = (action) => {
        break
      }
    case windowConstants.WINDOW_ON_WINDOW_UPDATE:
-      windowState = windowState.set('windowInfo', action.windowValue)
-      break
+    case appConstants.APP_WINDOW_READY:


If you did a follow up, this would be fairly easy to add a unit test for too 😄 Basically checking that focusTime gets added to windowInfo

bsclifton

Changes work great 😄 👍 I tested on macOS and Windows. Comments left about what could be better (if you wanted to do a follow up). Great job! 😄

bsclifton · 2017-09-18T16:25:57Z

I'll hold off on merge if you wanted to squash / address feedback and maybe @bbondy can re-review 😄

cezaraugusto

lgtm but needs rebase on windowStore.js

Resolves brave#10473 Auditors: Test Plan:

NejcZdovc · 2017-09-19T20:27:27Z

@cezaraugusto rebased and squashed

bbondy · 2017-09-20T15:30:37Z

js/stores/windowStore.js

@@ -2,6 +2,8 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

+/* global performance */


you can just use window.performance and avoid this.

cezaraugusto

@NejcZdovc pls revisit this later for follow-up feedbacks, given this is approved by all I'm merging, thanks

Windows are restored with the correct z-index

cezaraugusto · 2017-09-22T05:04:31Z

master 843d9b3
0.20.x 8e370de

NejcZdovc added the enhancement label Aug 18, 2017

NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Aug 18, 2017

NejcZdovc self-assigned this Aug 18, 2017

NejcZdovc added the PR/blocked label Aug 18, 2017

NejcZdovc modified the milestones: 0.20.x (Developer Channel), 0.21.x (Nightly Channel) Aug 18, 2017

NejcZdovc added PR/work-in-progress ⚒ and removed PR/work-in-progress ⚒ labels Aug 18, 2017

NejcZdovc mentioned this pull request Aug 21, 2017

Window off screen when monitors change #3558

Closed

NejcZdovc force-pushed the feature/#10473-zindex branch from 10e569e to d238b26 Compare August 21, 2017 06:57

NejcZdovc removed the PR/blocked label Aug 28, 2017

NejcZdovc force-pushed the feature/#10473-zindex branch 5 times, most recently from d1b494c to 2eeb806 Compare August 29, 2017 15:08

NejcZdovc requested review from bsclifton and cezaraugusto September 5, 2017 16:05

ghost added the sprint/1 label Sep 13, 2017

bbondy reviewed Sep 15, 2017

View reviewed changes

bbondy suggested changes Sep 15, 2017

View reviewed changes

NejcZdovc force-pushed the feature/#10473-zindex branch 2 times, most recently from adc23fd to f2a0ba9 Compare September 18, 2017 06:30

NejcZdovc requested a review from bbondy September 18, 2017 06:32

bsclifton force-pushed the feature/#10473-zindex branch from f2a0ba9 to 1b7598d Compare September 18, 2017 15:52

bsclifton reviewed Sep 18, 2017

View reviewed changes

bsclifton approved these changes Sep 18, 2017

View reviewed changes

cezaraugusto approved these changes Sep 19, 2017

View reviewed changes

Adds z-index for windows when restoring from the session

2f0dcfb

Resolves brave#10473 Auditors: Test Plan:

NejcZdovc force-pushed the feature/#10473-zindex branch from 1b7598d to 2f0dcfb Compare September 19, 2017 20:27

bbondy reviewed Sep 20, 2017

View reviewed changes

bbondy approved these changes Sep 20, 2017

View reviewed changes

cezaraugusto approved these changes Sep 22, 2017

View reviewed changes

cezaraugusto merged commit 843d9b3 into brave:master Sep 22, 2017

cezaraugusto added a commit that referenced this pull request Sep 22, 2017

Merge pull request #10580 from NejcZdovc/feature/#10473-zindex

8e370de

Windows are restored with the correct z-index

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows are restored with the correct z-index #10580

Windows are restored with the correct z-index #10580

NejcZdovc commented Aug 18, 2017 •

edited by bsclifton

Loading

codecov-io commented Aug 18, 2017 •

edited

Loading

NejcZdovc commented Aug 26, 2017

cezaraugusto commented Sep 5, 2017

NejcZdovc commented Sep 6, 2017

bbondy Sep 15, 2017

NejcZdovc Sep 17, 2017

bbondy Sep 15, 2017

NejcZdovc Sep 17, 2017

bbondy left a comment

cezaraugusto commented Sep 15, 2017

cezaraugusto commented Sep 15, 2017

NejcZdovc commented Sep 18, 2017

NejcZdovc commented Sep 18, 2017

bsclifton Sep 18, 2017

bbondy Sep 20, 2017

bsclifton Sep 18, 2017

bsclifton left a comment

bsclifton commented Sep 18, 2017

cezaraugusto left a comment •

edited

Loading

NejcZdovc commented Sep 19, 2017

bbondy Sep 20, 2017

cezaraugusto left a comment

cezaraugusto commented Sep 22, 2017 •

edited

Loading

Windows are restored with the correct z-index #10580

Windows are restored with the correct z-index #10580

Conversation

NejcZdovc commented Aug 18, 2017 • edited by bsclifton Loading

codecov-io commented Aug 18, 2017 • edited Loading

Codecov Report

NejcZdovc commented Aug 26, 2017

cezaraugusto commented Sep 5, 2017

NejcZdovc commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy left a comment

Choose a reason for hiding this comment

cezaraugusto commented Sep 15, 2017

cezaraugusto commented Sep 15, 2017

NejcZdovc commented Sep 18, 2017

NejcZdovc commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Sep 18, 2017

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Sep 19, 2017

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto commented Sep 22, 2017 • edited Loading

NejcZdovc commented Aug 18, 2017 •

edited by bsclifton

Loading

codecov-io commented Aug 18, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading

cezaraugusto commented Sep 22, 2017 •

edited

Loading