397717 - [10.5] Advanced Preferences Pane tabs render incorrectly

Status: RESOLVED WORKSFORME

(Core Graveyard :: Widget: Mac, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue

Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8pre) Gecko/2007092505 BonEcho/2.0.0.8pre.

STR:
1. Go to the advanced preference pane
2. Observe the attached screenshot.

Comment 1

[Josh Aas]

mozilla/gfx/src/mac/nsNativeThemeMac.cpp

In Mac OS X 10.5, Appearance Manager draws new-style tabs by default. There is no way to change the control version back to the old-style tabs that it draws on 10.4. If we rewrote the tab-rendering code to use HITheme we could specify the old tab type, but HITheme is 10.3+ and we have to support 10.2.

Apple should not have changed this Appearance Manager behavior, yuck.

Here are our options:
1. Don't fix this bug (bad)
2. Rewrite our code to use new-style tabs (very hard, very risky). This is hard because new-style tabs have certain layout requirements that complicate things, like the fact that tabs draw over the selected panel by about 10-15 pixels. Also 10.2 doesn't support new-style tabs (iirc) so we'd have to special case that all over.
3. Use weak linking an 10.5 special casing to render tabs with HITheme on 10.5

I guess we're going with option 3. Lets hope HITheme plays nice with Quickdraw.

Comment 2

[Marcia Knous [:marcia]]

I see this issue on Thunderbird trunk as well. There are a number of preferences panels that don't render correctly.

Comment 3

[Marcia Knous [:marcia]]

Attachment: Screenshot of Page Info Dialog

I just noticed this happens in the Page Info dialog as well in Firefox 2.0.0.8.

Comment 5

[Colin Barrett [:cbarrett]]

Looks like hurdle #1 is going to be the fact that HIThemeDrawTab wants a CGContext, but we're drawing with QuickDraw. Fantastic.

Comment 6

[Colin Barrett [:cbarrett]]

For future reference,

http://developer.apple.com/documentation/Carbon/Reference/QuickDraw_Ref/Reference/reference.html#//apple_ref/c/func/QDBeginCGContext

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

