Closed Bug 188254 Opened 19 years ago Closed 19 years ago

Land chimera's form controls on the trunk

Categories

(Core :: Layout: Form Controls, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch something like this (obsolete) — Splinter Review
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).
Attachment #112584 - Flags: review?(pinkerton)
Attachment #112584 - Flags: review?(pinkerton)
Attached patch revised patch (obsolete) — Splinter Review
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
Attachment #112584 - Attachment is obsolete: true
Attachment #112651 - Flags: review?(pinkerton)
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.
Attachment #112651 - Attachment is obsolete: true
Attachment #112651 - Flags: review?(pinkerton)
Attached patch updated patchSplinter Review
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.
Attachment #116484 - Flags: review?(pinkerton)
     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 on attachment 116484 [details] [diff] [review]
updated patch

marking pink's r= on the patch.
Attachment #116484 - Flags: review?(pinkerton) → review+
*** Bug 196462 has been marked as a duplicate of this bug. ***
Attachment #116484 - Flags: superreview?(sfraser)
*** Bug 196551 has been marked as a duplicate of this bug. ***
*** Bug 196580 has been marked as a duplicate of this bug. ***
*** Bug 196723 has been marked as a duplicate of this bug. ***
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 on attachment 116484 [details] [diff] [review]
updated patch

sorry
Attachment #116484 - Flags: superreview?(sfraser) → superreview-
> (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 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.
Attachment #116484 - Flags: superreview- → superreview+
shouldn't this bug be referring to  the 'camino' product, not 'browser'?
Landed.  Any cocoa-based Gecko product should now pick up the Aqua form controls.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I hope you guys are aware of bug 126263... I'm not sure what impact that will
have here
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?
Yea... Verifing aqua form widgets are back in the 2003-03-12-08 Camino NB.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.