For #13336: Open bookmarks in current tab by czlucius · Pull Request #23169 · mozilla-mobile/fenix

czlucius · 2022-01-12T15:23:31Z

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.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

Hi there
These are my fixes to #13336, to open bookmarks in the current tab. I am not sure whether this change is approved, but as the issue is still open, I am submitting a draft PR.
May I ask the Fenix UX team for their approval on this change, or is the bookmarks opening in a new tab as intended? I can work on it to add other things if needed.
Thank you!

Here are the videos of the proposed change:

1.Screen_Recording_20220112-231257_Firefox.Preview.mp4

Bookmarklets also work:

Screen_Recording_20220112-231756_Firefox.Preview.mp4

firefoxci-taskcluster · 2022-01-12T15:23:49Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

jonalmeida

The change looks good. You need to fix the tests (see the run-testDebugUnitTest task) and comment below. 🙂

jonalmeida · 2022-01-12T20:07:38Z

        openInNewTabAndShow(
            item.url!!,
-            true,
+            false, // See https://github.com/mozilla-mobile/fenix/issues/13336


No need to add a reference to the issue here. The commit message contains a reference to it already.

Suggested change

false, // See https://github.com/mozilla-mobile/fenix/issues/13336

false,

czlucius · 2022-01-13T13:14:15Z

@jonalmeida
I've removed the comment already.
The test that fails is

TEST: handleBookmarkTapped should open the bookmark

According to the test at BookmarkControllerTest, it checks if a bookmark is opened in a new tab:

fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt

Lines 112 to 132 in a49f80d

    
           @Test 
        
           fun `handleBookmarkTapped should load the bookmark in a new tab`() { 
        
               var invokePendingDeletionInvoked = false 
        
               val flags = EngineSession.LoadUrlFlags.select(EngineSession.LoadUrlFlags.ALLOW_JAVASCRIPT_URL) 
        
               createController( 
        
                   invokePendingDeletion = { 
        
                       invokePendingDeletionInvoked = true 
        
                   } 
        
               ).handleBookmarkTapped(item) 
        
               assertTrue(invokePendingDeletionInvoked) 
        
               verify { 
        
                   homeActivity.openToBrowserAndLoad( 
        
                       item.url!!, 
        
                       true, 
        
                       BrowserDirection.FromBookmarks, 
        
                       flags = flags 
        
                   ) 
        
               } 
        
           }

However, this issue is for the bookmark to open in the current tab, so the test is failing

jonalmeida · 2022-01-13T17:15:12Z

That's correct! The verify was asserting that we always opened in a new tab, and this the behaviour you've changed, so we just need to fix the test to verify the new behaviour. 🙂

czlucius · 2022-01-14T03:37:19Z

Ok, I've corrected the test.

czlucius · 2022-01-14T03:41:32Z

About the new behaviour of opening in the current tab, since Fenix doesn't associate the home screen with a physical tab, I found out that this will cause a problem: If the user opens a new tab(clicks New Tab), then subsequently opens the bookmark, the bookmark would open in the previous tab, which may not be the intended action the user wanted to take. How should we fix this side effect? (is there a method to find out whether the user is on the home screen page of a new tab? - there could be other instances where the home screen is launched without the intention to open a new tab, e.g. the home button)
Thanks.

czlucius · 2022-01-14T06:24:48Z

I've modified the test, but it's still failing

czlucius · 2022-01-17T04:47:23Z

@jonalmeida
May I ask why is the test failing even when both values (the one in the test, and the one in the BookmarkController class) are false? I've tried this out on my machine, this does not happen when both are true. Is there something else that I am missing here?
Thanks.

jonalmeida · 2022-01-17T16:38:03Z

@czlucius there were two tests that were failing:

handleBookmarkTapped should open the bookmark
handleBookmarkTapped should load the bookmark in a new tab

You fixed one correctly, you just need to fix the other. 🙂

czlucius · 2022-01-18T05:42:05Z

I've fixed the tests already.

About the new behaviour of opening in the current tab, since Fenix doesn't associate the home screen with a physical tab, I found out that this will cause a problem: If the user opens a new tab(clicks New Tab), then subsequently opens the bookmark, the bookmark would open in the previous tab, which may not be the intended action the user wanted to take. How should we fix this side effect? (is there a method to find out whether the user is on the home screen page of a new tab? - there could be other instances where the home screen is launched without the intention to open a new tab, e.g. the home button) Thanks.

What do we do about this?

handleBookmarkTapped should load the bookmark in a new tab

Should I also change the test name to reflect the new behavior?

Thank you.

maverick74 · 2022-01-23T13:44:52Z

@jonalmeida @czlucius isn't there a way to, for example, know if the home button is visible?
If we could know, it could be used to either load the bookmark on a new tab or in the present tab...

czlucius · 2022-01-26T07:58:44Z

@jonalmeida what do I do to continue from here?
Thanks.

jonalmeida · 2022-01-26T17:21:46Z

Sorry, I lost track of the message thread!

I've fixed the tests already.

🎉

About the new behaviour of opening in the current tab, since Fenix doesn't associate the home screen with a physical tab, I found out that this will cause a problem: If the user opens a new tab(clicks New Tab), then subsequently opens the bookmark, the bookmark would open in the previous tab, which may not be the intended action the user wanted to take. How should we fix this side effect? (is there a method to find out whether the user is on the home screen page of a new tab? - there could be other instances where the home screen is launched without the intention to open a new tab, e.g. the home button) Thanks.

What do we do about this?

Interesting. There is no concept of a "new tab" in Fenix, it's just the home screen. When the bookmark is opened, does it switch to that tab that it loads in? If so, I think that would be acceptable behaviour for now given how Fenix currently works. We can also discuss this further with UX in a separate issue as well.

handleBookmarkTapped should load the bookmark in a new tab

Should I also change the test name to reflect the new behavior?

Yes please! The name of the test should reflect the behaviour of it. If we're changing the behaviour, someone else coming back to this code in the future will be very confused why it says something and does something else. 🙂

czlucius · 2022-01-27T04:20:03Z

Ok 👍

czlucius · 2022-01-27T04:22:27Z

Interesting. There is no concept of a "new tab" in Fenix, it's just the home screen. When the bookmark is opened, does it switch to that tab that it loads in? If so, I think that would be acceptable behaviour for now given how Fenix currently works. We can also discuss this further with UX in a separate issue as well.

Your comment got me to think about if the user has 0 tabs(starting from a clean state), then this behaviour will attempt to open the bookmark in a nonexistent current tab, which may result in a crash or no-op. I shall go test this behaviour later.

czlucius · 2022-01-27T10:50:17Z

Interesting. There is no concept of a "new tab" in Fenix, it's just the home screen. When the bookmark is opened, does it switch to that tab that it loads in? If so, I think that would be acceptable behaviour for now given how Fenix currently works. We can also discuss this further with UX in a separate issue as well.

Your comment got me to think about if the user has 0 tabs(starting from a clean state), then this behaviour will attempt to open the bookmark in a nonexistent current tab, which may result in a crash or no-op. I shall go test this behaviour later.

Quite interestingly, the code still works well when I open a bookmark when number of tabs is 0. I guess openToBrowserAndLoad already has edge case that settled

jonalmeida · 2022-01-27T19:00:10Z

Your comment got me to think about if the user has 0 tabs(starting from a clean state), then this behaviour will attempt to open the bookmark in a nonexistent current tab, which may result in a crash or no-op. I shall go test this behaviour later.

This is good train of thought to consider and even better that it is already handled by the openToBrowserAndLoad method.

jonalmeida · 2022-01-27T21:29:22Z

@czlucius I tried out the patch and it looks good! However there is one more entry point into bookmarks from the home screen's "Recent bookmarks". I wonder if you're interested in also fixing that too? Otherwise, we can file a separate bug for it.

I'm also happy to land this as is, and if you're still interested in picking up the other issue as a follow-up. 🙂

czlucius · 2022-01-28T06:34:13Z

I think the recent bookmarks from the home screen is somewhat related to this issue: #20012

If bookmarks were to open in the current tab, but top sites still open in a new tab, the behaviour will be quite confusing.

Furthermore, the home screen is also shown when the user explicitly clicks "New tab", and opening the recent bookmark in the current tab would not be the intended behaviour in this case.
Perhaps another issue can be filed about this, and I can look into it?

Thanks.

czlucius · 2022-01-30T15:05:45Z

@jonalmeida
If the fix is good as it is now, you can merge it(and maybe open a new issue for the home screen one - maybe for top sites, bookmarks and also collections?)

The minor flaw that I described earlier is illustrated in this recording:

Screen_Recording_20220130-225842_Firefox.Preview.mp4

Thanks.

Cheap-Skate · 2022-02-02T10:35:17Z

@jonalmeida If the fix is good as it is now, you can merge it(and maybe open a new issue for the home screen one - maybe for top sites, bookmarks and also collections?)

Please open this issue & Thanks for your efforts so far 👍!!

…rent tab

czlucius · 2022-02-03T05:29:01Z

#2871 is related as well, and since this is fixed, that issue may be fixed too

czlucius · 2022-02-07T11:31:55Z

@jonalmeida
The issue that I stated above is being reported by some users:
#23595

Is there a method that returns whether the home screen is opened? We could open in new or current tab based on this value.

Thanks!

maverick74 · 2022-02-07T16:22:18Z

Created bug #23609 to address top sites behavior

Cheap-Skate · 2022-02-10T07:15:56Z

#23659 another one reporting the new behavior as a bug

czlucius · 2022-02-15T12:30:04Z

@jonalmeida this seems to be causing problems, as seen in #23595, #23659, and #23746. I think this should be looked into(before this gets into release).

maverick74 · 2022-02-15T12:41:23Z

@czlucius i think you missed bug #20012

czlucius · 2022-02-15T12:43:08Z

No, I was talking about the problems caused by changing the behavior to open bookmarks in the current tab.

maverick74 · 2022-02-15T12:49:00Z

Oh! My mistake then! Sorry.
But, i think 20012 should be addressed (after the ones you mentioned) as well to avoid more confusion with this change

This reverts commit e73deb2.

This reverts commit e73deb2. (cherry picked from commit d1c0e9b)

This reverts commit e73deb2. (cherry picked from commit d1c0e9b) Co-authored-by: Roger Yang <royang@mozilla.com>

…le#23169) * For mozilla-mobile#13336: Open bookmarks in current tab * For mozilla-mobile#13336: Fix tests to verify bookmark opening in current tab * Change test name for handleBookmarkTapped

…lla-mobile#23169)" This reverts commit e73deb2.

jonalmeida reviewed Jan 12, 2022

View reviewed changes

czlucius force-pushed the fix-issue-13336 branch from 363184a to 1ddef7e Compare January 13, 2022 13:00

jonalmeida added the pr:needs-changes PRs that need some changes/fixes before they can land label Jan 13, 2022

czlucius force-pushed the fix-issue-13336 branch from bde067b to 222cf09 Compare January 18, 2022 04:59

Yahiroz mentioned this pull request Jan 19, 2022

Bookmarklets doesn't work from bookmarks #2871

Closed

czlucius marked this pull request as ready for review January 30, 2022 15:08

czlucius requested review from a team as code owners January 30, 2022 15:08

czlucius added 2 commits February 2, 2022 19:51

For mozilla-mobile#13336: Open bookmarks in current tab

d1c7a83

For mozilla-mobile#13336: Fix tests to verify bookmark opening in cur…

d5048d6

…rent tab

Change test name for handleBookmarkTapped

e9b4089

jonalmeida force-pushed the fix-issue-13336 branch from 2659d52 to e9b4089 Compare February 3, 2022 00:51

jonalmeida added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed pr:needs-changes PRs that need some changes/fixes before they can land labels Feb 3, 2022

gabrielluong added pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] and removed pr:needs-landing PRs that are ready to land [Will be merged by Mergify] labels Feb 3, 2022

jonalmeida approved these changes Feb 3, 2022

View reviewed changes

mergify Bot merged commit e73deb2 into mozilla-mobile:main Feb 3, 2022

czlucius mentioned this pull request Feb 3, 2022

Bookmarks should open in existing tab when navigating to bookmarks UI from a page #13336

Closed

delxg mentioned this pull request Feb 15, 2022

[Bug]: Opening a bookmark in private browsing mode doesn't work if there are no existing private tabs #23746

Closed

rocketsroger added a commit that referenced this pull request Feb 22, 2022

Revert "For #13336: Open bookmarks in current tab (#23169)"

b1a36ac

This reverts commit e73deb2.

rocketsroger mentioned this pull request Feb 22, 2022

Revert "For #13336: Open bookmarks in current tab" #23897

Merged

mergify Bot pushed a commit that referenced this pull request Feb 23, 2022

Revert "For #13336: Open bookmarks in current tab (#23169)"

d1c0e9b

This reverts commit e73deb2.

mergify Bot pushed a commit that referenced this pull request Feb 23, 2022

Revert "For #13336: Open bookmarks in current tab (#23169)"

b45adf2

This reverts commit e73deb2. (cherry picked from commit d1c0e9b)

rocketsroger added a commit that referenced this pull request Feb 23, 2022

