Closed Bug 311798 Opened 14 years ago Closed 14 years ago
Feedback line in bookmark menu during drag-and-drop is gone
On firefox of Trunk, the feedback line of bookmark (toolbar's) menu is not rendered when I drag a icon or bookmark menu item. This may be regression of bug 243078. I tested on Win2k, WinXP(with Classic and with Luna). Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051008 Firefox/1.6a1
Please block the final release if bug 243078 is fixed on 1.8 branch. This is important. Because The feedback line is important for bookmark menu. # If we have this, the dragging on bookmark menu is hard to use.
This cannot be reproduced on 1.8 branch.
Unless it's checked in on the branch, don't ? branch-flags.
(In reply to comment #0) > This may be regression of bug 243078. I see this bug on a branch build with the patch for bug 243078 applied, so it's almost certainly caused by that.
Summary: Feedback line of bookmark menu is gone → Feedback line in bookmark menu during drag-and-drop is gone
This was indeed caused by bug 243078. The problem is not with the logic, but the CSS (specifically, that in menu.css and browser.css). The problem is twofold: 1. [menus.css] Now that menus and menuitems by default are set to "display:menuitem" (so that native theming works), borders drawn around them simply have no effect. However, the best way to do this might be with an outline anyway (I can't test it here, so I have no idea if it would work; see comment 2). 2. [brower.css] Even when setting -moz-appearance: none on .bookmark-item[dragover-left="true"] (and other similar CSS properties), silver's patch also took the transparent border out of the CSS. Therefore, one has to set a border on at least one side of it when these properties are true, resulting in the hightlighted menu item's jumping a little (hence the notion that outline might be superior). Another possible solution is to still use a border, but also use -moz-box-sizing:border-box for these cases. I don't, again, know if this would work properly, because all I have here is Portable Firefox 1.0.6, but I'll try to whip up a better patch when I get home (I already have a decent one). Also, does this dependency really make any sense? If anything, bug 243078 depends on this.
Outline is far superior, assuming Gecko doesn't have any interesting bugs. :) Trying a build of FF 1.5 branch now.
Well, it would be if the damn thing allowed single sides to be done! *sigh* Ok, new idea coming...
Argh, XUL sucks lots. Anyway, I got a nice outline-single-edge thing working for menu items. Got to work out which bits of the branch changes are relevant though.
Hm. I actually came up with a seperate (hackish) temporary fix involving setting the borders on the generated xul:labels that belong to menuitems. While this is obviously not ideal, it (A) appears to work, (B) doesn't jump, and (C) most importantly, doesn't introduce any more changes that could potentially break something (unless I'm misunderstanding the fix you created). While I agree in principle that there should be a way to set partial outlines on menuitems, I think that patch should wait for trunk as long as there's a servicable substitute. I'll upload it in patch format as soon as I can.
I tried that (among other combinations), and it looked totally wrong. :) The patch I'm waiting for CVS to spew out actually makes it look like it did before, but without making the menus any more spaced out than they should be.
I don't like doing this, but it's the only way to restore the correct appearance now that browser.css's assumptions are invalid.
Note: this is a branch patch because the original stuff is branch, and I don't intend to make a trunk patch myself - this change will be included in patch 7 on bug 243078 when reviewed. Also note: I really dislike doing what I did in the patch, but thanks to the mess drivers have put us in, this is the best you'll get.
I've got another patch that works on trunk, is CSS-based and only twelve lines. The main difference is that, in my patch, the feedback line only extends as far as the label text does. I've tested this on menus in the bookmarks toolbar, in the bookmark menus, and in various other places, and it works fine. However, Silver's patch (though it's much bigger) maintains the old appearance, so if it's possible for his patch to land rather than mine I'm fine with that. I should soon also have a version of his patch for trunk, which I will upload if his patch gets r+ so it can be more thoroughly tested.
Er, in testing this patch in preparation for making a trunk patch, I noticed that this fails when you drag a bookmark to a folder in the bookmarks toolbar (which my patch does not). I don't remember how I got around this, and your patch is rather large and complex (and you're away), but in the meantime I'll upload my patch just so there's something that DOES work up here. It would suck if there were a regression from a regression, wouldn't it?
Attachment #199136 - Flags: superreview?(bryner) → superreview+
Comment on attachment 199136 [details] [diff] [review] Safe CSS-only patch (trunk) You shouldn't use descendant selectors or mix tag and class names - stick to the child selector and use classes throughout i.e. .bookmark-item[dragover-top="true"] > .menu-text
Attachment #199136 - Flags: review?(mconnor) → review+
Silver's latest patch applies on trunk without any work from me. I think we should probably take it over mine, as his is visually correct. When the whole situation with bug 243078 is over with, though, I'd like to work on a proper fix for this bug.
So, this is the patch that causes all these regressions with the native theme and the non-native themes. This patch was created to solve a minor regression, but creates a series of more serious regressions (big favicons, very wide bookmark menus, not-right aligned submenu arrows and accelerators, and more to be discovered?). This shows why it is dangerous to submit this late in the release schedule such a big patch, with may be too hasty patches for its regressions Note, the last patch seems to be allready committed to both trunk and branch without any reviews... 'Work on a proper fix when this is over': means that the 'stack > .menu-content-inner' redirection will be changed again?
*** Bug 312445 has been marked as a duplicate of this bug. ***
Not a problem on branch w/ patch 243078 backed out. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051015 Firefox/1.4.1 ID:2005101504
*** Bug 313757 has been marked as a duplicate of this bug. ***
Comment on attachment 199150 [details] [diff] [review] Fix the bookmark toolbar folders clearing this, since bookmarks dnd code was/is being rewritten for Places.
This is not reproduced current trunk build, probably, bug 313388 fixes this bug. -> FIXED
Status: NEW → RESOLVED
Closed: 14 years ago
Depends on: 313388
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.