Closed Bug 337771 Opened 14 years ago Closed 12 years ago

Native -moz-appearance work for menus and toolbars on Windows XP

Categories

(SeaMonkey :: Themes, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla-mozilla-20000923, Assigned: zeniko)

References

()

Details

Attachments

(2 files, 3 obsolete files)

This bug is a sub-bug of bug 243078, for dealing with writing and updating the existing native code for the menu and toolbar -moz-appearance styles on Windows XP. This will not cover Vista - that will be another bug, as it requires a different (but actually simpler) code path.

Please do NOT comment on this bug unless you have a technical contribution, or
it will become impossible to work on this bug.
Status: NEW → ASSIGNED
This is the current patch to trunk for the native appearance work. There's still some work to do (3 XXX comments, for a start), and I'm pretty sure it is leaking handles too.
Assignee: silver → nobody
Status: ASSIGNED → NEW
James,

Any movement on this or 243078?

~B
I am no longer assigned, and hence no longer working on any of these bugs. Please see http://twpol.dyndns.org/weblog/2006/08/23/01.
Attached patch unbitrotted (obsolete) — Splinter Review
So, this is James's patch cleaned up, unbitrotted and slightly refactored. Works pretty much as expected and fixes the worst visual regression since Firefox 2.

There'd still be some few issues left (e.g. what about toolbar separators taller than 2px?) but those can be handled in follow up bugs as well. Better to get this one (and Vista bug 355789) done before Beta 1 or people will start wondering...
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #266254 - Flags: review?(neil)
Comment on attachment 266254 [details] [diff] [review]
unbitrotted

