Closed Bug 311798 Opened 14 years ago Closed 14 years ago

Feedback line in bookmark menu during drag-and-drop is gone

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: pythonesque+bugzilla)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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.
Flags: blocking1.8rc1?
This cannot be reproduced on 1.8 branch.
Unless it's checked in on the branch, don't ? branch-flags.
Flags: blocking1.8rc1?
(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.
Assignee: nobody → pythonesque+bugzilla
Blocks: 243078
No longer depends on: 243078
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.
Attachment #199105 - Flags: review?(mconnor)
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)
Attachment #199136 - Flags: review?(mconnor)
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 #199105 - Attachment is obsolete: true
Attachment #199150 - Flags: review?(mconnor)
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.
Attachment #199105 - Flags: review?(mconnor)
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.
Attachment #199150 - Flags: review?(mconnor)
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.