1839066 - "Library" window: Cannot edit bookmarks / sort folder when previous view was "Downloads"

Status: VERIFIED FIXED

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

Description

[Markus.N]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/114.0

Steps to reproduce:

• Open "Show All Downloads" from the main menu (if visible) or Ctrl-Shift-Y otherwise.
• Expand "All Bookmarks" if not already expanded.
• Select "Bookmarks Menu" or any of it's sub-folders.
• Select a bookmark.

Actual results:

• The bookmark editor at the bottom does not open.
• Right-clicking on the folder (in the tree pane) seems to have no effect.
• Manually sorting bookmarks by drag-and-drop seems to have no effect.

Expected results:

The bookmark editor should open, and both of the sorting operations should also have a visible effect.

If you switch to another folder and back (green arrow on top left), the editor opens again, and the results of the previously done sort operations become visible.

It happens on any folder that is selected right after "Downloads" and only to the first folder selected this way.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Bookmarks & History' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: Editing bookmark in Library window is broken.

Error in the Browser Console:

Error: Cannot use an incomplete node to initialize the edit bookmark panel
    _setPaneInfo chrome://browser/content/places/editBookmark.js:85
    initPanel chrome://browser/content/places/editBookmark.js:295
    PO__fillDetailsPane chrome://browser/content/places/places.js:761
    PO_updateDetailsPane chrome://browser/content/places/places.js:443
    onfocus chrome://browser/content/places/places.xhtml:1
places.js:765:30
    PO__fillDetailsPane chrome://browser/content/places/places.js:765

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9a955a5bcfc2c339788c7878b2fe82d036870ac1&tochange=57b5df18c27c5ddaf61b165bab8b213272836501

Comment 4

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

:emilio, since you are the author of the regressor, bug 1820634, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Marco Bonardo [:mak]]

The selected node in updateDetailsPane doesn't seem to have a .parent, that should be set when the container contents are filled by https://searchfox.org/mozilla-central/rev/c936f47f3a629ae49a4d528d3366bf29f2d4e4a7/toolkit/components/places/nsNavHistoryResult.cpp#739

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1839066 - Improve selection checks in places-tree. r=mak

This doesn't fix the bug but I saw some exceptions related to having a
non-null view with a null selection that this fixes.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1839066 - Make nsTreeBodyFrame::SetView deal with setting the same view. r=mak

This actually fixes the bug. The issue is that we end up with
nsITreeView.setTree(null) then nsITreeView.setTree(<tree>).

This code deals with the first call:

https://searchfox.org/mozilla-central/rev/c936f47f3a629ae49a4d528d3366bf29f2d4e4a7/browser/components/places/content/treeView.js#1651

But the later call doesn't restore the state properly and a bunch of nodes end
up unparented. That might be worth fixing on its own...

This is probably a long/forever-standing bug that was uncovered by different
amount of reflow calls.

Depends on D181379

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1839066 - Simplify some loops in nsNavHistoryResult. r=mak

This is just drive-by.

Depends on D181380

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48bc91bb0060
Improve selection checks in places-tree. r=mak
https://hg.mozilla.org/integration/autoland/rev/2b2f7fab33a1
Make nsTreeBodyFrame::SetView deal with setting the same view. r=mak
https://hg.mozilla.org/integration/autoland/rev/e852daaa49b4
Simplify some loops in nsNavHistoryResult. r=mak

Comment 10

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

The bug is marked as tracked for firefox115 (beta) and tracked for firefox116 (nightly). However, the bug still has low severity.

:cbellini, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/48bc91bb0060
https://hg.mozilla.org/mozilla-central/rev/2b2f7fab33a1
https://hg.mozilla.org/mozilla-central/rev/e852daaa49b4

Comment 12

[Donal Meehan [:dmeehan]]

:emilio is this safe for an uplift request to beta?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9339828 [details]
Bug 1839066 - Make nsTreeBodyFrame::SetView deal with setting the same view. r=mak

Beta/Release Uplift Approval Request

• User impact if declined: comment 0
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple fix to tree code.
• String changes made/needed: none
• Is Android affected?: No

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9339827 [details]
Bug 1839066 - Improve selection checks in places-tree. r=mak

Approved for 115.0b9.

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9339828 [details]
Bug 1839066 - Make nsTreeBodyFrame::SetView deal with setting the same view. r=mak

Approved for 115.0b9.

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f2ae5810826
https://hg.mozilla.org/releases/mozilla-beta/rev/405e1f323fb4

Comment 17

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-06-18 Nightly build on Windows 10.
Verified as fixed on Firefox 115.0b9(build ID: 20230622161221) and Nightly 116.0a1(build ID: 20230623092529) on Windows 10, macOS 12, Ubuntu 22.
Navigating from the Downloads section to the Bookmarks section now works properly.