Seems vaguely ok, although to be fair I only tried this on a suiterunner build (so no toolbar background) and I don't use Luna.
Attachment #266254 - Flags: review?(neil) → review+
(In reply to comment #4)
>Better to get this one (and Vista bug 355789) done
Speaking of getting things done, any chance of separating the various parts out into separate themeable objects i.e.
.menu-iconic-left[type="checkbox"] { -moz-appearance: menucheckbox; }
.menu-iconic-left[type="radio"] { -moz-appearance: menuradio; }
.menu-right { -moz-appearance: menuarrow; }
A separate apparance for menuitems on a menubar might help bug 355789 too.
Attachment #266254 - Flags: superreview?(mconnor)
Attachment #266254 - Flags: review?(emaijala)
(In reply to comment #6)
> any chance of separating the various parts out into separate themeable objects

Wouldn't that make state keeping even more tedious (as now we'd have to check hover-state for the menuitem and its ornaments separately)?

I guess separate appearances can always be added as part of bug 355789, should they really be required - with the above patch, the default theme at least doesn't draw any stock images through CSS anymore (BWT the way the GTK2 code works as well).

Bug 355789 could probably rather take advantage of a few more devs actually having access to Vista...
The patch for bug 243078 landed half-finished, but at the time we didn't care because 'it was trunk'. Now 'trunk' is gearing up to be a release build and we need to get this completed. Requesting blocking.
Flags: blocking1.9?
Comment on attachment 266254 [details] [diff] [review]
unbitrotted

r=emaijala
Attachment #266254 - Flags: review?(emaijala) → review+
I just applied this patch to my local tree. Are dropshadows supposed to be visible still underneath the menus still? I thought they were supposed to disappear when fully-native theming was enabled?
OK, looking more carefully at other native apps, I guess I was mistaken...
Changes as suggested by comment #6 to get more in line with the current patch from bug 355789. Now, if anybody could get mconnor over here or suggest an appropriate superreviewer...
Attachment #266254 - Attachment is obsolete: true
Attachment #270944 - Flags: superreview?(mconnor)
Attachment #270944 - Flags: review?(emaijala)
Attachment #266254 - Flags: superreview?(mconnor)
Blocks: 355789
> Now, if anybody could get mconnor over here or suggest an appropriate superreviewer...
How about roc, bz, or Neil?
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/widget/src/windows/nsNativeThemeWin.cpp

Attachment #270944 - Flags: superreview?(mconnor) → superreview?(bzbarsky)
Comment on attachment 270944 [details] [diff] [review]
style menu item decoration separately

>Index: widget/src/windows/nsNativeThemeWin.cpp
>+        nsIContent* parent = content ? content->GetParent() 
: nsnull;

Do you expect viewport frames to come through here?  If not, the content will never be null.

This code in general looks pretty fragile to me, unless I'm missing something.  What assumptions is it making about the content/frame trees?  Document them?  And add pointers to the relevant content/layout code that changes need to update this code too?

As things stand, I have no way to tell whether the code is correct.

Oh, and this code doesn't deal with XBL anonymous content.  Is that on purpose?

>@@ -876,23 +893,16 @@ RENDER_AGAIN:
>-  else if (aWidgetType == NS_THEME_TOOLBOX) {

So how come this is no longer needed?

>@@ -937,24 +947,27 @@ RENDER_AGAIN:
>+  else if (aWidgetType == NS_THEME_TOOLBAR && !state) {

Do you really mean "!state"?  Or "state != 0"?  The two give the same answer, but are not conceptually equivalent...

>+    // This is the same value as the top border on NS_THEME_TOOLBAR in GetWidgetBorder.
>+    widgetRect.bottom = widgetRect.top + 2;

In that case make it a #define somewhere and use that symbolic value in both places.  As this patch stands it would be trivial to change it in GetWidgetBorder without realizing that it needs changing here.

>@@ -1394,42 +1405,42 @@ nsNativeThemeWin::ClassicGetWidgetBorder

Where does the magic "3" come from?  And the rest of the magic numbers?

I didn't really review these changes, because with no explanation of how these numbers all fit together they're basically unreviewable.

>+        // XXXzeniko Windows renders these 1px lower than you'd expect

Again, this should probably be a #define that gets added in...

>+      // We render only mFlatMenus here - classic ones are styled through CSS

So can you assert that mFlatMenus is true?

I have to admit that I'm feeling pretty out of my depth reviewing the remaining changes to this file; I assume ere will cover that.  All of this code could use a lot more comments, I think.  And perhaps replacing of if cascades with lookup tables.  It'd make things a lot clearer...

I'm also not qualified to review the theme changes, bug I assume that you made sure that your core changes don't break themes other than winstripe?
Attachment #270944 - Flags: superreview?(bzbarsky) → superreview-
Minor update and a few answers.

(In reply to comment #14)
> This code in general looks pretty fragile to me,

Yeah, I'll rather leave that to a follow up bug and somebody who understands the potential issues better than myself. Reverted the changes. As long as the first toolbar isn't hidden or collapsed this will work as intended.

> >-  else if (aWidgetType == NS_THEME_TOOLBOX) {
> So how come this is no longer needed?

Toolbars no longer have a 1px top/bottom border but a 2px top border only (which is accounted for below).

> Do you really mean "!state"?  Or "state != 0"?

The latter. Corrected.

> In that case make it a #define somewhere and use that symbolic value

Done.

> Where does the magic "3" come from?  And the rest of the magic numbers?

From the same place as all the other magic numbers in that function: empirical observation. Not sure whether a comment like "that's the values Windows seems to use internally" would help much (kinda seems implied IMO).

> >+        // XXXzeniko Windows renders these 1px lower than you'd expect
> Again, this should probably be a #define that gets added in...

AFAICT this is the same issue as above: we have to do the same magic as Windows. Since we don't have #defines for all of it, I'm not sure whether or how to use one in this case.

> So can you assert that mFlatMenus is true?

Done.

> All of this code could use a lot more comments, I think.  And perhaps
> replacing of if cascades with lookup tables.  It'd make things a lot clearer...

Not sure what that means for my patch, since this applies to the whole file.

> I'm also not qualified to review the theme changes, bug I assume that you made
> sure that your core changes don't break themes other than winstripe?

Except maybe third-party themes? Our other themes either aren't used under Windows anyway resp. aren't affected by the CSS changes, and AFAICT Thunderbird and SeaMonkey don't try to change the base appearance of the menus based on our defaults. Haven't compiled&tested them though - will do the CSS fine-tuning in follow up bugs once mozilla.org does the compiling...
Attachment #270944 - Attachment is obsolete: true
Attachment #270971 - Flags: superreview?(bzbarsky)
Attachment #270971 - Flags: review?(emaijala)
Attachment #270944 - Flags: review?(emaijala)
> Yeah, I'll rather leave that to a follow up bug

And I'd rather not land code in the tree that the next layout checkin could break, frankly.  I can't sr that part of the patch as it stands...

> Not sure what that means for my patch, since this applies to the whole file.

Followup bug, at least.

> Except maybe third-party themes? 

Including, actually.  If we have to break them we have to break them (and I'm fine with it happening), but we don't really want to do it gratuitously.  If we do break them, we should release note it.

The real question is whether your core changes require the theme changes to take advantage of them or whether themes will sort of get blindsided.  If the latter, this needs to get relnoted.
(In reply to comment #16)
> I'd rather not land code in the tree that the next layout checkin could break

In case we're talking about the same piece of code (GetThemePartAndState for NS_THEME_TOOLBAR): there's nothing to land except a comment for which I'll file a follow-up bug once this bug is fixed.

> Followup bug, at least.

Filed bug 386929.

> > Except maybe third-party themes? 
> Including, actually.

Will do some testing and ask for a relnote if needed (or whatever we do to point themers to the many places where they'll have to adapt their theme anyway).
IMHO, we're breaking 3rd-party themes in so many cases on trunk that a single change like this doesn't need relnoting per se. Themes from any Gecko 1.8 product are incompatible with Gecko 1.9-based apps in many ways, and EM helps us in not activating them.
I guess that we'll have a new edition of http://developer.mozilla.org/en/docs/Theme_changes_in_Firefox_2 .

Anyway, an unrepresentative test with AMO's Top 5 themes shows that (1) they're already somewhat broken on Trunk, (2) this patch doesn't make any difference and (3) some of them really could take advantage of the new -moz-appearance values.
Yes, I was talking about the GetThemePartAndState changes.  You seem to have removed them in the second version of the patch.  Were they not needed?
(In reply to comment #20)
They weren't crucial, indeed. As I said in comment #15, we currently just depend on the first toolbar not being hidden (or else there will be an additional 2px border above). I'll see for a fix for that minor regression in a follow-up bug.
Comment on attachment 270971 [details] [diff] [review]
style menu item decoration separately (v1.5)

Ah, ok.  Looks good, then!  Please cc me on that followup?
Attachment #270971 - Flags: superreview?(bzbarsky) → superreview+
Attachment #270971 - Flags: review?(emaijala) → review+
Attachment #270971 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in "unbitrotted patch for check-in". Clearing checkin-needed keyword.
Keywords: checkin-needed
Attachment #271820 - Attachment description: unbitrotted patch for check-in → unbitrotted patch for check-in (checked in)
Marking this bug FIXED. Please file regressions or edge-cases (e.g. where we're a few pixels off) as new bugs and make them depend on bug 243078. Don't forget to attach screenshots of both Minefield and Windows Explorer (resp. IE6) for comparison.

(In reply to comment #21)
> I'll see for a fix for that minor regression in a follow-up bug.

Filed bug 388167 - as it turns out, it's no regression but simply still broken.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #6)
>.menu-right { -moz-appearance: menuarrow; }
Well, one out of three isn't bad ;-)
Blocks: 388318
Blocks: 373696
Blocks: 386732
No longer blocks: 373696
Depends on: 373696
Depends on: 389290
Blocks: 390861
Depends on: 415329
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.