Closed Bug 1805678 Opened 1 year ago Closed 1 year ago

The bookmarks menu shows less and less as you move the mouse around

Categories

(Firefox :: Bookmarks & History, defect)

defect

Tracking

()

VERIFIED FIXED
110 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox108 --- unaffected
firefox109 --- unaffected
firefox110 + verified

People

(Reporter: bj, Assigned: Gijs)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(2 files)

With today's Nightly add the Bookmarks menu to the toolbar, open the menu, and move the mouse around over it. The more you hover over menus the more parts of the menu vanish.

Tested with a new profile.

Mozregression: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6002e93434a2e65a900fbceec1b88322a74ef94e&tochange=06c21393dcc748b7ed2006c1a550919518630e95

(That doesn't look right. I'll explore the changes and possibly try mozregression again.)

Gijs, could bug 1803800 cause this regression?

Flags: needinfo?(gijskruitbosch+bugs)
Regressed by: 1803800
No longer regressed by: 1803800
Regressed by: 1803800

It's certainly possible. It refactored a bunch of places/bookmarks UI code. I can reproduce the issue but there are no JS errors at all, which makes it a little tricky to understand what is broken. I'll keep looking...

Also, the "Other Bookmarks" folder in the toolbutton does not expand left/right, but downward.

OK, I think I've worked out what the issue is here.

It turns out that in the bookmarks menu button (and only there - we somehow don't need this in the main bookmarks menu? It seems this was done to make sure styling matched, in bug 969592) we reuse the options object for the "parent" menu's view (so that of the main bookmarks menu button's list of bookmarks).

The change in bug 1803800 assumed that the options object passed into the view was unique to that view, and added the root/view element reference to it only if this was not supplied as part of the options object. This means that when a subview opens, we reuse the root/view element reference used for the bookmarks menu's main view, and that's why then the items from the subfolder get transplanted into the main folder and things just generally go haywire from there.

This only happens for the "builtin" items that produce subfolders - if you have subfolders you created yourself, they don't cause the same issue.

I already have "a" fix, but I'm trying to work out what is actually the "right" fix. It would also be good to have some automated tests - it's surprising nothing caught this in automation.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Summary: The bookmarks menu dhows less and less as you move the mouse around → The bookmarks menu shows less and less as you move the mouse around

Only the bookmarks menu button is doing this peculiar re-using of
options.

I tried various fixes: keeping the inheritance but replacing only the
rootElt/viewElt (and using Object.assign to clone the options so modifications
don't 'transmit' to the ancestor menus) but it was messy and net code increase
for a pretty crufty UI surface. I also wasn't sure if that would end up
negatively influencing (now or in the future) e.g. menus shown from the main
bookmarks toolbar view (which would show up as a 'parent' view for the menus
we open from there), and thought that reusing the options was likely to cause
further confusion in future as well, should we add more, uh, options, to them.

So in the end I stuck with Keeping It Simple - I just repeat the one-off
repeating entry class. I'm not even sure how needed this is - in particular,
I wonder why we don't need it for other submenus for 'real' bookmark folders!
But I didn't investigate this too much. If you're sure that we can get rid of
some more of it, happy to do that in a followup.

Note that the addition of _init() was not ultimmately needed to fix this bug
(I think), but it brought the initialization sequence and when we set
_placesView more in line with what we did before and what I already did for
PlacesToolbar, so that seemed like a good thing to avoid further/other issues.

.footer is no longer used on extraClasses, so I got rid of that.

I simplified the .entry bits because they were unnecessarily
verbose. Honestly I think we may be able to remove the getter/setter
for options entirely, but I got bored of trying to modernize things...

PlacesPanelMenuView was introduced in bug 963095 and then replaced
as part of Photon (with PlacesPanelview) and then I removed the only
callsite (that was already behind a pref) in bug 1354117, so that was
just dead code, AFAICT, so I removed it.

Depends on D164756

Blocks: 1805889

I triggered landing since you're traveling, so we get the fix out sooner.

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/82e2726e9aec
ensure bookmark menu button submenus don't reuse root/view element references, r=mak
https://hg.mozilla.org/integration/autoland/rev/110abf97facf
clean up cruft in browserPlacesViews, r=mak

Backed out for causing mochitests failures in browser_history_recently_closed_middleclick.js.

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_history_recently_closed_middleclick.js | uncaught exception - Error: No DOM node set for aPlacesNode.
Flags: needinfo?(gijskruitbosch+bugs)

Apologies for the delay here. I was traveling pretty much all of today, and unfortunately the test errors here did not and do not reproduce on the mac laptop I'm carrying and on which I wrote the patches here - because the test failure is in this bit of the test which just skips that part of the test entirely on macOS as we cannot programmatically open the macOS menubar.

Thankfully Marco figured out the issue. The problem now is that I can't verify that the patch works locally because I'm away from home and my Windows/Linux machines/VMs. I also don't really want to get backed out for a second separate thing tomorrow after sleep, as I don't have much time then either. So I pushed to try: https://treeherder.mozilla.org/jobs?repo=try&revision=4d6f6475c154f214113dfe455380bc1db80be23e

Assuming all is fine when I look at that tomorrow morning I'll reland the updated patch.

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eb7976183d0c
ensure bookmark menu button submenus don't reuse root/view element references, r=mak
https://hg.mozilla.org/integration/autoland/rev/39fbab46a5a4
clean up cruft in browserPlacesViews, r=mak
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
Flags: qe-verify+

This issue is Verified as fixed in our latest Nightly and Beta builds 110.0b5.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: