Fix off-screen element detection issue

[mojganii]

Due to the increasing number of items in VPN settings, our current UI test struggles to locate elements that are off-screen. This update resolves the issue by implementing scrolling to find the targeted elements.

---

This change is 

[buggmagnet]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 602 at r1 (raw file):

    private func isExpanded(_ section: Section) -> Bool {
        let snapshot = snapshot()

I realize that the only reason the previous solution worked, was that the table view was not big enough to actually start recycling headers, since the only place where the information about being expanded was the header itself, and it's always false by default, had the table been big enough, we would have started recycling expanded headers, and hilarity would have ensued.

That being said, this solution has the same problem. By doing this, we're implicitly relying on the view states to keep the information whether headers are expanded or not, which when they get reused, will not work correctly.

The VPNSettingsViewModel should instead keep the information whether headers are expanded here.

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):
This will not always work, swipeUp doesn't let you control where the swipe will end, so you might scroll past the element you're trying to reach.

Same for the swipeDown usage below.

I'm also confused now, because the tap documentation says the following :

If the element exists within a scrollable view but is offscreen, XCTest will attempt to scroll the element onscreen before performing the tap.

@niklasberglund do you know what could be the problem that we are facing here ?

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 36 at r1 (raw file):
I don't think we need this, there's already the [isHittable](https://developer.apple.com/documentation/xctest/xcuielement/1500561-ishittable property which does what we want

isHittable returns true if the element exists and can be clicked, tapped, or pressed at its current location. It returns false if the element does not exist, is offscreen, or is covered by another element.

[rablador]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 602 at r1 (raw file):

Previously, buggmagnet wrote…

I realize that the only reason the previous solution worked, was that the table view was not big enough to actually start recycling headers, since the only place where the information about being expanded was the header itself, and it's always false by default, had the table been big enough, we would have started recycling expanded headers, and hilarity would have ensued.

That being said, this solution has the same problem. By doing this, we're implicitly relying on the view states to keep the information whether headers are expanded or not, which when they get reused, will not work correctly.

The VPNSettingsViewModel should instead keep the information whether headers are expanded here.

I think it will be safe to ask the snapshot for this information. It's more or less a view model itself and keeps track of the raw data you're providing it with. If x number of items have been added to the snapshot and you ask it at any later time, you should get x number of items back.

[niklasberglund]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):

If the element exists within a scrollable view but is offscreen, XCTest will attempt to scroll the element onscreen before performing the tap.

Oohh really? I was not aware of this.

swipeUp doesn't let you control where the swipe will end, so you might scroll past the element you're trying to reach.

If its scrolling past the element(and this swiping is still necessary) you could specify a slower gesture velocity https://developer.apple.com/documentation/xctest/xcuicoordinate/3752787-swipeup

[niklasberglund]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):

If the element exists within a scrollable view but is offscreen, XCTest will attempt to scroll the element onscreen before performing the tap.

But usually it won't exist because of table view optimisation right? It will only exist if its partially visible or maybe a tiny bit off screen? So the automatic scrolling is not as helpful as it sounds

[mojganii]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 602 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think it will be safe to ask the snapshot for this information. It's more or less a view model itself and keeps track of the raw data you're providing it with. If x number of items have been added to the snapshot and you ask it at any later time, you should get x number of items back.

@buggmagnet your original option is true.the problems is related to dequeue the header.but there are two solutions here:
1- keep tracking of expanding state in the view model
2- use screenshot to tell us about the state which is the thing @rablador mentioned and I agree to ask screenshot for the state instead of keeping the flag in view model.

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

If the element exists within a scrollable view but is offscreen, XCTest will attempt to scroll the element onscreen before performing the tap.

But usually it won't exist because of table view optimisation right? It will only exist if its partially visible or maybe a tiny bit off screen? So the automatic scrolling is not as helpful as it sounds

regarding tap method. it's not working about the UITableViewHeaderFooterView but when it comes to UITableViewCell then there is no need to scroll to find the element.

@niklasberglund do we have a safer way to look for an element or velocity can solve the problem? I was thinking to scroll page by page.
I see what @buggmagnet is referring but I couldn't find a better solution .

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 36 at r1 (raw file):

Previously, buggmagnet wrote…

I don't think we need this, there's already the [isHittable](https://developer.apple.com/documentation/xctest/xcuielement/1500561-ishittable property which does what we want

isHittable returns true if the element exists and can be clicked, tapped, or pressed at its current location. It returns false if the element does not exist, is offscreen, or is covered by another element.

yeah, but isHittable relies on isUserInteractionEnabled, then if the item is already finable by accessible identifier but it's not clickable then isHittable returns false. checking the exists flag to recognize if the element is there or not is safer IMHO.

[buggmagnet]

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @mojganii, @niklasberglund, and @rablador)

---

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 602 at r1 (raw file):

Previously, mojganii wrote…

@buggmagnet your original option is true.the problems is related to dequeue the header.but there are two solutions here:
1- keep tracking of expanding state in the view model
2- use screenshot to tell us about the state which is the thing @rablador mentioned and I agree to ask screenshot for the state instead of keeping the flag in view model.

Alright, if you both feel this is an acceptable solution, then let's go with it 👍

However, We should add a comment explaining that we are using the snapshot as the source of truth on whether headers are expanded or not.

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):

Previously, mojganii wrote…

regarding tap method. it's not working about the UITableViewHeaderFooterView but when it comes to UITableViewCell then there is no need to scroll to find the element.

@niklasberglund do we have a safer way to look for an element or velocity can solve the problem? I was thinking to scroll page by page.
I see what @buggmagnet is referring but I couldn't find a better solution .

We discussed a bit more with @mojganii, and we came to the conclusion that while the solution is not perfect, it is working, and it is an improvement over what we currently have, therefore, we are going with this.

If the tests start failing again in the future, of if this doesn't scale, we will revisit this later.

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 20 at r2 (raw file):

    }

    func scrollUpToElement(element: XCUIElement, maxScrolls: UInt = 5) {

As a side note, shouldn't that be scroll down to element, since we're using a swipe up motion ? (same for the other method)

[niklasberglund]

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 23 at r1 (raw file):

Previously, buggmagnet wrote…

We discussed a bit more with @mojganii, and we came to the conclusion that while the solution is not perfect, it is working, and it is an improvement over what we currently have, therefore, we are going with this.

If the tests start failing again in the future, of if this doesn't scale, we will revisit this later.

@mojganii @buggmagnet nice 👍

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 20 at r2 (raw file):

Previously, buggmagnet wrote…

As a side note, shouldn't that be scroll down to element, since we're using a swipe up motion ? (same for the other method)

Agree with @buggmagnet and wonder if scrolling up is necessary or do we actually only need to scroll down?

[rablador]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)

[mojganii]

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 602 at r1 (raw file):

Previously, buggmagnet wrote…

Alright, if you both feel this is an acceptable solution, then let's go with it 👍

However, We should add a comment explaining that we are using the snapshot as the source of truth on whether headers are expanded or not.

Done.

---

ios/MullvadVPNUITests/XCUIElement+Extensions.swift line 20 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Agree with @buggmagnet and wonder if scrolling up is necessary or do we actually only need to scroll down?

Done.

[buggmagnet]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.