Revert "For #13336: Open bookmarks in current tab (#23169)" (#23914)

690c94f

This reverts commit e73deb2. (cherry picked from commit d1c0e9b) Co-authored-by: Roger Yang <royang@mozilla.com>

Cheap-Skate mentioned this pull request Mar 4, 2022

new prefs switch in UI #24087

Closed

DavidJCobb mentioned this pull request Mar 22, 2022

Make Home screen with an actual tab #20012

Closed

pedroldk pushed a commit to pedroldk/fenix that referenced this pull request Mar 22, 2022

Revert "For mozilla-mobile#13336: Open bookmarks in current tab (mozi…

a177f7f

…lla-mobile#23169)" This reverts commit e73deb2.

	false, // See https://github.com/mozilla-mobile/fenix/issues/13336
	false,

Conversation

czlucius commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

To download an APK when reviewing a PR:

Uh oh!

firefoxci-taskcluster Bot commented Jan 12, 2022

Uh oh!

jonalmeida left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonalmeida Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

czlucius commented Jan 13, 2022

Uh oh!

jonalmeida commented Jan 13, 2022

Uh oh!

czlucius commented Jan 14, 2022

Uh oh!

czlucius commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czlucius commented Jan 14, 2022

Uh oh!

czlucius commented Jan 17, 2022

Uh oh!

jonalmeida commented Jan 17, 2022

Uh oh!

czlucius commented Jan 18, 2022

Uh oh!

maverick74 commented Jan 23, 2022

Uh oh!

czlucius commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonalmeida commented Jan 26, 2022

Uh oh!

czlucius commented Jan 27, 2022

Uh oh!

czlucius commented Jan 27, 2022

Uh oh!

czlucius commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonalmeida commented Jan 27, 2022

Uh oh!

jonalmeida commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czlucius commented Jan 28, 2022

Uh oh!

czlucius commented Jan 30, 2022

Uh oh!

Cheap-Skate commented Feb 2, 2022

Uh oh!

czlucius commented Feb 3, 2022

Uh oh!

czlucius commented Feb 7, 2022

Uh oh!

maverick74 commented Feb 7, 2022

Uh oh!

Cheap-Skate commented Feb 10, 2022

Uh oh!

czlucius commented Feb 15, 2022

Uh oh!

maverick74 commented Feb 15, 2022

Uh oh!

czlucius commented Feb 15, 2022

Uh oh!

maverick74 commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

czlucius commented Jan 12, 2022 •

edited

Loading

jonalmeida left a comment •

edited

Loading

czlucius commented Jan 14, 2022 •

edited

Loading

czlucius commented Jan 26, 2022 •

edited

Loading

czlucius commented Jan 27, 2022 •

edited

Loading

jonalmeida commented Jan 27, 2022 •

edited

Loading