Caret 1px too far to the right in empty inputs/textarea's in win2000 theme

RESOLVED FIXED

Status

SeaMonkey
Themes
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Martijn Wargers (dead), Unassigned)

Tracking

({regression})

Trunk
x86
Windows XP
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
See screenshots I'm going attach.
This changed with the fix for bug 326123.
(Reporter)

Comment 1

12 years ago
Created attachment 211611 [details]
Screenshot IE6, on win2000 theme
(Reporter)

Comment 2

12 years ago
Created attachment 211612 [details]
Screenshot, current trunk build in win2000 theme
(Reporter)

Comment 3

12 years ago
Created attachment 211614 [details]
Screenshot in current trunk build with WinXP theme
(Reporter)

Comment 4

12 years ago
So current trunk build in WinXP theme looks all right, comparable to IE6, but current trunk build in Win2000 theme, it looks like the caret is 1px too far too the right.

Uri thinks this might be a cause for this:
http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsNativeThemeWin.cpp#958 
The solution might be to apply this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsNativeThemeWin.cpp&rev=3.53#958
only in the XP/Luna case.

Comment 6

12 years ago
Cannot one use macros like #if _WIN32_WINNT>=0x0501 ... #else ... #endif for such a case?
(In reply to comment #6)
> Cannot one use macros like #if _WIN32_WINNT>=0x0501 ... #else ... #endif for
> such a case?
> 

Perhaps. My problem is that I can't work on this since I don't have access to a Windows machine.
(Reporter)

Comment 8

12 years ago
Created attachment 214895 [details] [diff] [review]
patch

Ok, this fixes it for me.
Screenshots here for comparison before/after patch and IE6, using classic/winxp themes:
http://wargers.org/mozilla/bug326926/326926_caret_comparison.htm
I also added some extra vertical padding for winxp inputs/textarea's because they are missing it (see comparison page).

Classic theme also needs some extra vertical padding, but when I do that I get a nasty side-effect where the anonymous div inside the input/textarea doesn't fit anymore.
I think it could also be fixed, maybe, but then I would also need to change stuff in forms.css and I don't know how that would work on other platforms.
Attachment #214895 - Flags: review?(neil)
(Reporter)

Comment 9

12 years ago
(In reply to comment #8)
> Classic theme also needs some extra vertical padding,
It needs it only for textarea, when you look at the comparison page, that's because inputs already have some extra vertical padding in forms.css:
http://lxr.mozilla.org/seamonkey/source/layout/style/forms.css#82
Thanks, Martijn, and sorry again for breaking this and not being able to fix it.

Comment 11

12 years ago
Comment on attachment 214895 [details] [diff] [review]
patch

>   if (aFrame && aWidgetType == NS_THEME_TEXTFIELD) {
>     nsIContent* content = aFrame->GetContent();
>     if (content && content->IsContentOfType(nsIContent::eHTML)) {
>       // We need to pad textfields by 1 pixel, since the caret will draw
>       // flush against the edge by default if we don't.
>       aResult->left++;
>       aResult->right++;
>+      aResult->top = aResult->bottom = 2;
Did you try this with XUL textfields, only it looks as if it might belong before the nsIContent* line (compare with ClassicGetWidgetBorder).

>         // HTML text-fields need extra padding
>-        (*aResult).left = (*aResult).right = 3;
>-      else
>         (*aResult).left = (*aResult).right = 2;
>+      else
>+        (*aResult).left = (*aResult).right = 1;
This change looks reasonable.
(Reporter)

Comment 12

12 years ago
Created attachment 214903 [details] [diff] [review]
patch2

This patch is dealing only with the part where this bug is about. Perhaps, that is preferred.

(In reply to comment #11)
> Did you try this with XUL textfields, only it looks as if it might belong
> before the nsIContent* line (compare with ClassicGetWidgetBorder).
That would make the url bar 2px higher in winxp theme, which isn't happening with the original patch.
Attachment #214903 - Flags: review?(neil)

Comment 13

12 years ago
Comment on attachment 214903 [details] [diff] [review]
patch2

On second thoughts, this bit is wrong, because the XUL textbox border should always be 2px; however padding changes in the theme may be necessary.
Attachment #214903 - Flags: review?(neil) → review-

Comment 14

12 years ago
Comment on attachment 214895 [details] [diff] [review]
patch

>       // We need to pad textfields by 1 pixel, since the caret will draw
>       // flush against the edge by default if we don't.
>       aResult->left++;
>       aResult->right++;
>+      aResult->top = aResult->bottom = 2;
Don't hardcode 2, follow the example given to you.
Attachment #214895 - Flags: review?(neil) → review-
(Reporter)

Comment 15

12 years ago
Created attachment 214918 [details] [diff] [review]
patch3

So like this, then?
Attachment #214895 - Attachment is obsolete: true
Attachment #214903 - Attachment is obsolete: true
(Reporter)

Updated

12 years ago
Attachment #214918 - Flags: review?(neil)

Comment 16

12 years ago
Comment on attachment 214918 [details] [diff] [review]
patch3

>       aResult->left++;
>       aResult->right++;
>+      aResult->top += 1;
>+      aResult->bottom += 1;
When I said follow the example, I really meant ++; r=me with that fixed. Oh, and file style appears to be top, left, bottom, right so if you could reorder the lines too that would be neat, thanks.
Attachment #214918 - Flags: review?(neil) → review+
(Reporter)

Comment 17

12 years ago
Created attachment 215066 [details] [diff] [review]
patch3, revised

(In reply to comment #16)
> When I said follow the example, I really meant ++; r=me with that fixed.
Oops.
Attachment #214918 - Attachment is obsolete: true
(Reporter)

Comment 18

12 years ago
Comment on attachment 215066 [details] [diff] [review]
patch3, revised

Neil, sorry, I also need sr+.
Can I ask you, or should I ask someone else?
Attachment #215066 - Flags: review?(neil)
(Reporter)

Updated

12 years ago
Attachment #215066 - Flags: superreview?(neil)
Attachment #215066 - Flags: review?(neil)
Attachment #215066 - Flags: review?

Comment 19

12 years ago
(In reply to comment #18)
>Neil, sorry, I also need sr+.
>Can I ask you, or should I ask someone else?
You could ask ere to review then I can grant sr, or you can ask rbs for sr.
(Reporter)

Comment 20

12 years ago
Comment on attachment 215066 [details] [diff] [review]
patch3, revised

Ok, asking rbs.
Attachment #215066 - Flags: superreview?(neil) → superreview?(rbs)

Comment 21

12 years ago
Comment on attachment 215066 [details] [diff] [review]
patch3, revised

sr=rbs

Updated

12 years ago
Attachment #215066 - Flags: superreview?(rbs) → superreview+
(Reporter)

Comment 22

12 years ago
Checking in widget/src/windows/nsNativeThemeWin.cpp;
/cvsroot/mozilla/widget/src/windows/nsNativeThemeWin.cpp,v  <--  nsNativeThemeWi
n.cpp
new revision: 3.55; previous revision: 3.54
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Comment on attachment 215066 [details] [diff] [review]
patch3, revised

This patch was already checked in.
Attachment #215066 - Flags: review?
(Assignee)

Updated

9 years ago
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.