Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled #22120

MattLichtenstein · 2024-09-20T20:14:05Z

📜 Tickets

Jira ticket
Github issue

💡 Description

When the address toolbar is set to the top position and we are on a webpage, only show the address toolbar bottom border once scrolled

📝 Notes

This reverts much of what was changed in Bugfix FXIOS-10004 [Toolbar redesign] Fix address toolbar bottom borders on scroll #22079 where we would dispatch websiteDidScroll based on the current redux state. This does not work because scrollViewDidScroll happens to quickly and frequently meaning the state information being checked to determine if we should dispatch another action will likely be outdated

📹 Video

Simulator.Screen.Recording.-.iPhone.15.-.2024-09-26.at.17.59.51.mp4

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

thatswinnie · 2024-09-23T08:01:03Z

firefox-ios/Client/Frontend/Browser/Toolbars/Redux/AddressBarState.swift

+        guard let toolbarState = store.state.screenState(ToolbarState.self, for: .toolbar, window: action.windowUUID)
+        else { return state.borderPosition }
+
+        let isWebPage = action.url?.isWebPage() ?? false


Figma says that the navigation border should also be shown when the toolbar is set to top and we are on the homepage.
Also I am not sure why the middleware is not used to get the correct position anymore. I don't really like the mix between having logic here and also using Redux as the source of truth. So far the middleware was handling the toolbar border position using the ToolbarManager. Is there a reason we don't use that anymore but also leave the code in place?

Maybe I am misunderstanding the spec, but to me, it reads that it should only be visible on scroll when the toolbar is set to the top and on the homepage, but not when the page is scroll to the top.

I put this logic directly in the reducer instead of the middleware because it had no dependencies (if you don't include ToolbarManager). This code is specifically for when the url changes and we either navigate to a webpage and show the border, or navigate to the homepage and hide the border. We still use the middleware (and ToolbarManager) for monitoring for homepage scroll. I will take a look and see if there is a more consistent way to write this where the logic isn't as spread out, because I agree, it is confusing

cyndichin · 2024-09-23T14:08:27Z

firefox-ios/Client/Frontend/Browser/TabScrollController.swift

-                GeneralBrowserMiddlewareAction(
-                    scrollOffset: scrollView.contentOffset,
-                    windowUUID: windowUUID,
-                    actionType: GeneralBrowserMiddlewareActionType.websiteDidScroll))


for context, are we removing this because it is already handled in the Home Page and making this redundant?

This code was specifically for showing/hiding the border when scrolling on webpages, which we don't care about because we always want to show the address bar border when on a webpage, and that is being handled in the new changes to AddressBarState.

Scrolling on the homepage is handled in HomepageViewController.

Oh okay, thanks for the clarity, didn't realize the design requirements for this. thanks!

Ahh, I was totally wrong on this - as @thatswinnie pointed out in a comment below, the design requirement is to show the top address bar bottom border on scroll - so I will be adding this back 😅

Oh okay cool, thank you! Feel free to ping me if you still like a review from me. thanks!

cyndichin · 2024-09-23T14:10:32Z

firefox-ios/Client/Frontend/Browser/Toolbars/Redux/AddressBarState.swift

