342182 - text field on "new bookmarks" folder is "too short"

Status: RESOLVED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

text field on "new bookmarks" folder is "too short".  this is a regression, but I'm not sure how recently.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1a3) Gecko/20060620 BonEcho/2.0a3

screen shot coming.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 2

[Phil Ringnalda (:philor)]

I thought it was from something I broke when I first saw it last night: I'm guessing maybe bug 337825, which put microsummary titles in the name field.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Seems likely to be a regression from bug 337825, as I noticed a similar problem while developing the patch for that bug.  I fixed it on Linux, though, and I don't notice it there.  Building on Mac now so I can troubleshoot.

Comment 4

[Phil Ringnalda (:philor)]

*** Bug 342206 has been marked as a duplicate of this bug. ***

Comment 5

[Phil Ringnalda (:philor)]

And as Jo points out, the properties dialog has the same problem.

Comment 6

[Marcia Knous [:marcia]]

Also noticed the same problem when creating a new folder from manage bookmarks.

(In reply to comment #5)
> And as Jo points out, the properties dialog has the same problem.
> 

Comment 7

[Marcia Knous [:marcia]]

nominating. when you try to create a new folder it is nearly impossible to see what you are typing with this bug.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I've built on Mac and can reproduce the problem.  I suspect some native widgetry at play here, but it isn't clear how to get it to behave right.  I'll troubleshoot more today.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: an attempt to fix the problem

This patch makes the editable menulist really look like a textbox if no microsummaries are available.  It seems to fix the problem on Mac, except for a bit of remaining too-shortness when microsummaries are available (but that looks like a general editable menulist problem, not one specific to the microsummary menulist).

On Linux, it reverts my recent changes to make those menulists precisely consistent with adjacent tags, resulting in some one- or two- pixel misalignments.  But we can probably tackle that in a separate bug (indeed, those changes should probably happen for menulists in general, not just the microsummary menulist).

I don't know how it works on Windows, but Seth says he'll build with the patch applied and let me know.

Comment 10

[(not reading, please use [e-mail] instead)]

> I don't know how it works on Windows, but Seth says he'll build with the patch
> applied and let me know.

will apply and test on win32 now.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: screen shot (left, properties before myk's patch, right, after myk's patch)

on my branch build, I didn't see what I saw on the mac, but I did see that the name text field was "too big".  (see the left hand side of my screen shot)

after the patch, things look better (see the right hand side of my screen shot)

has anyone seen the original problem on win32?  If so, perhaps I wasn't seeing it due to my display settings / default font on my laptop (which are large.)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

> screen shot (left, properties before myk's patch, right, after myk's patch)

These are reversed, right?

> has anyone seen the original problem on win32?  If so, perhaps I wasn't seeing
> it due to my display settings / default font on my laptop (which are large.)

Bug 342221 shows some weirdness on win32, although it's not the same as it is on the Mac (or in your screenshot).  The Mac-Win differences are to be expected, since the problem is partly related to the way we mix native and CSS styles for the textbox and menulist elements, and the native styles are of course different on the two platforms.

But I'm not sure why your Windows regression looks different from the one in bug 342221.  Perhaps it's because of your large default display settings.

Given that the patch seems to solve the problem on your Windows build and my Mac build, I think it's the right fix, so I have requested review.

I also filed the three other bugs I've noticed in the course of investigating this regression (and the modifications that caused it):

bug 342389: font size inconsistency between Add Bookmark and Bookmark Properties
bug 342391: editable menulist slightly cuts off text in add bookmark dialog
bug 342392: editable menulist slightly different in size from other widgets

Comment 13

[Simon Bünzli]

Please see bug 342221 for the winstripe part of this bug.

Comment 14

[(not reading, please use [e-mail] instead)]

>> screen shot (left, properties before myk's patch, right, after myk's patch)
>
>These are reversed, right?

no, they are not reversed.  notice how the name field is *too tall* on the left hand side, unlike the other text areas.  whereas after your patch (on the right hand side) it looks correct.

I'll try again after I change my default display / font size.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

> no, they are not reversed.  notice how the name field is *too tall* on the left
> hand side, unlike the other text areas.  whereas after your patch (on the right
> hand side) it looks correct.

Ah, sorry, I misunderstood, as the field on the right side seemed more off because of the background color extending outside the border of the field (although the height of the field is indeed correct, I now see).

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: mac changes only

Simon's fixes for win32 in bug 342221 are better than mine, so removing the win32 changes from this patch, leaving just the Mac changes.  I also incorporated the rule for hiding the popup from the patch in that bug.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Fix checked in to trunk.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 226683 [details] [diff] [review]
patch v2: mac changes only

Notes for drivers considering the approval1.8.1 request:

This patch has just landed on the trunk, but letting it bake for a couple days
won't help test it, because it's specific to the old bookmarks code, which is
disabled on the trunk.

This patch fixes a recent regression in bookmarks dialog appearance and
behavior.  There is little risk to the patch (it's entirely CSS), although it's
possible (but unlikely) for the patch to introduce a new regression in the
process of fixing the current one.

This Mac-specific patch is the equivalent of the Windows-specific patch you approved last week in bug 342221.

Comment 19

[Rob Campbell [:rc] (:robcee)]

Not fixed in 2006062703.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Checked in to the 1.8 branch.  Should show up in tomorrow's nightly.