IIRC, mento had some interesting code in the bug where we toyed with doing 10.3-style tabs that also addressed that hurdle.  I think.  (But then my memory's not what it used to be anymore, apparently.)

Comment 8

[Colin Barrett [:cbarrett]]

Does QA have a machine I can test this on 10.2 with?

Comment 9

[Marcia Knous [:marcia]]

(In reply to comment #8)
> Does QA have a machine I can test this on 10.2 with?
> 

Yes, we have a PPC machine in the lab which has all flavors.

Comment 10

[Mark Mentovai]

See bug 231313.  I got this working in the past (but the tabs had to be above the pane they control, see that bug) but the system left blue turds behind in the separators between tabs.  I couldn't get rid of the turds, so the patch didn't go anywhere.

(It was hard to find because it's assigned to Josh, even though the experimental code was mine.)

Apple shouldn't have changed this without accounting for the ugly lack of end caps.

This will be a little annoying to fix with weak linking if you're still building ppc with the 10.2.8.sdk.  It should be a breeze if you can build ppc with the 10.3.9.sdk (while still targeting 10.2), which is a definite possibility, but that might be changing too much for a stable branch.

Comment 11

[Colin Barrett [:cbarrett]]

Attachment: fix 1.0

I load the symbol for HIThemeDrawTab at runtime, and this code will only run on 10.5. Works fine on my 10.5 machine (running the outdated 9a528d, unfortunately).

Comment 12

[Colin Barrett [:cbarrett]]

Attachment: fix 1.0.1

Fix a whitespace problem, I hope.

Comment 13

[Colin Barrett [:cbarrett]]

I have a couple more whitespace fixes, will fix on checkin (or in the next round of this patch if there are review comments)

Comment 14

[Josh Aas]

Comment on attachment 285538 [details] [diff] [review]
fix 1.0.1

Can you put the start and end CGContext code into separate functions like in the NPAPI example code? If we ever need that again it'll be nice, and it is easy code to separate out.

Comment 15

[Josh Aas]

Comment on attachment 285538 [details] [diff] [review]
fix 1.0.1

+      // Get a CGContext from our QuickDraw port fo HITheme to draw into.

"fo" -> "for"

+      HIThemeTabDrawInfo tdi;

This makes it so you can only compile against an SDK that has HITheme. I think we have tinderboxes compiling against the 10.2 SDK. Did you have a reason why this would be OK? Otherwise I think you need to copy the definition of HIThemeTabDrawInfo into our source.

Comment 16

[Colin Barrett [:cbarrett]]

(In reply to comment #15)
> (From update of attachment 285538 [details] [diff] [review])
> +      // Get a CGContext from our QuickDraw port fo HITheme to draw into.
> 
> "fo" -> "for"

Fixed
 
> +      HIThemeTabDrawInfo tdi;
> 
> This makes it so you can only compile against an SDK that has HITheme. I think
> we have tinderboxes compiling against the 10.2 SDK. Did you have a reason why
> this would be OK? Otherwise I think you need to copy the definition of
> HIThemeTabDrawInfo into our source.

Nope, I just overlooked it. Any suggestions on where to put that definition? Just at the top of this file? When can be back in the MV office I plan to test compile whatever patch I have at that point on a 10.2 machine to make sure it builds and produces a working build.

Comment 17

[Colin Barrett [:cbarrett]]

(In reply to comment #14)
> (From update of attachment 285538 [details] [diff] [review])
> Can you put the start and end CGContext code into separate functions like in
> the NPAPI example code? If we ever need that again it'll be nice, and it is
> easy code to separate out.

Sure. should I just have them in this same file?

Comment 18

[Mark Mentovai]

We can use the new API on 10.3 and up, can't we?

If so, we can cut code size for the Intel build a little bit if we use some preprocessor magic to eliminate the old codepath on that platform.  You can use the code referenced in comment 10 as a guide.

Comment 19

[Josh Aas]

To clarify Mark's comment, he is talking about the fact that we don't need to include the Appearance Manager code path on Intel because all Intel machines have access to HITheme.

Comment 20

[Colin Barrett [:cbarrett]]

(In reply to comment #18)
> We can use the new API on 10.3 and up, can't we?
> 
> If so, we can cut code size for the Intel build a little bit if we use some
> preprocessor magic to eliminate the old codepath on that platform.  You can use
> the code referenced in comment 10 as a guide.

The code in comment 10 doesn't have any preprocessor magic in it? Is it just #ifdef PPC for Mozilla code?

I'll get back to this bug when I'm back in the office, since it will be much easier to make sure this works right with access to a 10.2 machine.

Comment 21

[Mark Mentovai]

Guess I didn't mean comment 10, then.

Sorry, I meant the TransitionWindowWithOptions stuff in widget/src/mac/nsMacWindow.cpp.  You can actually avoid the CFBundle loading stuff and eliminate the extra codepaths on x86.

The general idea is:

// begin mento-section

#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_3
// SDK is older than 10.3
// Declare things that aren't declared in the 10.2 SDK.  Your patch doesn't
// do this and won't build for ppc in the official build environment without
// taking this into account.
typedef struct HIThemeTabDrawInfo {
  UInt32 version;
  ...
} HIThemeTabDrawInfo;
#endif

typedef OSStatus (*funcptr)(...);

#if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_3
  // The compiler will warn about this, but it results in the "else" clause
  // being stripped.  This is exactly what we want.  You can avoid the
  // warning with an extra #if around the "else" clause below instead of
  // writing if (1) here, but be sure to leave all {scoping} the same no
  // in all preprocessing cases.
  if (1)
#else
  // I recommend using nsToolkit::OSXVersion() to avoid duplicated code.
  // Also, it caches the version avoiding repeated calls, and it'll probably
  // eliminate the "gestalt failed, do nothing" case in your patch.  You can
  // use widget/src/mac/nsToolkit from gfx on this branch.
  if (nsToolkit::OSXVersion() >= MAC_OS_X_VERSION_10_3_HEX)
#endif
  {
    funcptr f;
#if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_3
    // Deployment target is 10.3 or higher, we can link against this directly
    f = ::HIThemeDrawTab;
#else
    // Deployment target is less than 10.3, need to look up the symbol at
    // runtime.  This isn't weak linking, that's something else.  Weak
    // linking would be even more painful to use here because we support
    // building with the 10.2 SDK.  (The details of the differences between
    // this and weak linking are beyond scope here, but talk to me if you're
    // interested.)
    f = (funcptr)::CFBundleGetFunctionPointerForName(..., "HIThemeDrawTab");
#endif
    // end mento-section, begin cbarrett-section
    // new codepath using the new stuff you wrote
    f(...);
  }
  else {
    // existing codepath, old API
    ::DrawThemeTab(...)
  }

Comment 22

[Colin Barrett [:cbarrett]]

Great, thanks Mark.

Yeah, I know the difference between getting a symbol at runtime and weak linking, just failed to indicate that in the comments :\

Comment 23

[Marcia Knous [:marcia]]

I think I forgot to add at some point that the same thing happens on trunk builds with the Advanced Preferences pane, although it the tabs look a bit different. Page Info is not affected because we changed the layout for FF 3.

Comment 24

[Josh Aas]

Apple fixed this bug in the final release of Mac OS X 10.5. We don't have to fix it on our end any more.