188254 - Land chimera's form controls on the trunk

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Brian Ryner (not reading)]

We should land Chimera's form control implementation onto the trunk.  It would
be nice to get the aqua form controls into our Carbon builds, but I think doing
it for Cocoa builds would be a good start.  If we just worry about Ccooa, we
don't have to worry about coming up with a carbon equivalent of the native popup
menu that's used for selects.

Comment 1

[Brian Ryner (not reading)]

Attachment: something like this

This merges the chimera form controls (plus some improvements, such as using
the "small" sized controls) to the trunk for Cocoa builds.  I went ahead and
added a cross-platform base class for nsNativeTheme to reduce code duplication
(I'll convert windows and gtk to use this base class later).

Comment 2

[Brian Ryner (not reading)]

Attachment: revised patch

changes:
- cleaned up some unecessary stuff in nsNativeTheme.h/.cpp
- changed to private inheritence of nsNativeTheme since it's just an
implementation aid.
- added better comments in nsComboboxControlFrame about what's going on
- fixed licenses on new css files
- removed a css rule that tried to set the width of the inner button frame of
the combobox - it's unnecessary since the combobx frame hardcodes the button
frame width to the width of a scrollbar

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 112651 [details] [diff] [review]
revised patch

Don't review this yet.	I need to clean it up a bit, and I'm going to go ahead
and use "small" controls for buttons and menulists as well.

Comment 4

[Brian Ryner (not reading)]

Attachment: updated patch

This patch implements "small" versions of the controls for everything except
menulists (I couldn't get the popup menu to use the smaller font, so I decided
to leave it be.  We may have to make this fully native).  All of the fonts
should now match the HIG.  Also, the CSS system fonts have been changed to
reflect reality on OS X.

Comment 5

[Mike Pinkerton (not reading bugmail)]

     case NS_THEME_TEXTFIELD:
     {
-      SInt32 shadow = 0, frameOutset = 0;
-      ::GetThemeMetric(kThemeMetricEditTextWhitespace, &shadow);
-      ::GetThemeMetric(kThemeMetricEditTextFrameOutset, &frameOutset);
-      aResult->SizeTo(shadow + frameOutset, shadow + frameOutset, shadow +
frameOutset,
-                        shadow + frameOutset);
+      aResult->SizeTo(2, 2, 1, 1);
       break;
     }

why?

r=pink otherwise. can't wait for this to land!

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 116484 [details] [diff] [review]
updated patch

marking pink's r= on the patch.

Comment 7

[Simon Fraser [no longer active]]

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

Comment 8

[Chris Petersen]

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

Comment 9

[Chris Petersen]

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

Comment 10

[Chris Casciano]

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

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 116484 [details] [diff] [review]
updated patch

+#ifndef MOZ_WIDGET_COCOA
+  // Only support HTML widgets for Cocoa

Should this be a pref instead? Or an nsILookAndFeel thing?

 CPPSRCS = \
	nsRenderingContextImpl.cpp \
	gfxImageFrame.cpp \
+	nsNativeTheme.cpp \

This might be shared code, but we really want the code to be in the gfx
component DLL, don't we? (this is bug 193435).

+PRInt32
+nsNativeTheme::GetContentState(nsIFrame* aFrame)

A comment to say what "content state" is would be nice.

nsNativeTheme contains a lot of layout/content-related code. It doesn't feel
right for this to be in gfx, and the added dependencies:

+		  content \
+		  layout \
+		  necko \
+		  dom \
+		  locale \

reflect this. Is there a better home for nsNativeTheme? Or do we need to
refactor, so that  the control drawing code goes into gfx (or widget?) but the
rest stays in layout/content.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 116484 [details] [diff] [review]
updated patch

sorry

Comment 13

[Brian Ryner (not reading)]

> (From update of attachment 116484 [details] [diff] [review])
> +#ifndef MOZ_WIDGET_COCOA
> +  // Only support HTML widgets for Cocoa
>
> Should this be a pref instead? Or an nsILookAndFeel thing?

It wouldn't make sense to have it in nsILookAndFeel, because that's a
compiled-time value, and we know at present that we don't have a native popup
menu implementation Carbon so it's highly unlikely someone would want to use the
HTML form controls.  It could be a pref in case an embedder wants to implement a
click listener to build a native popup menu, but if that were the case I think
we should just add the necessary code to gecko.  I don't know of any Carbon
embedders who are clamoring for Aqua form controls.

> This might be shared code, but we really want the code to be in the gfx
> component DLL, don't we? (this is bug 193435).

gfx/src/shared _is_ in the component dll.  Look at EXTRA_DSO_LDOPTS in
gfx/src/mac/Makefile.in.

> A comment to say what "content state" is would be nice.

"The state of the content"?  It seems pretty self-explanatory.

> Is there a better home for nsNativeTheme? Or do we need to
> refactor, so that  the control drawing code goes into gfx (or widget?) but the
> rest stays in layout/content.

These aren't new dependencies, except for 'dom'.  They are all current
dependencies of mac gfx.

We coud refactor so that gfx is passed some struct containing the state instead
of the frame.   But this would require ripping up the native theme code on 3
platforms, and I think the benefit from doing so is dubious.  In any case, I'd
say that should be a separate bug.

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 116484 [details] [diff] [review]
updated patch

> "The state of the content"?  It seems pretty self-explanatory.

What kind of state? There's no way I can tell from reading the code.

Fair enough on the other comments, but I would like to see the long-term
removal of gfx dependencies on higher-level libraries.

Comment 15

[Daniel Andrews]

shouldn't this bug be referring to  the 'camino' product, not 'browser'?

Comment 16

[Brian Ryner (not reading)]

Landed.  Any cocoa-based Gecko product should now pick up the Aqua form controls.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I hope you guys are aware of bug 126263... I'm not sure what impact that will
have here

Comment 18

[Calum Robinson]

Could a "margin: 2px;" be added for buttons? If you look at the 2 buttons on the
Google front page, they are right next to each other (and right beside the text
field). Without the margin they look all bunched up, but with a 2px margin they
look alot better, especially with these new small controls (which look great). 

Or are the attributes like this not to be messed with for compatibility?

Comment 19

[Chris Petersen]

Yea... Verifing aqua form widgets are back in the 2003-03-12-08 Camino NB.