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

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Martijn Wargers (dead)]

See screenshots I'm going attach.
This changed with the fix for bug 326123.

Comment 1

[Martijn Wargers (dead)]

Attachment: Screenshot IE6, on win2000 theme

Comment 2

[Martijn Wargers (dead)]

Attachment: Screenshot, current trunk build in win2000 theme

Comment 3

[Martijn Wargers (dead)]

Attachment: Screenshot in current trunk build with WinXP theme

Comment 4

[Martijn Wargers (dead)]

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 

Comment 5

[Uri Bernstein]

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

[Frank Wein [:mcsmurf]]

Cannot one use macros like #if _WIN32_WINNT>=0x0501 ... #else ... #endif for such a case?

Comment 7

[Uri Bernstein]

(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.

Comment 8

[Martijn Wargers (dead)]

Attachment: 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.

Comment 9

[Martijn Wargers (dead)]

(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

Comment 10

[Uri Bernstein]

Thanks, Martijn, and sorry again for breaking this and not being able to fix it.

Comment 11

[[e-mail]]

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.

Comment 12

[Martijn Wargers (dead)]

Attachment: 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.

Comment 13

[[e-mail]]

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.

Comment 14

[[e-mail]]

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.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch3

So like this, then?

Comment 16

[[e-mail]]

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.

Comment 17

[Martijn Wargers (dead)]

Attachment: patch3, revised

(In reply to comment #16)
> When I said follow the example, I really meant ++; r=me with that fixed.
Oops.

Comment 18

[Martijn Wargers (dead)]

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?

Comment 19

[[e-mail]]

(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.

Comment 20

[Martijn Wargers (dead)]

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

Ok, asking rbs.

Comment 21

[rbs]

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

sr=rbs

Comment 22

[Martijn Wargers (dead)]

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

Comment 23

[Simon Paquet [:sipaq]]

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

This patch was already checked in.