Closed Bug 433055 Opened 13 years ago Closed 13 years ago

Text fields in bookmark properties should have the same height

Categories

(Firefox :: Theme, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: ehsan.akhgari, Assigned: dao)

References

()

Details

(Keywords: polish, Whiteboard: [polish-easy] [polish-visual][polish-p2])

Attachments

(1 file)

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.
Whiteboard: [polish-easy] [polish-visual]
mh, i suppose the different height is due to the name picker used for microsummaries
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.
Assignee: nobody → dao
This is ugly. IMHO you should use a real textbox when there are no microsummaries.
Attachment #348483 - Flags: review?(mak77)
Attachment #348483 - Attachment description: places → places theme patch
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
(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.
(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 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.
Attachment #348483 - Flags: review?(mak77) → review?(dietrich)
(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.
(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.
Attachment #348483 - Flags: review?(dietrich) → review+
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.
(In reply to comment #10)
> r=me w/ comment #9 resolved.

by that you mean removing the redundant margin declaration?
(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.
Attachment #348483 - Flags: approval1.9.1b2?
Attachment #348483 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 348483 [details] [diff] [review]
places theme patch

a191b2=beltzner
http://hg.mozilla.org/mozilla-central/rev/71a967ce2879
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
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.
Status: RESOLVED → VERIFIED
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.
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][polish-p2]
You need to log in before you can comment on or make changes to this bug.