@@ -192,13 +192,14 @@ struct AddressBarState: StateType, Equatable {

        case ToolbarActionType.urlDidChange:
            guard let toolbarAction = action as? ToolbarAction else { return state }
+            let borderPosition = getAddressToolbarBorderPosition(action: toolbarAction, state: state)


I like that we're using an existing action, but also curious on why not moving the logic in the middleware and can pass in the border position using
let borderPosition = action.addressBorderPosition

This would require a urlDidChange middleware action, right? This was mainly to avoid adding an unnecessary middleware action for something that didn't technically need (i.e. no dependencies required) to go through the middleware, but I understand wanting to have all of the border logic in once place. Do you think it makes sense to add a middleware action here?

We could also send two actions: the existing urlDidChange that goes to the reducer and one to the middleware that updates only the border. This way we don't duplicate code.

The concerns I have about this code being in multiple places is that it renders the code we have in DefaultToolbarManager and it's unit tests useless and it's not the source of truth anymore. And we can't call the DefaultToolbarManager from the reducer as it's not supposed to have dependencies.

I have added ToolbarMiddlewareActionType.urlDidChange so that the middleware continues to be the only one who handles border position logic therefore this logic is now removed from the AddressBarState's reducer. The downfall is that we now have to dispatch both ToolbarMiddlewareActionType.urlDidChange and ToolbarActionType.urlDidChange in tandem 😪

Instead of in tandem, what about dispatching one action that gets sent to the middleware first and then once the border position is calculated then we dispatch another action that can update the state altogether?

Also side note, I didn't add a isMicrosurveyShown check in the DefaultToolbarManager because I feel that the toolbar manager should be independent of knowing external views. It makes it seem coupled by adding the additional checks. What do you think about doing something similar to the microsurvey in which for the web page check we just send the payload of updating the border when needed?

thatswinnie · 2024-09-25T08:26:05Z

BrowserKit/Sources/ToolbarKit/ToolbarManager.swift

                                         scrollY: CGFloat) -> AddressToolbarBorderPosition {
        // display the top border if
        // - the toolbar is displayed at the bottom
        // display the bottom border if
-        // - the toolbar is displayed at the top and the website was scrolled
+        // - the toolbar is displayed at the top and page is scrolled (applies to homepage)
+        // - the toolbar is displayed at the top and we are on a webpage


I am confused about this change. Is this coming from design? In Figma the requirements for websites are the following:

So the bottom border for websites should only be displayed on scroll unless the user is in private mode - only then it's displayed always.

mergify · 2024-09-26T15:32:58Z

This pull request has conflicts when rebasing. Could you fix it @MattLichtenstein? 🙏

mobiletest-ci-bot · 2024-09-26T22:15:09Z

	Messages
📖	Project coverage: 31.58%
📖	Edited 5 files
📖	Created 0 files

Client.app: Coverage: 30.12

File	Coverage
BrowserViewController.swift	5.07%	⚠️
HomepageViewController.swift	36.98%	⚠️
TabScrollController.swift	66.09%	✅
BrowserViewController+WebViewDelegates.swift	4.7%	⚠️

Generated by 🚫 Danger Swift against 5b1292f

thatswinnie · 2024-09-27T07:30:20Z

...ontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift

@@ -741,6 +741,11 @@ extension BrowserViewController: WKNavigationDelegate {
                        actionType: ToolbarActionType.urlDidChange
                    )
                    store.dispatch(action)
+                    let middlewareAction = ToolbarMiddlewareAction(
+                        windowUUID: windowUUID,


I think the scrollOffset should be added as a parameter as well.

cyndichin · 2024-10-01T12:49:55Z

firefox-ios/Client/Frontend/Browser/TabScrollController.swift

+        if (lastContentOffsetY > 0 && scrollView.contentOffset.y <= 0) ||
+            (lastContentOffsetY <= 0 && scrollView.contentOffset.y > 0) {
+            lastContentOffsetY = scrollView.contentOffset.y


cyndichin

looking great now!~ theres some overlap between the tab scroll and home page so would be nice if we could keep it DRY. However, this was implemented prior so can leave as is. approved! thanks for going through the iterations!

MattLichtenstein requested a review from a team as a code owner September 20, 2024 20:14

MattLichtenstein requested a review from cyndichin September 20, 2024 20:14

thatswinnie reviewed Sep 23, 2024

View reviewed changes

cyndichin reviewed Sep 23, 2024

View reviewed changes

thatswinnie reviewed Sep 25, 2024

View reviewed changes

MattLichtenstein added 4 commits September 26, 2024 14:07

calculate border position on url change

084df8f

let middleware handle border position

18f29b8

linter

7edbcda

fix + add tests

4410006

MattLichtenstein force-pushed the ml/FXIOS-10099_address-toolbar-border-missing-on-webpage branch from 2aa788a to 4410006 Compare September 26, 2024 18:07

MattLichtenstein had a problem deploying to github-pages September 26, 2024 18:08 — with GitHub Actions Failure

MattLichtenstein added 3 commits September 26, 2024 17:53

only show address toolbar borders on scroll

56aa32a

comment

f00a3d6

fix test names

35db1ff

MattLichtenstein changed the title ~~Bugfix FXIOS-10099 Always show address toolbar bottom border on webpages~~ Bugfix FXIOS-10099 Only show address toolbar bottom border on webpages when scrolled Sep 26, 2024

lint

3e2a09b

MattLichtenstein requested a review from thatswinnie September 26, 2024 22:05

thatswinnie reviewed Sep 30, 2024

View reviewed changes

add param

5b1292f

MattLichtenstein requested a review from thatswinnie September 30, 2024 14:12

MattLichtenstein changed the title ~~Bugfix FXIOS-10099 Only show address toolbar bottom border on webpages when scrolled~~ Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled Sep 30, 2024

thatswinnie approved these changes Oct 1, 2024

View reviewed changes

cyndichin reviewed Oct 1, 2024

View reviewed changes

cyndichin approved these changes Oct 1, 2024

View reviewed changes

MattLichtenstein merged commit 5f94cbf into main Oct 1, 2024
12 of 13 checks passed

MattLichtenstein deleted the ml/FXIOS-10099_address-toolbar-border-missing-on-webpage branch October 1, 2024 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled #22120

Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled #22120

MattLichtenstein commented Sep 20, 2024 •

edited

Loading

thatswinnie Sep 23, 2024

MattLichtenstein Sep 23, 2024

cyndichin Sep 23, 2024

MattLichtenstein Sep 23, 2024

cyndichin Sep 25, 2024

MattLichtenstein Sep 25, 2024

cyndichin Sep 25, 2024

cyndichin Sep 23, 2024

MattLichtenstein Sep 23, 2024

thatswinnie Sep 24, 2024

MattLichtenstein Sep 24, 2024

cyndichin Sep 25, 2024

thatswinnie Sep 25, 2024

mergify bot commented Sep 26, 2024

mobiletest-ci-bot commented Sep 26, 2024 •

edited

Loading

thatswinnie Sep 27, 2024

cyndichin Oct 1, 2024

cyndichin left a comment

Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled #22120

Bugfix FXIOS-10099 [Toolbar redesign] Only show address toolbar bottom border on webpages when scrolled #22120

Conversation

MattLichtenstein commented Sep 20, 2024 • edited Loading

📜 Tickets

💡 Description

📝 Notes

📹 Video

📝 Checklist

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

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

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

Choose a reason for hiding this comment

mergify bot commented Sep 26, 2024

mobiletest-ci-bot commented Sep 26, 2024 • edited Loading

Client.app: Coverage: 30.12

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyndichin left a comment

Choose a reason for hiding this comment

MattLichtenstein commented Sep 20, 2024 •

edited

Loading

mobiletest-ci-bot commented Sep 26, 2024 •

edited

Loading