Blockable JS Alerts inside tabs

[graeme]

Task/Issue URL: https://app.asana.com/0/72649045549333/1201615584595450/f
Tech Design URL: https://app.asana.com/0/481882893211075/1203352316751829/f
CC:

Description:

We need to update our JS alerts so that they:

• are presented on the tab level, preventing spammy alerts from blocking the window
• allow the user to block additional alerts being shown on that website

Steps to test this PR:
https://privacy-test-pages.glitch.me/features/js-alerts.html

Testing checklist:

• Test with Release configuration
• Test proper deallocation of tabs
• Make sure committed submodule changes are desired

---

Internal references:

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

[mallexxx]

The code looks fine, good job! 👍
Please see some comments inline

[mallexxx]

please let‘s keep the Tab.swift code with minimal changes needed, the alert state can be managed using only the userInteractionDialog property

[graeme]

I'm not 100% sure about that... but I'll remove the alert blocking functionality now based on your above comment and we can discuss this more if we decide to include that in a later PR

[mallexxx]

it seems we don‘t need this extra property here since it‘s held in the closure

[mallexxx]

please avoid force unwrapping when not needed, extra let jsAlertController = ..create(); self. jsAlertController = jsAlertController would be fine

[Bunn]

Nice 🎉

[Bunn]

The code looks good, I think Alex caught all the issues before.
There are some swiftlint rules not being respected that would be good to take a look before merging.

That said I found 2 issues

• 1/ This is minor, I'd check with design to see if they think it's a blocker, IMO it is a non critical edge case but if the window is really small and then an alert is displayed, it automatically increases the window size. I'll leave it to you to decide if you want to handle this or not.
  See the video:

ResizeBug.mov

---

• 2/ The color gets messed up if you change the system from/to light/dark mode. IMO a blocker and the reason I'm setting as request changes.

[github-actions]

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 9b42b1d

[mallexxx]

I noticed another issue, the button colour doesn‘t match the system accent colour (is always blue)

[mallexxx]

I think that‘s really close, good job!
I‘ve found several issues and we‘re good to go:

• Alert background isn‘t blocking click events, e.g. Inspector UI elements aren‘t blocked when the alert is displayed, right-clicking background alert displays page context after the alert is hidden
• Scrolling position (for long text) is not zero at appearance (see screenshot attached)
  

Enter key still doesn‘t work properly ☹️ :

• textField receives gets First Responder even if it‘s hidden
• after selecting the AddressBar and hitting Esc key Enter won‘t dismiss the alert
• Esc key doesn‘t dismiss an alert() - it does in Safari
• I suggest implementing acceptsFirstResponder { true } and handling keys in keyDown handler in the JSAlertController, setting first responder to jsAlertController.view on viewDidAppear.
• As a fallback for the AddressBar deactivated with Esc key I suggest the following:

        view.window?.observe(\.firstResponder) { window, _ in
            if window.firstResponder is WebView {
                self.view.makeMeFirstResponder()
            }
        }

[mallexxx]

Another minor thing I noticed is when an empty alert() is shown there‘s an empty place in the displayed alert. I‘m not strong on fixing this though

[mallexxx]

Scrolling problem still present, it‘s fixed for me if I replace sizeToFit() with view.layoutSubtreeIfNeeded()
.. just wondering can it fix the delayed animation problem either?

Other than that, LGTM!
Approved after fixing the issue

Alex has taken over