1767434 - Library window is not scrolled and focused onto the selected bookmark when sorting is used after landing patch from bug #1471546

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

Library window is not scrolled and focused onto the selected bookmark when sorting is used after landing patch from bug #1471546.

Regression range pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2e154dcda079ad6ca50bc5d18cbd0991386c5989&tochange=2ca189b8925dba3f28a1cefdc3e6f143ffbee290

mozillaregression-GUI points that regression is caused by:
Narrowed integration regression window from [78ae535c, 2ca189b8] (3 builds) to [2e154dcd, 2ca189b8] (2 builds) (~1 steps left)
Found commit message:
Bug 1471546 - Scroll to bookmark in bookmark library on initial selection when it's not visible r=mak

Differential Revision:
https://phabricator.services.mozilla.com/D139905

Steps to reproduce:

1. Open latest Mozilla Firefox Nighty 102.0a1 (2022-05-03) (64-bit)
2. Open Library
3. Sort bookmarks
4. Select bookmark
5. Notice that Library window is not scrolled and focused onto the selected bookmark

Observed results:
Library window is not scrolled and focused onto the selected bookmark when sorting is used.

Expected results:
Library window is scrolled and focused onto the selected bookmark when sorting is used.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

apparently the view is scrolled back to the top... likely .bookmarkIndex is broken if it's not a naturally sorted bookmark folder?

Comment 2

[James Teow [:jteow]]

Yes, upon checking it locally bookmarkIndex doesn't get updated when a list is re-sorted from the default. I was using it as a shortcut so that I wouldn't always have to commit to a lookup but for accuracy maybe we should always do ContentArea.currentView.view.treeIndexForNode(view.selectedNode); and scrap using bookmarkIndex. What do you think Marco?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to James Teow [:jteow] from comment #2)

Yes, upon checking it locally bookmarkIndex doesn't get updated when a list is re-sorted from the default. I was using it as a shortcut so that I wouldn't always have to commit to a lookup but for accuracy maybe we should always do ContentArea.currentView.view.treeIndexForNode(view.selectedNode); and scrap using bookmarkIndex. What do you think Marco?

If we want to keep the shortcut for perf reasons, I think you could check PlacesUtils.nodeIsFolder(...view.result.root), it would only apply in that case.

Comment 4

[James Teow [:jteow]]

Attachment: Bug 1767434 - Make updateDetailsPane always retrieve treeIndex r?mak

The problem with my previous solution for bug 1471546 was that bookmarkIndex is
not updated when a user sorts the list of bookmarks. So my solution is to always
use the treeIndex since its more reliable.

I've also added two tests: the first checks if the row the visible after clicking
a bookmark that's close to the bottom of the viewable area, and the second does the
same after re-sorting the list.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to P1 as the bug is tracked by a release manager for the current beta.
See Triage for Bugzilla for more information.
If you disagree, please discuss with a release manager.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is marked as tracked for firefox102 (nightly) and tracked for firefox101 (beta).
:jteow, could you consider increasing the severity of this tracked bug?

For more information, please visit auto_nag documentation.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b1519f1aea8
Make updateDetailsPane always retrieve treeIndex r=mak

Comment 8

[James Teow [:jteow]]

Comment on attachment 9275079 [details]
Bug 1767434 - Make updateDetailsPane always retrieve treeIndex r?mak

Beta/Release Uplift Approval Request

• User impact if declined: Upon sorting a list of items in the Library window, clicking additional items in that window won't result in window scrolling to the correct item that was clicked.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Please read bug info: https://bugzilla.mozilla.org/show_bug.cgi?id=1767434#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes to the code should only affect the Library window and tests related to clicking an item within a Library window.
• String changes made/needed:
• Is Android affected?: No

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7b1519f1aea8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b4f8b435c679

Comment 11

[Ardelean Oana, Desktop QA [:oardelean]]

I managed to reproduce this issue on Windows 7 on Firefox 101.0b7(20220515185854). Verified as fixed on Nightly 102.0a1(20220517092745) on Windows 7.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9275079 [details]
Bug 1767434 - Make updateDetailsPane always retrieve treeIndex r?mak

Approved for 101.0b8.

Comment 13

[Ardelean Oana, Desktop QA [:oardelean]]

I managed to reproduce this issue on Windows 7 on Firefox 101.0b7(20220515185854). Verified as fixed on Firefox 101.0b8(20220517185920) on Windows 7.

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

As OP, I'm also confirming that bug is fixed in Mozilla Firefox Nightly 102.0a1 (2022-05-17).
Thank you very much! \o/