433055 - Text fields in bookmark properties should have the same height

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[(no longer active)]

For a sample of the problem, please check out <https://bugzilla.mozilla.org/attachment.cgi?id=320156>.  All single like text boxes in those dialogs should have the same height.

Comment 1

[Marco Bonardo [:mak]]

mh, i suppose the different height is due to the name picker used for microsummaries

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

There are a lot of other instances of text fields being different heights on Vista, which cropped up when we switched to the native appearance. cc'ing vlad in case he knows more.

Comment 3

[Dão Gottwald [:dao]]

Attachment: places theme patch

This is ugly. IMHO you should use a real textbox when there are no microsummaries.

Comment 4

[Marco Bonardo [:mak]]

this is cool, but there is still a jumpy behaviour on the namepicker when we HAVE a microsummary, the text is missing padding on the left and has too much padding on the top... so if you move in the Library from an item that has microsummary to an item that does not have one, the namepicker jumps.

You can test with one of these sites: https://wiki.mozilla.org/Microsummaries/Sites

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> this is cool, but there is still a jumpy behaviour on the namepicker when we
> HAVE a microsummary, the text is missing padding on the left and has too much
> padding on the top...

This might be harder to fix. I suggest we file a new bug under Toolkit:Theme, as it affects all editable menulists.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #3)
> This is ugly. IMHO you should use a real textbox when there are no
> microsummaries.

Btw, faking the textbox also means that you're lacking the fix for bug 288194.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 348483 [details] [diff] [review]
places theme patch

>diff --git a/browser/themes/winstripe/browser/places/editBookmarkOverlay.css b/browser/themes/winstripe/browser/places/editBookmarkOverlay.css
>--- a/browser/themes/winstripe/browser/places/editBookmarkOverlay.css
>+++ b/browser/themes/winstripe/browser/places/editBookmarkOverlay.css
>@@ -79,38 +79,33 @@
> 
> /**** name picker ****/
> 
> /* Make the microsummary picker look like a regular textbox instead of 
>  * an editable menulist when no microsummaries are available.
>  */
> #editBMPanel_namePicker[droppable="false"] {
>   /* These rules come from the textbox element in textbox.css. */
>-
>-  /* Normal editable menulists set this to "none". */
>-  -moz-appearance: textfield;
>+  -moz-appearance: textfield; /* Normal editable menulists set this to "menulist". */
>   cursor: text;
>-
>+  margin: 2px 4px;

is this margin really needed?


for menulists i see in toolkit menulist.css

127 /* ::::: editable menulists ::::: */
128 
129 .menulist-editable-box {
130   padding-top: 3px;
131   padding-bottom: 3px;
132   -moz-padding-start: 2px;
133   -moz-padding-end: 0px;
134 }
so yes i agree with your suggest to file a toolkit theme bug.

over to dietrich.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> >+  margin: 2px 4px;
> 
> is this margin really needed?

It's the margin that real textboxes have as well per textbox.css.

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> (In reply to comment #7)
> > >+  margin: 2px 4px;
> > 
> > is this margin really needed?
> 
> It's the margin that real textboxes have as well per textbox.css.

yes but from menulist.css:
menulist {
49   -moz-appearance: menulist;
50   margin: 2px 4px;

it's already defined...

hwv i can see the point in having a real textbox there, that could be the right fix, maybe now or for a followup, dietrich can elaborate over that.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 348483 [details] [diff] [review]
places theme patch

using the proper widget would be better, but this is an acceptable interim solution. r=me w/ comment #9 resolved.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> r=me w/ comment #9 resolved.

by that you mean removing the redundant margin declaration?

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #11)
> (In reply to comment #10)
> > r=me w/ comment #9 resolved.
> 
> by that you mean removing the redundant margin declaration?

i guess so since he talked about interim solution. We should also file a Bookmarks&History bug to do the correct field replacement in future and a toolkit theme bug for menulists padding.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348483 [details] [diff] [review]
places theme patch

a191b2=beltzner

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/71a967ce2879

Comment 15

[Dão Gottwald [:dao]]

bug 465718 and bug 465719

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre ID:20081120033739

Now the drop down is the only element which is too high. Mostly it will depend on the shown icon on the left side. Could we shrink this too, so we have the same height as the buttons at the right side? If there is no bug about it, I could file a new one - at least if it can be fixed.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Considering bookmarking a secondary UI, and this was obviously wrong.