The bookmarks menu shows less and less as you move the mouse around
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
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.
(That doesn't look right. I'll explore the changes and possibly try mozregression again.)
Comment 1•1 year ago
|
||
Gijs, could bug 1803800 cause this regression?
Assignee | ||
Comment 2•1 year ago
|
||
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...
Updated•1 year ago
|
Comment 4•1 year ago
|
||
Also, the "Other Bookmarks" folder in the toolbutton does not expand left/right, but downward.
Assignee | ||
Comment 5•1 year ago
|
||
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 | ||
Comment 6•1 year ago
|
||
Oh, I meant to link to the offending constructors: https://searchfox.org/mozilla-central/rev/43cb6eca1c3069d46d589d52ab4949257e630d19/browser/base/content/navigator-toolbox.inc.xhtml#629-631,650-652,664-666
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 7•1 year ago
|
||
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.
Assignee | ||
Comment 8•1 year ago
|
||
.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
Comment 9•1 year ago
|
||
I triggered landing since you're traveling, so we get the fix out sooner.
Comment 10•1 year ago
|
||
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
Comment 11•1 year ago
|
||
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.
Assignee | ||
Comment 12•1 year ago
|
||
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.
Comment 13•1 year ago
|
||
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
Comment 14•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb7976183d0c
https://hg.mozilla.org/mozilla-central/rev/39fbab46a5a4
Updated•1 year ago
|
Comment 15•1 year ago
|
||
This issue is Verified as fixed in our latest Nightly and Beta builds 110.0b5.
Description
•