Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev tools visibility fix #457

Merged
merged 6 commits into from
Mar 11, 2022
Merged

Dev tools visibility fix #457

merged 6 commits into from
Mar 11, 2022

Conversation

tomasstrba
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/1199178362774117/1201903875992992/f
Tech Design URL:
CC:

Description:
Fix of dev tools visibility after switching between tabs

Steps to test this PR:

  1. Open the devtools.
  2. Switch to a different tab
  3. Switch back to the original tab.
  4. Make sure dev tools are open and visible

Execute testing steps from #295

  1. Open youtube.com full screen, exit full screen
  2. Open one tab with youtube.com; open full screen, hit cmd+w
  3. Open one New Tab, second tab with youtube.com; open full screen, hit cmd+w
  4. Open one window with youtube tab and a New Tab, enter full screen; Open another window on external display with youtube.com, enter full screen, hit cmd+w in 2nd window, hit ctrl+Tab in 1st window

Testing checklist:

  • Test on Big Sur or Catalina

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@mallexxx mallexxx self-assigned this Mar 9, 2022
@tomasstrba
Copy link
Contributor Author

@mallexxx, please, are you planning to review this PR? I can find someone else if you are busy

@mallexxx
Copy link
Collaborator

@tomasstrba yes, doing it now

Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rolled back change was meant to remove inspector view from webView's parent view when switching Tabs, otherwise there were some weird effects of multiple inspector stacking when many tabs have inspector open.
Instead I suggest adding the following code to WebView.swift, it will fix the issue:

  override func viewDidMoveToWindow() {
        super.viewDidMoveToWindow()
        if self.isInspectorShown {
            self.openDeveloperTools()
        }
    }

@mallexxx mallexxx assigned tomasstrba and unassigned mallexxx Mar 11, 2022
@tomasstrba
Copy link
Contributor Author

@mallexxx got it! 👍 Thanks! Let me adjust the the solution

@tomasstrba tomasstrba assigned mallexxx and unassigned tomasstrba Mar 11, 2022
@tomasstrba
Copy link
Contributor Author

Alex, thanks for the tip! 👍 This is ready for a re-review

@@ -42,6 +42,12 @@ final class WebView: WKWebView {
"WKMenuItemIdentifierDownloadLinkedFile": UserText.downloadLinkedFileAs,
"WKMenuItemIdentifierSearchWeb": UserText.searchWithDuckDuckGo
]

deinit {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also reorganized WebView.swift and added few marks. Hope it is ok

Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (after fixing the // swiftlint:enable type_body_length linter issue)

@mallexxx mallexxx assigned tomasstrba and unassigned mallexxx Mar 11, 2022
@tomasstrba tomasstrba merged commit eb170ad into develop Mar 11, 2022
@tomasstrba tomasstrba deleted the tom/dev-tools-fix branch March 11, 2022 14:43
samsymons added a commit that referenced this pull request Mar 16, 2022
* develop:
  fix launch link when no windows are open (#470)
  Version 0.20.1
  Update storyboards to use correct color names. (#468)
  Fix suggestion selection crash (#469)
  Version 0.20.0
  Dev tools visibility fix (#457)
  Fix BrowserTabViewController leakage (#463)
  Remove the upgrade logic. (#460)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants