Bug 1812144 - `Homescreen` appbar drag issue fixed #852

gitstart · 2023-02-16T19:32:24Z

Homescreen Appbar is scrollable even if there is no content, from customize homepage all options disabled and then the appbar is scrolling if you tap on the bar and scroll up.

HomeScreen

Issue Screenshots

fnx-60-issue.webm

Fix Screenshots

fnx-60-fix.webm

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

Mugurell

Overall it looks okay but it is not a complete fix.
It is still possible to have more than two items in the RecyclerView but with it's height still smaller than needed to overflow the screen height and so the issue will still be reproduceable even after this patch.

LostAppBar.mp4

Based on the above scenario it seems to me that we'd need to disable AppBarLayout's dragging based on whether it's plus the height of the RecyclerView is bigger than the available screen height.

*Maybe a bit more performant solution would be to set a flag only once on whether the homescreen should be draggable or not and use this in the setDragCallback of the AppBarLayout.

Mugurell · 2023-02-27T09:34:28Z

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+            val behavior = AppBarLayoutBehaviorEmptyRecyclerView()
+            appbarLayoutParams.behavior = behavior
+        }
+        binding.homeAppBar.setExpanded(true, true)


I'm curious on why this is needed - line 513.
It will break the functionality from when the user navigates to a newly created collection.

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

fenix/app/src/main/java/org/mozilla/fenix/utils/AppBarLayoutBehaviorEmptyRecyclerView.kt

gitstart · 2023-02-27T11:00:53Z

Thank you @Mugurell for reviewing the PR. 🙏
Will update the requested changes soon. 🚀

github-actions · 2023-02-27T14:36:38Z

🚧 Commit message is using the wrong format: Update SavedLoginsListView.kt

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-02-27T14:37:03Z

🚧 Commit message is using the wrong format: Update SavedLoginsListView.kt

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-02-27T14:37:45Z

🚧 Commit message is using the wrong format: Update SavedLoginsListView.kt

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-02-27T14:45:16Z

🚧 Commit message is using the wrong format: Update SavedLoginsListView.kt

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-02-27T14:45:19Z

🚧 Commit message is using the wrong format: Update SavedLoginsListView.kt

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

gitstart · 2023-02-27T15:56:55Z

Hello @Mugurell,🙏
Pushed requested changes. 🚀

Mugurell

Thank you for the update!
Just one naming comment to ensure this change will be clear as intended for future readers.

Mugurell · 2023-03-01T16:26:04Z

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+            appBarBehavior.setDragCallback(
+                object : AppBarLayout.Behavior.DragCallback() {
+                    override fun canDrag(appBarLayout: AppBarLayout): Boolean {
+                        return false
+                    }
+                },
+            )


Interesting approach to disable dragging in all scenarios.

Looking through the initial tickets for the current functionality - mozilla-mobile/fenix#99, mozilla-mobile/fenix#137, mozilla-mobile/fenix#7700 I don't see any discussions or indications about the scenarios in which we'd want the appBar to be draggable or not.

Given that currently the appBar can be dragged only when there aren't enough elements on the screen to overflow it and in this scenario the drag gesture results in the bug we need so solve disabling dragging seems like the sensible choice.

Mugurell · 2023-03-01T16:28:44Z

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

@@ -503,6 +505,21 @@ class HomeFragment : Fragment() {
        }
    }

+    private fun setScrollingBehavior() {


Sorry for the being pedantic but the new approach that will disable dragging in all situations I think warrants being specific about the intent.
Can you please rename to something like disableAppBarDragging().
It's an important behavior change that I think we need to highlight.

gitstart · 2023-03-02T06:50:58Z

Thank you for the update! Just one naming comment to ensure this change will be clear as intended for future readers.

Thank you @Mugurell for reviewing the PR.🙏
I have updated the function name as per the recommendation. ✅

Mugurell · 2023-03-02T08:26:37Z

bors try

bors · 2023-03-02T09:01:28Z

try

Build failed:

complete-push

Mugurell · 2023-03-02T10:57:55Z

bors retry

bors · 2023-03-02T11:33:17Z

try

Build failed:

complete-push

Mugurell · 2023-03-02T15:06:52Z

Same ui test failing in both try runs:
org.mozilla.fenix.ui.TabbedBrowsingTest#verifyContextMenuShortcuts with the following stacktrace:

java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.mozilla.fenix.helpers.MatcherHelper.assertItemWithResIdExists(MatcherHelper.kt:48)
at org.mozilla.fenix.helpers.MatcherHelper.assertItemWithResIdExists$default(MatcherHelper.kt:45)
at org.mozilla.fenix.ui.robots.HomeScreenRobot.verifyHomeWordmark(HomeScreenRobot.kt:195)
at org.mozilla.fenix.ui.TabbedBrowsingTest$verifyContextMenuShortcuts$11.invoke(TabbedBrowsingTest.kt:366)
at org.mozilla.fenix.ui.TabbedBrowsingTest$verifyContextMenuShortcuts$11.invoke(TabbedBrowsingTest.kt:361)
at org.mozilla.fenix.ui.robots.NavigationToolbarRobot$Transition.openTabFromShortcutsMenu(NavigationToolbarRobot.kt:242)
at org.mozilla.fenix.ui.TabbedBrowsingTest.verifyContextMenuShortcuts(TabbedBrowsingTest.kt:361)

Seems like a new issue which needs to be investigated more before attempting to merge.

gitstart · 2023-03-03T06:23:55Z

Same ui test failing in both try runs: org.mozilla.fenix.ui.TabbedBrowsingTest#verifyContextMenuShortcuts with the following stacktrace:

java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.mozilla.fenix.helpers.MatcherHelper.assertItemWithResIdExists(MatcherHelper.kt:48)
at org.mozilla.fenix.helpers.MatcherHelper.assertItemWithResIdExists$default(MatcherHelper.kt:45)
at org.mozilla.fenix.ui.robots.HomeScreenRobot.verifyHomeWordmark(HomeScreenRobot.kt:195)
at org.mozilla.fenix.ui.TabbedBrowsingTest$verifyContextMenuShortcuts$11.invoke(TabbedBrowsingTest.kt:366)
at org.mozilla.fenix.ui.TabbedBrowsingTest$verifyContextMenuShortcuts$11.invoke(TabbedBrowsingTest.kt:361)
at org.mozilla.fenix.ui.robots.NavigationToolbarRobot$Transition.openTabFromShortcutsMenu(NavigationToolbarRobot.kt:242)
at org.mozilla.fenix.ui.TabbedBrowsingTest.verifyContextMenuShortcuts(TabbedBrowsingTest.kt:361)

Seems like a new issue which needs to be investigated more before attempting to merge.

Hello @Mugurell,
Nothing was touched on tests. Do you need any help from our end? 🙏
Thank you!

Mugurell · 2023-03-03T16:28:28Z

Hello @Mugurell, Nothing was touched on tests. Do you need any help from our end? 🙏 Thank you!

Seems like the issue is real and is happening after these changes in this very specific scenario:

have a private tab opened
long press on the tab counter to see a contextual menu
choose "New tab"

Expected result (as seen in the current applications):

the Firefox header is seen at the top of the screen.

Actual result (after this patch):

the Firefox logo is not seen at the top of the screen, as seen in the below video:

FenixHeaderBug.mp4

We need to have the Firefox logo shown in all cases where the users goes to the homescreen - with the least amount of changes.
Or find another way to solve the initial issue.

gitstart · 2023-03-06T09:17:55Z

Hello @Mugurell, Nothing was touched on tests. Do you need any help from our end? 🙏 Thank you!

Seems like the issue is real and is happening after these changes in this very specific scenario:

have a private tab opened

long press on the tab counter to see a contextual menu

choose "New tab"

Expected result (as seen in the current applications):

the Firefox header is seen at the top of the screen.

Actual result (after this patch):

the Firefox logo is not seen at the top of the screen, as seen in the below video:

We need to have the Firefox logo shown in all cases where the users goes to the homescreen - with the least amount of changes. Or find another way to solve the initial issue.

Okay @Mugurell, Let me check it.

Mugurell · 2023-03-06T12:10:42Z

fenix/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+            )
+            appBarLayoutParams.behavior = appBarBehavior
+        }
+        binding.homeAppBar.setExpanded(true)


I see this change being added in the last commit.
Is this proposed as a fix for the issue seen in tests - described more here - #852 (comment) ?

binding.homeAppBar.setExpanded(true) would regress another behavior as described here - #852 (comment)

If the patch cannot be improved to avoid both issues we could check with UX to temporary approve one since the original issue seems a bit more important to fix.

Let me know what your thoughts are.

Hello @Mugurell,
This fix is workaround with less code. If we need proper implementation, then for simple fix a lot more code would be added. As you mentioned on #852 (Comment) that make sure to add least changes.

Mugurell · 2023-03-06T16:47:16Z

@mozilla-mobile/ux Need your help on deciding how to go forward with this. We have 3 scenarios:

the original issue - https://bugzilla.mozilla.org/show_bug.cgi?id=1812144 seems more important since in that scenario the user could not easily scroll the homescreen to see again the header. They could navigate to a page/open settings and then go back to the homescreen as a long workaround.
Trying to fix that issue leaves us with two new ones:
with this patch we would though break the scenario in which when navigating to a newly created collection from the snackbar the Firefox header would be hidden - after this patch it would be shown OR
with this patch we would break the scenario in which when we'd try to open a normal tab while being in private mode the Firefox header would be shown - after this patch it would be hidden.

Giving that we plan to refactor the homescreen to Compose is it acceptable to merge this patch to fix the issue from https://bugzilla.mozilla.org/show_bug.cgi?id=1812144 and temporarily get one of the last two issues or should we avoid merging and leave the fix for the Bugzilla issue to be included in the Compose refactoring?

lime124 · 2023-03-06T18:01:56Z

@Verdi can you provide guidance on this one please?

Verdi · 2023-03-07T16:13:13Z

Ok @Mugurell and I went over the issues. The patch fixes the initial issue and makes the other two issues better by making the header visible. We should land this.

Mugurell · 2023-03-07T16:42:46Z

Thank you all for the support and sticking through with this!
Looks like we are good to go!

@gitstart Can you please

remove the line about collapsing the header when navigating to a new collection
update the commit / PR title to say that with the patch that scenario is fixed instead of the suggesting that the result will be have the homescreen scrollable even with no content
squash your commits locally to only remain with one that can be easily landed

github-actions · 2023-03-08T10:48:37Z

🚧 Commit message is using the wrong format: updated-changes

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-03-08T10:51:14Z

🚧 Commit message is using the wrong format: updated-changes

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-03-08T10:51:33Z

🚧 Commit message is using the wrong format: updated-changes

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

gitstart · 2023-03-08T10:53:21Z

Thank you all for the support and sticking through with this! Looks like we are good to go!

@gitstart Can you please

remove the line about collapsing the header when navigating to a new collection

update the commit / PR title to say that with the patch that scenario is fixed instead of the suggesting that the result will be have the homescreen scrollable even with no content

squash your commits locally to only remain with one that can be easily landed

Thank you @Mugurell,
Requested all the changes are updated.

Mugurell

LGTM. Thank you!

Mugurell · 2023-03-08T11:13:28Z

bors try

Mugurell · 2023-03-08T11:16:51Z

Just a small remark that this will lead to another bug which will have the header expanded after navigating to a newly created collection - issue seen when there are more sections shown on the screen and the screen has to be scrolled to highlight the collection. This was discussed with @ Verdi and agreed that it's a smaller issue than the original one in which the private tabs button would not be seen and accepted until the Compose refactoring.

ScrollingToACollection.mp4

bors · 2023-03-08T13:35:55Z

try

Build succeeded:

complete-push

gitstart marked this pull request as ready for review February 16, 2023 19:36

debojyoti452 force-pushed the FNXOSS-60 branch from 47285b1 to 3ba9f20 Compare February 27, 2023 08:24

Mugurell self-requested a review February 27, 2023 09:57

Mugurell suggested changes Feb 27, 2023

View reviewed changes

gitstart force-pushed the FNXOSS-60 branch from 3ba9f20 to 123d036 Compare February 27, 2023 11:00

debojyoti452 force-pushed the FNXOSS-60 branch from fb1b525 to f72a8bf Compare February 27, 2023 14:30

debojyoti452 approved these changes Feb 27, 2023

View reviewed changes

gitstart force-pushed the FNXOSS-60 branch from 81c9c60 to af5f37a Compare February 27, 2023 14:51

Mugurell reviewed Mar 1, 2023

View reviewed changes

gitstart force-pushed the FNXOSS-60 branch from 765cc00 to 6b670e3 Compare March 2, 2023 06:54

bors bot pushed a commit that referenced this pull request Mar 2, 2023

Try #852:

7fd733f

bors bot pushed a commit that referenced this pull request Mar 2, 2023

Try #852:

36b7c46

Mugurell reviewed Mar 6, 2023

View reviewed changes

Mugurell added do not land PRs that requires coordination before landing Needs-UX Issues or PRs that need UX input or review labels Mar 6, 2023

Mugurell removed the Needs-UX Issues or PRs that need UX input or review label Mar 7, 2023

gitstart changed the title ~~Bug 1812144 - Homescreen is scrollable even if there is no content~~ Bug 1812144 - Homescreen title bar drag issue fixed Mar 8, 2023

gitstart changed the title ~~Bug 1812144 - Homescreen title bar drag issue fixed~~ Bug 1812144 - Homescreen appbar drag issue fixed Mar 8, 2023

Bug 1812144 - Homescreen appbar drag issue fixed

87048c8

gitstart force-pushed the FNXOSS-60 branch from 6a3552a to 87048c8 Compare March 8, 2023 10:51

Mugurell approved these changes Mar 8, 2023

View reviewed changes

bors bot pushed a commit that referenced this pull request Mar 8, 2023

Try #852:

74f2c73

Mugurell added 🛬 needs landing PRs that are ready to land and removed do not land PRs that requires coordination before landing labels Mar 8, 2023

mergify bot and others added 2 commits March 8, 2023 15:26

Merge branch 'main' into FNXOSS-60

fc380ca

Merge branch 'main' into FNXOSS-60

5ec1d36

mergify bot merged commit e1de44e into mozilla-mobile:main Mar 8, 2023

This pull request was closed.

Bug 1812144 - Homescreen appbar drag issue fixed #852

Bug 1812144 - Homescreen appbar drag issue fixed #852

Conversation

gitstart commented Feb 16, 2023 • edited by github-actions bot Loading

Issue Screenshots

Fix Screenshots

Mugurell left a comment

Choose a reason for hiding this comment

Mugurell Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

gitstart commented Feb 27, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Feb 27, 2023

gitstart commented Feb 27, 2023

Mugurell left a comment

Choose a reason for hiding this comment

Mugurell Mar 1, 2023

Choose a reason for hiding this comment

Mugurell Mar 1, 2023

Choose a reason for hiding this comment

gitstart commented Mar 2, 2023

Mugurell commented Mar 2, 2023

bors bot commented Mar 2, 2023

try

Mugurell commented Mar 2, 2023

bors bot commented Mar 2, 2023

try

Mugurell commented Mar 2, 2023

gitstart commented Mar 3, 2023

Mugurell commented Mar 3, 2023 • edited Loading

gitstart commented Mar 6, 2023

Mugurell Mar 6, 2023

Choose a reason for hiding this comment

gitstart Mar 6, 2023

Choose a reason for hiding this comment

Mugurell commented Mar 6, 2023

lime124 commented Mar 6, 2023

Verdi commented Mar 7, 2023

Mugurell commented Mar 7, 2023

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

gitstart commented Mar 8, 2023

Mugurell left a comment

Choose a reason for hiding this comment

Mugurell commented Mar 8, 2023

Mugurell commented Mar 8, 2023

bors bot commented Mar 8, 2023

try

Bug 1812144 - `Homescreen` appbar drag issue fixed #852

Bug 1812144 - `Homescreen` appbar drag issue fixed #852

gitstart commented Feb 16, 2023 •

edited by github-actions bot

Loading

Mugurell Feb 27, 2023 •

edited

Loading

Mugurell commented Mar 3, 2023 •

edited

Loading