Native Theme Rendering for Windows XP Menus, toolbars

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Toolkit
Themes
P1
normal
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Unassigned)

Tracking

Trunk
mozilla1.9alpha8
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: NON-TECHNICAL COMMENTS GO HERE: http://forums.mozillazine.org/viewtopic.php?t=317569, URL)

Attachments

(15 attachments, 10 obsolete attachments)

3.54 KB, image/png
Details
9.79 KB, image/png
Details
5.94 KB, image/png
Details
4.91 KB, image/png
Details
6.08 KB, image/png
Details
50.14 KB, patch
Details | Diff | Splinter Review
20.35 KB, image/jpeg
Details
47.64 KB, image/png
Details
5.65 KB, image/png
Details
47.32 KB, image/png
Details
22.42 KB, image/png
Details
46.11 KB, image/jpeg
Details
24.43 KB, image/png
Details
6.62 KB, patch
Ere Maijala (slow)
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
74.59 KB, image/png
Details
The toolbar border shadow is much too dark on WinXP Luna, and the menus are
incorrect (need to be drawn by native theme renderer). These are fairly high
visibility glitches.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Firefox1.0beta

Updated

13 years ago
Flags: blocking1.0?
Flags: blocking1.0? → blocking1.0+
Target Milestone: Firefox1.0beta → Firefox1.0
As a temporary workaround, the modifications from
http://texturizer.net/firefox/tips.html#app_xpmenus could be applied by default

Comment 2

13 years ago
if those were applied by default, it would look wrong on WinXP Classic, most
third party visual styles, and all older Windows OSes.
(In reply to comment #2)
> if those were applied by default, it would look wrong on WinXP Classic, most
> third party visual styles, and all older Windows OSes.

It doesn't look wrong in third party visual styles - the colours used are those
defined by Windows (XP's Advanced Appearance dialogue), so it actually follows
third party styles better than the current set-up. Correct, it would look
"wrong" in Classic/pre-XP, but at the moment it looks equally "wrong" in Luna.

According to Google Zeitgeist, 55% of Windows users are on XP, for whom the
*default* setting is Luna. So the default Firefox setting should, in my opinon,
match as well as it can.

...or not - it was just a suggestion for until we have proper native rendering.
Assignee: bugs → varga
Status: ASSIGNED → NEW

Comment 4

13 years ago
*** Bug 245253 has been marked as a duplicate of this bug. ***
Not a "blocker"
Flags: blocking-aviary1.0+ → blocking-aviary1.0-

Comment 6

13 years ago
(In reply to comment #3)
> It doesn't look wrong in third party visual styles - the colours used are those
> defined by Windows (XP's Advanced Appearance dialogue), so it actually follows
> third party styles better than the current set-up. Correct, it would look
> "wrong" in Classic/pre-XP, but at the moment it looks equally "wrong" in Luna.
> 
> According to Google Zeitgeist, 55% of Windows users are on XP, for whom the
> *default* setting is Luna. So the default Firefox setting should, in my opinon,
> match as well as it can.
> 
> ...or not - it was just a suggestion for until we have proper native rendering.

It does look wrong in third party styles though, very wrong.  Well, I guess it
depends on the style you're using.  This has really bugged me for a long time, I
really hope there is someone looking into this.

Updated

12 years ago
Flags: blocking-aviary1.1?

Updated

12 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-

Updated

12 years ago
Flags: blocking-aviary2.0?

Comment 7

12 years ago
The group boxes and tree views should also be rendered by the native theme
rendered, also the tabboxes still need work.

Comment 8

12 years ago
Nominating to block Firefox 1.5 Beta 2, because fixing this would stop the
flamewars over bug 303806.
Flags: blocking1.8b5?

Comment 9

12 years ago
The menu highlight in 1.5 beta 1 (for example when you hover the mouse over
"File") extends to fill height of the toolbar. This creates big ugly blue boxes
when the navigation buttons are placed on the same toolbar as the menu,
particularly when large icons are used. The problem seems to have been
introduced by bug 253661 but according to comment #65 in that bug it's part of
this bug.

Bug 303806 was supposed to be a temporary fix until this bug was sorted but
hasn't managed to fix this particular problem.
*** Bug 307652 has been marked as a duplicate of this bug. ***
*** Bug 307701 has been marked as a duplicate of this bug. ***
*** Bug 307653 has been marked as a duplicate of this bug. ***
*** Bug 307855 has been marked as a duplicate of this bug. ***

Comment 14

12 years ago
I'm currently having a go at hitting the native theme code hard enough to fix
this. It's not pretty, but I've got -moz-appearance working with menuitems and
menupopups with XP themes, as well as fixing a bug with the toolbar and toolbox
impls., and now need to sort out the classic renderings (and make the CSS-only
appearance match classic too).

Comment 15

12 years ago
i'm very appreciative that someone is finally working on things like this.  any
chance you could have a go at implementing -moz-appearance: dualbutton on
windows, too?  i put together a css hack that does it in the relevant bug, but a
true fix would be so much better.

Comment 16

12 years ago
It's nice to be appreciated.

Seriously, I'll work on the dualbutton if I can, but for now I'm sticking to the
menus. Am I right in thinking the dualbutton is the buttons with drop-downs
split into two halfs, e.g. the back and forward buttons? I've definately got
enough an idea of the Windows theme system to get all the visuals needed, though
I've not looked at how the buttons are identified in the nsITheme system.

Sneak-peak of CSS-free menu apperances:
http://twpol.dyndns.org/temp/appearance_menu_1.png (Luna VX)
http://twpol.dyndns.org/temp/appearance_menu_2.png (Luna)
No Classic shot yet (not coded that view) and those two aren't bug-free yet. :)

Comment 17

12 years ago
looking good!

yeah, dual buttons are the ones with a dropdown like back and forward.  if you
want a look at the css hack i came up with, it's in bug 216266.  

hmm, have you tried this with windowblinds?  it'd be extra-cool if it picks up
windowblinds-styled menus too.

Comment 18

12 years ago
James, how does this look in Classic, by the way?  I'm using that and would love
it to work right (with black menu background colour, nonetheless).

Comment 19

12 years ago
Shouldn't this be assigned to James Ross now?
btw target milestone should change

p.s. great work :D

Comment 20

12 years ago
Henrik Pauli, currently it looks like ass in classic, because it is still using
my XP theme code. ;)

I've almost got the CSS-only (i.e. no theme code in sight) mode looking just
like classic, however, and it is only a matter of time before the -moz-apperance
version also works in classic.

I only started yesterday, and there's a lot to do with 3 different modes to
handle, after all. :)

Re: window blinds, I will see how it looks, but can't promise anything. If it
uses the uxtheme stuff in XP, I think it'll be good though.

Ressigning me, clearing target milestone and setting P1.
Assignee: Jan.Varga → silver
Priority: P3 → P1
Target Milestone: Firefox1.0 → ---

Comment 21

12 years ago
Ok, Window Blinds is evil. It injects its own DLL into every process, and hooks
into the uxtheme API calls, and that's the good news. The bad news is that is
also hooks itself into rendering native menus, which obviously doesn't work for us.

I expect that is because the uxtheme API wont let you get any menu information
except a selection of colours (which is all themes can specify). Problem is that
Window Blinds is drawing the menus itself, so the menu colours are reasonable
but not going to give the same effects.

So Window Binds LAF is not possible, but I'll make sure the real themes work.

Comment 22

12 years ago
Classic: http://twpol.dyndns.org/temp/appearance_menu_3.png

Before anyone asks, no I'm not quite done yet. Currently the themed toolbars
(previous images 1 and 2) have some small graphical errors (and in fact the
classic view has some too, but elsewhere in the Fox), and the code can't yet
switch between Classic apperance and Themed apperance by itself.

Comment 23

12 years ago
Certainly looks sweet so far!

Hey, one thing that I just remembered.  You know the check mark and radio bullet
and the submenu arrow, right?  On screenshot 1 it's of a different colour than
the text -- currently, in the "original" mozilla builds, the unfocused check
mark and bullet and arrow seems to be strictly black and the focused ones
strictly white... I don't know the source of the browser so I don't know if it's
possible to fix this, but it would be awesome if these widgets behaved as the
colour of the menu suggests, using the text colour.
Silver, if the checkboxes and submenu arrows are also drawn natively, please
make sure not to break RTL UI.

Comment 25

12 years ago
Currently the check, radio and submenu arrows are still just images attached via
CSS. I plan to leave them like that for the initial patch, and then fix that
separately (though I guess it could just be done now too...).

Asaf, thanks for the reminder, though I've had RTL going through my head all day
today with the CSS changes, so you need not worry just yet. :)
*** Bug 307958 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Component: General → Themes
Product: Firefox → Core
Version: unspecified → Trunk

Comment 27

12 years ago
James, could you use native code calling uxtheme to render the menus? This is what Java does to render 
components on Windows.

Comment 28

12 years ago
"native code calling uxtheme" is exactly what the code is doing (for all the
existing theming and the stuff I'm adding) - it uses the OpenThemeData API call
(see MSDN) and associated calls. However, the problem is, this call does not
provide access to whatever-it-is that uxtheme has for menus (the call fails with
"Element not found" for "Menu").

As it happens, I don't think there *is* any theme data for menus apart from a
list of about 4 colours - which I've already deciphered and coded it to use. :)

Here's a little nugget of information: Windows Media Player 10 custom-draws its
Media Library menus (the one listing all the albums etc.), and it doesn't match
the LunaVX theme where my code does (it misses the borders of highlighted menu
items).

Current status: just trying to make the check/radio images work.

Odd bug of the day: changing too or from Classic mode does NOT trigger a
ThemeChange notification inside the nsITheme interface.

Comment 29

12 years ago
Right! I've got the menu check, radio and submenu images working.

http://twpol.dyndns.org/temp/appearance_menu_1b.png
http://twpol.dyndns.org/temp/appearance_menu_2b.png
http://twpol.dyndns.org/temp/appearance_menu_3b.png

And, the all important RTL version:
http://twpol.dyndns.org/temp/appearance_menu_4.png

Got a problem now with the bookmarks menu, but it's getting there!

Comment 30

12 years ago
James, might want to look through this Windows Vista UX Guidelines (preliminary)
http://www.microsoft.com/downloads/details.aspx?FamilyID=fd380553-911e-4659-a085-4dd58ae4b9ae&displaylang=en&Hash=8VLJ9G9

Linking you so that maybe you can do some things now to make sure Firefox will
work with vista UI and integrate nicely.

Silver, this is good stuff.  If there's any extra work for Vista, we can do that
in a followup bug.  However, the linked doc is an interface design doc, not an
API guide.

Comment 32

12 years ago
Created attachment 195679 [details] [diff] [review]
Current toolbox, toolbar, menubar, menupopup, menuitem patch

This is straight from my tree (though it probably wont apply, as I appended
diffs from two different directories: widget and toolkit/themes/winstripe), and
as such expect to find excessively long lines, bad indenting, and absolutely no
comments!

I'm attaching this now a) so I don't lose the thing, b) so anyone who wants can
both try it themselves, and c) so people can spot errors in the code.

If anyone wants a screenshot of X or Y woth this patch, I'll do it - though I
think my 1b, 2b, 3b and 4 images should have covered everything this patch will
affect.
*** Bug 308069 has been marked as a duplicate of this bug. ***

Comment 34

12 years ago
Wow, this is great, James.

I see that you've commented out the codes for linking to the radio and checkmark
images. Does this mean we no longer need 'em?

Comment 35

12 years ago
I'm not quite sure what to do with them. The XBL elements they occupy are
required to keep the positioning of the text correct, but the images are drawn
natively...

The actual images are drawn as part of the background to the element so, if the
images in CSS are also specified, they go ontop of the native images and mess
things up.

As far as I can tell, they would only been needed if you had a normal menu item
and just turned off -moz-appearance on it - you'd then get the CSS style, which
looks just like Classic - which would leave you with gaps for those images.

Comment 36

12 years ago
With this patch, what happens to the menu bar when the toolbar is taller than
normal? (e.g if the back/forward buttons have been moved onto the same row as
the menu) Currently, when hovering over menu items, the highlight fills the
whole height of the toolbar, which looks a bit odd. Other windows apps (e.g IE /
Explorer) restrict the height of the highlight.

I assume this would be fairly simple to fix with CSS. e.g 
menubar { max-height: 1.2em } or similar.

Comment 37

12 years ago
If only it was. :(  (currently they do go the entire height)

I tried some CSS on the menubar, but it inexplicably broke the spring component
in Customize (it ended up being 2px high). I don't actually understand why, but
since trunk Firefox is totally screwed WRT extensions and their overlays, I
can't get to DOMI involved to find out why.

It is on my list of "things that aren't quite right" though, along with the
wrong text colour for highlighted menubar items in Windows Classic.

Comment 38

12 years ago
On this screen: http://twpol.dyndns.org/temp/appearance_menu_2b.png, shouldn't
"View" be in white letters?
Otherwise, awesome work, keep it up!

This should really get in for 1.5.

Comment 39

12 years ago
It should, yes, as I already said in comment 37 ("...along with the
wrong text colour for highlighted menubar items in Windows Classic"). It's not
simple to fix, though, and I am currently evaluating various options.

Comment 40

12 years ago
It's too late in the game for this and this is something we've been targeting
for the 2.0 release. 
Flags: blocking1.8b5? → blocking1.8b5-

Comment 41

12 years ago
If there's a working patch for this between the b2 and rc1, why not? You can
always back it out if it happens to bring serious regressions. The current
situation, well, sucks (can't phrase it otherwise).

Comment 42

12 years ago
(In reply to comment #40)
> It's too late in the game for this and this is something we've been targeting
> for the 2.0 release. 

I talked to Mike Connor yesterday on IRC and he stated that given we have three
weeks till the next beta it's possible this could land (that's not verbatim but
pretty close) providing this is wrapped up soon.

~B
Flags: blocking1.8b5- → blocking1.8b5?
it was possible, but that wasn't a definitive statement.  This certainly isn't a
blocker, but that doesn't automatically mean we'll reject a safe and reasonably
baked patch.  this is a 300+ line patch though, so the risk of regressions is
statistically high.
Flags: blocking1.8b5? → blocking1.8b5-

Comment 44

12 years ago
The good thing about this patch is that regressions are totally visible and
there's a fair few testers in the community - the anti-Luna brigade ;) - who
want to see this land for Firefox 1.5.
Created attachment 196027 [details] [diff] [review]
patch2

This is the same as James'patch, but this one should apply cleanly.

Comment 46

12 years ago
I think you guys are wrong to not consider this important enough to block 1.5, 
but then why not ask all the users who are getting seriously irrate with the 
problem itself? (heck, *I* am one such irrate user, I just happen to be irrate 
enough and knowledgable enough about the codebase to actually fix it)

I wont be at my computer until Friday, when I will fix the remaining niggles I 
have with the impl. hopefully, and you will then have over two weeks to get it 
through the appalling review process in time for FF 1.5 Beta 2.

Please start picking a reviewer and super-reviewer NOW, because I am not 
interested in fighting just to get someone to review this in time to not 
disappoint your users.

Comment 47

12 years ago
James, I started a thread on mozillazine to discuss and test your patch(s) and
to report any bugs there so as to not spam this bug. 

http://forums.mozillazine.org/viewtopic.php?t=317569

Updated

12 years ago
Whiteboard: has partial patch, needs heavy testing

Comment 48

12 years ago
(In reply to comment #3)
> According to Google Zeitgeist, 55% of Windows users are on XP, for whom the
> *default* setting is Luna. So the default Firefox setting should, in my opinon,
> match as well as it can.

The information we don't have is how many of those people are actually using
Luna. If 9% or more of Windows XP users are not using Luna, then that means the
change to Luna-style menus us making Firefox appear "wrong" on more than 50% of
Windows users' machines. Common sense would be that Firefox should at least
appear "normal" on the majority of users' machines, but I suspect the move was
not motivated by common sense.
note that those stats are as of June 2004, and most stats I've skimmed put XP at
65% or higher and continuing to grow.

Comment 50

12 years ago
but there are no stats on how many people use luna.  most people i know who use
XP turn it off because it's, well, hideous.

Comment 51

12 years ago
That doesn't really mean anything.  Most people I know seem to post screenshots
with completely untouched WM settings and I have no idea how they can live like
that :)

Speculation won't lead us anywhere really.  Let's go try to find some
statistics... or start a meme on LiveJournal on this topic or something.  Should
be more effective than this "most of my friends" thing.

Comment 52

12 years ago
Statistics aren't going to get this bug fixed and in either case this isn't the
place for them. Please direct such comments to this thread:
http://forums.mozillazine.org/viewtopic.php?t=317569

Comment 53

12 years ago
Created attachment 196424 [details] [diff] [review]
Gecko 1.8 branch with improvements

This is the original patch ported to the 1.8 branch, plus lots of improvements
(like fixing those damn autocomplete popups).

One unfortunate bit was I had to rewrite the bookmark toolbar chevron code
because it completely failed to cope with borders on the toolboxes/toolbars. It
now also works great in RTL too!

The only issues I know of with it are a slight mis-alignment in the bookmarks
menus (to be fixed shortly), and I believe it uses the Win2000 menubar styles
on Win9x versions too, which I don't know if it is worth fixing (I don't have
anything but 2K and XP to test with, unfortunately).

I've made a zip of my branch build with this patch in, for anyone that wants to
test it:
  http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip
Attachment #195679 - Attachment is obsolete: true
Attachment #196027 - Attachment is obsolete: true

Comment 54

12 years ago
Everything looks great to me! While I don't like the fact that the blue bg is
there for hovering over the menus, it is native with my theme, so you seemed to
have done one hell of a good job.

I don't see any problems on my end.

(In reply to comment #46)
> I think you guys are wrong to not consider this important enough to block 1.5, 
> but then why not ask all the users who are getting seriously irrate with the 
> problem itself? (heck, *I* am one such irrate user, I just happen to be irrate 
> enough and knowledgable enough about the codebase to actually fix it)
> 
> I wont be at my computer until Friday, when I will fix the remaining niggles I 
> have with the impl. hopefully, and you will then have over two weeks to get it 
> through the appalling review process in time for FF 1.5 Beta 2.
> 
> Please start picking a reviewer and super-reviewer NOW, because I am not 
> interested in fighting just to get someone to review this in time to not 
> disappoint your users.

As for reviewer and superreviewer, I say mano should review, and mconnor should
superreview, with of course ben approving.
Whiteboard: has partial patch, needs heavy testing → has nearly-complete patch, needs immediate testing, targetting 1.5
spam: Of couse, it does not matter that i'm not peering win32 widget/gfx or that
mconnor isn't a sr. If Tongleback says, we do.

enough spam from me, go ahead and stop adding useless comments too.

Comment 56

12 years ago
I'm thinking a toolkit peer (e.g. mconnor) for review, and a gfx/widget peer for
superreview (e.g. neil or roc) would be about right. Of course, anyone else is
free to look over it and find problems. :)

Comment 57

12 years ago
Wow, this just looks great! I have tested this with Luna and a custom theme
(Watercolor) and it seems to work very good in both themes. I have found a
little glitch, though. I use the Cutemenus extension and menu items without an
icon appear a little too much to the left. A good example can be seen in the
Edit menu with the two last items: http://www.tweakers.net/ext/f/66533/full.png.
I don't know if this is something that has to be fixed by the extension or by
you, but considering the fact you are working on this, I thought at least you
had to know it.

Comment 58

12 years ago
The problem is that the icons used are larger than the space allocated to the
menu item images by Windows (13 or 14px). The bookmarks menu has this issue too,
but has CSS to make the items without images line up, and I've just corrected
that CSS so they do line up with my patch.

The question is, should all menu items allocate space for a 16px-wide image?

In fact, if you actually look closely at the CuteMenus effect on the File menu,
the first two items don't even line up with each other, as the new tab image is
17x17, and the rest are 16x16 (I think import is actually bigger too).

Comment 59

12 years ago
Created attachment 196443 [details] [diff] [review]
Updated to fix alignment issues in bookmarks

This is basically it. The final patch. Probably. :)

http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip has been
updated with a new build.
Attachment #196424 - Attachment is obsolete: true

Updated

12 years ago
Attachment #196443 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196443 - Flags: review?(emaijala)

Comment 60

12 years ago
Created attachment 196456 [details]
Screenshot on Win98

The little arrow to show that there's a submenu is not shown (Win98 with a
clean profile).

Comment 61

12 years ago
Nice improvement on the menus and context menu on Classic! I also like the
corrected vertical spacing of the menu items.

I found a few issues that this patch doesn't yet address:
- In Windows XP Luna (or Media Center style), the context menu still has no 3D
shadow.
- The toolbar border shadow is still too dark.
- When clicking menulists (e.g. Options->Content->Default Font), the the menu
still doesn't slide down, but opens instantly.

Comment 62

12 years ago
Steffen, none of those are even close to related to this bug, except the toolbar
border, which is the same colour as it was before, and can be fixed separately.

Interesting thing with Win98... wonder what is going on there.

Comment 63

12 years ago
Actually, the toolbar border is *better* than it was - it looks perfect on one
theme I have here, it is just not quite matching in the other theme. I actually
think IE hard-codes the colours, anyway, so we're looking better than IE in that
sense.

Comment 64

12 years ago
Created attachment 196472 [details] [diff] [review]
Corrected padding on menubar

This update just corrects the padding of items in the menubar, and removes the
use of DFCS_TRANSPARENT, which was unnessessary (and might be the problem with
Win98).

James C, could you please try out the updated build at
http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip and see if
the submenu arrows work on Win98 or not. Thanks.
Attachment #196443 - Attachment is obsolete: true
Attachment #196472 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196472 - Flags: review?(emaijala)

Updated

12 years ago
Attachment #196443 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196443 - Flags: review?(emaijala)

Comment 65

12 years ago
The latest build didn't fix the submenu arrows on Win98.

Comment 66

12 years ago
Just a note on Royale, The Menu BAckrounds use the Same Colour as the Toolbar
backround (grey), instead of White

Comment 67

12 years ago
Created attachment 196477 [details]
Screenshot On WinXP with Royale theme

Comment 68

12 years ago
Created attachment 196478 [details]
disabled submenu in win2k

Just wanted to write about disabled submenu hovering in win2k. see part "1" of
the screenshot. The arrow is white, while it should remain dead.

But I noticed new build and tried it before posting. The result is shown in
part "2" of the schot.

Comment 69

12 years ago
S. Jewers, I think you've confused the theme engine there [1]. In particular,
the fact the background of the text on the menubar items is not the same as the
menubar colour - I've see that here, and you need to kick the theme engine to
fix it (usually switching theme to Classic and back does it, reboot as a final
resort).

[1] It looks like the theme says to use flat menus, but the theme engine has got
confused as is not doing the flat menu rendering.

Comment 70

12 years ago
Tim, part 2 is not a disabled menu item.

I've fixed the colour change on hover here now. I would really like some help
with working out why the arrows don't appear on Win98... do
checkmarks/radiomarks appear ok?

Comment 71

12 years ago
Just to confirm: I get the same behavior as Shayne Jewers on a cleanly restarted
WinXP using the Silver theme (no theme hacking involved) with the most recent
version of the patch and a clean profile.

Comment 72

12 years ago
What are you confirming? You're using a different theme - and one that is known
to work fine!

Comment 73

12 years ago
My bad. Looks like I've managed to hose up my visual configuration. It works
fine with an untouched Silver theme, but not under the one I've fiddled around
with (IIRC I've done so only through the official display properties).
Notwithstanding all other programs (including vanilla Firefox) display properly.

Comment 74

12 years ago
I just wanted to confirm that this patch works fine with Royale (Media Center
style).

Comment 75

12 years ago
Created attachment 196513 [details]
Menubar

The arrows are shown on Win98 with the latest build, as are checkmarks and
radiomarks. 

The only problem I'm having now is that I have my personal bookmarks menu items
in the menubar to save space and they are much further to the right in the
build with this patch than in the latest nightly. Also, this makes the menubar
a couple of pixels taller than by default and the menu items are not vertically
centred. Little things and I'm not sure if they're both caused by this patch
but I thought it was worth pointing them out.

Comment 76

12 years ago
The bootmarks toolbar change is a result of me fixing the code that decides what
to display. In fixing that, it now behaves just like a normal spring element,
and will always be allocated the same space as other springs on the same
toolbar, irregardless of the items on it. It also means it works a lot better in
RTL mode.

As for the vertical alignment - that matches IE here in my latest build. If that
is not quite vertically centered according to you, so be it. We've got to come
to some compromises - the menus are pixel-aligned currently.

Going to attach new patch now Win98 problem is fixed.

Comment 77

12 years ago
Created attachment 196519 [details] [diff] [review]
Final version of menubar, menuitem, toolbar patch

This is hopefully the last update for now; it fixes the images not displaying
on Windows 98 (and probably 95/ME), and also makes the hover effects apply to
disabled items correctly.

Assuming no serious issues are found with this, I'd like to fix any remaining
problems (e.g. colours of the toolbar separators, if they need fixing) in
separate much smaller patches.
Attachment #196472 - Attachment is obsolete: true
Attachment #196519 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196519 - Flags: review?(emaijala)

Updated

12 years ago
Attachment #196472 - Attachment is obsolete: false
Attachment #196472 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #196472 - Flags: review?(emaijala)

Comment 78

12 years ago
(In reply to comment #69)
> [1] It looks like the theme says to use flat menus, but the theme engine has got
> confused as is not doing the flat menu rendering.
Actually, Royale doesen't use flat menus :S

i'll try switching to classic and back, to see if it fixes

Comment 79

12 years ago
Created attachment 196542 [details]
Minor Problems

Selecting a search engine adds a checkmark to the left of the engine name which
is visible when selecting an engine with an icon with a transparent background
(e.g. Yahoo).

Sidebar header should have top/bottom borders.

Comment 80

12 years ago
The search engines thing is a bug with their menu, really - I can't do anything
about that. Also, the sidebar header looks better without borders in Classic
(thanks to the bad decisions on the apperance of the content area, and the
sidebar itself). If the 'theme people' want to come and mess with that stuff
later, they can. IF you want me to fix it, tough, I'm not going to until this
patch has landed and I can actually do things without making another huge patch.

Comment 81

12 years ago
Fair enough, just thought they were worth pointing out.

BTW - was the problem with Cutemenus causing menuitems to be misaligned fixed? I
thought an earlier version of this patch fixed it but it seems to be happening
again...

Comment 82

12 years ago
The issue is with the extension, basically. It is using images bigger than the
size available, which pushes the text across. The alternative, squashing the
images, would ruin the favicons in bookmarks (they're bigger too). As I think I
said at the time, unless there is a policy decision to make the image area of
menus always big enough for 16x16, it will be up to the icon to be small enough
(13x13).

Comment 83

12 years ago
(In reply to comment #69)
> you need to kick the theme engine to
> fix it (usually switching theme to Classic and back does it,

Tried that, didn't work :S

btw if you wanna test it on royale, you can download it at http://tinyurl.com/97qpj

Comment 84

12 years ago
Looks fine to me: http://twpol.dyndns.org/temp/ff_theme_royale.png

It seems that fully-native menus (i.e. those found on apps that don't use
ReBars, e.g. Paint: http://twpol.dyndns.org/temp/paint_theme_royale.png ), there
is a rendering problem with the Windows Theme Engine - it really should not be
drawing the text with a grey background onto the almost-white background, and
then drawing it all grey on click, that's just broken, and IE + my patch render
it much better.

Comment 85

12 years ago
yours looks diffrent than mine o_O
http://img79.imageshack.us/img79/1117/untitled0pw.png

must be something on my end.......

(sorry for the bugspam btw)

Comment 86

12 years ago
(In reply to comment #85)
> must be something on my end.......

Royale is a Tablet PC skin. It acts strangely sometimes on XP, like distorting
the icons on the taskbar. This can happen if you don't like the normal window
title height and fiddle around with this setting. I have also seen this
phenomena from the screen shot, even on normal windows menus, ie not in Firefox
but in bundled apps. This was very long ago, hence not necessarily connected to
this patch.

Comment 87

12 years ago
I just tried the Royale theme again, and it did the same as attachment 196477 [details]
this time - it's definately not playing the same game as us. I think it is
probably best not to worry about exactly what it is up to until the main patch
here is checked-in, or we may be here for weeks. :)

Comment 88

12 years ago
keep in mind there are several (slightly different) versions of the royale theme
floating around.  they might behave differently.

Comment 89

12 years ago
(In reply to comment #85)
> yours looks diffrent than mine o_O
> http://img79.imageshack.us/img79/1117/untitled0pw.png
> 
> must be something on my end.......
> 
> (sorry for the bugspam btw)

Ok this is to all the guys who are reporting problems with the royale theme.
This "bug " is nothing to do with James Ross patch, so James dont worry :)

Anyway, to get rid of the rendering bug, go to appearance tab and choose Royale
and apply. The bug will be gone, I posted here as I did not want James going off
checking his patch when it nothing that he has done wrong. Hope that helps.

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta5

Comment 90

12 years ago
Comment on attachment 196519 [details] [diff] [review]
Final version of menubar, menuitem, toolbar patch

My build is hosed, will take a little while before I can get to this. I'll
review the c++ part of the patch, but you should find someone else to do the
theme xul, css etc.
Ere, I'll review the xul/css changes, not a problem there, its the widget code
that we really need your review on.

Comment 92

12 years ago
Comment on attachment 196519 [details] [diff] [review]
Final version of menubar, menuitem, toolbar patch

Is this a branch patch or something?

>RCS file: /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.cpp,v

>RCS file: /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.h,v

Comment 93

12 years ago
It has to be, or it stands *zero* chance of landing on the branch. As it
happens, it should apply to trunk fine after correcting the paths to those two
files, and one adjustment for an extra header.

Comment 94

12 years ago
Neil, see previous comment.

Also, to make things clear, this patch is simply to clean up the mess made of
the Firefox 1.5 Beta 1 release in time for Beta 2. Once that has actually
happened, I'll move forward with a full trunk fix, which will include a few
extra things that I can't fit in time-wise on the branch.

Updated

12 years ago
Summary: Native Theme Rendering for WinXP Menus, toolbars → Native Theme Rendering for Windows XP Menus, toolbars

Comment 95

12 years ago
Silver, please ask someone else for review if you're in hurry. I can't review
(or do) anything at the moment due to ongoing problems with my network connection.

Comment 96

12 years ago
*sigh*

mconnor, suggestions?

Updated

12 years ago
Attachment #196519 - Flags: review?(emaijala) → review?(dougt)

Comment 97

12 years ago
Comment on attachment 196519 [details] [diff] [review]
Final version of menubar, menuitem, toolbar patch

I only reviewed the the c++ files. 

Comments for attachment 196519 [details] [diff] [review]

Please move these:
+#define DFCS_RTL	      0x00010000   // OURS, not Microsoft's!
+#define DFCS_CONTAINER       0x00020000   // OURS too!

to below the the ifndef then define's statements.

You may also want to redefine all of these constants with the form:

#ifdef VALUE
#undef VALUE
#endif
#define VALUE

This ensure that the constant's value is consistent regarless of the SDK.



You do not need to use :: when calling:
+  rv = ::SystemParametersInfo(SPI_GETFLATMENU, 0, &isFlatMenus, 0);


I never liked comments that are sort of cryptic:
+    case NS_THEME_TOOLBAR: {
+      // Don't draw this as background, use special code.


Can you expand on what you are doing so that the casual reader can understand
without thinking too hard?


Are you always guarteed that the |content| will be non-null if aFrame is
non-null??  This pattern exist elsewhere in the patch.

+      if (aFrame) {
+	 nsIContent* content = aFrame->GetContent();
+	 nsIContent* parent = content->GetParent();


Please rework the NS_THEME_TOOLBAR code block so that aState is only touched
once (use an else)


Could you comment on why you care about this specific state? I think i
understand, just add some comments.
+  if ((part >= 0) && (state >= 0))


Assign nsnull to menuFrame when declaring.  This pattern exist in other parts
of the patch.

+      nsIMenuFrame *menuFrame;
+      CallQueryInterface(aFrame, &menuFrame);
+
+      if (menuFrame) {


It is a good idea to comment why these values were used:
+	 (*aResult).left = 3;
+	 (*aResult).right = 4;


If menuFrame is null, you will crash:

+      if (menuFrame) {
+	 nsIMenuParent *menuParent = menuFrame->GetMenuParent();
+	 if (menuParent)
+	   menuParent->IsMenuBar(isTopLevel);
+      }
+
+      if (isTopLevel) {
...
+      } else {
+	 PRBool isContainer;
+	 menuFrame->


In DrawMenuImage(), can or should these be cached:

+    int checkW = ::GetSystemMetrics(SM_CXMENUCHECK);
+    int checkH = ::GetSystemMetrics(SM_CYMENUCHECK);

And if so, should we stash hMonoBitmap for reuse?



Changes to layout/style/ should be run by dbaron or jst.
Attachment #196519 - Flags: review?(dougt) → review-

Comment 98

12 years ago
(In reply to comment #97)
> You do not need to use :: when calling:
> +  rv = ::SystemParametersInfo(SPI_GETFLATMENU, 0, &isFlatMenus, 0);

I know I don't, but it is the style used in this file so I'm sticking to it.

> I never liked comments that are sort of cryptic:
> +    case NS_THEME_TOOLBAR: {
> +      // Don't draw this as background, use special code.
> 
> Can you expand on what you are doing so that the casual reader can understand
> without thinking too hard?

I can try...

> Are you always guarteed that the |content| will be non-null if aFrame is
> non-null??  This pattern exist elsewhere in the patch.

As far as I can make out from
http://wiki.mozilla.org/Gecko:Key_Gecko_Structures_And_Invariants, yes.

> Please rework the NS_THEME_TOOLBAR code block so that aState is only touched
> once (use an else)

That would needlessly complicate that bit of code.

> Could you comment on why you care about this specific state? I think i
> understand, just add some comments.
> +  if ((part >= 0) && (state >= 0))

I will add comments.

> In DrawMenuImage(), can or should these be cached:
> 
> +    int checkW = ::GetSystemMetrics(SM_CXMENUCHECK);
> +    int checkH = ::GetSystemMetrics(SM_CYMENUCHECK);
> 
> And if so, should we stash hMonoBitmap for reuse?

They could be cached, but I'd have to go and fix the bug that means the theme
code doesn't get told about metric changes, only theme changes (and even then,
not always). I'm not willing to try and fix that with this patch.

Comment 99

12 years ago
I've already updated my patch based on the comments, should I attach now or wait
for more comments?

Comment 100

12 years ago
ofcourse attach it

Comment 101

12 years ago
Created attachment 197534 [details] [diff] [review]
Updated with comments [checked in]
Attachment #196519 - Attachment is obsolete: true
Attachment #197534 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #197534 - Flags: review?(dougt)

Updated

12 years ago
Attachment #196519 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

This patch look okay.  I am somewhat concerned about using values that are
obtained from visual inspection.  What if they change with some service pack? 
Given this isn't a problem, r=me.  you should get an addl. review by mconnor
et. al on the xul/css changes.
Attachment #197534 - Flags: review?(dougt) → review+

Updated

12 years ago
Whiteboard: has nearly-complete patch, needs immediate testing, targetting 1.5

Comment 103

12 years ago
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

Requesting additional review on behalf of silver, for XUL/CSS portion.
Attachment #197534 - Flags: review?(mconnor)

Comment 104

12 years ago
affects thunderbird too?
I've only just had time to give this patch a whirl, and then it was on suite for
some reason (!), so I had to guess at some of the menu.css changes.

I would really appreciate it if the menu images could be drawn opaquely,
so that xul|* { -moz-appearance: none; } didn't break anything.

Comment 106

12 years ago
What would it 'fix' by drawing them opaquely? Surely that's not even
realistically possible, given the dynamic backgrounds?
Sorry, but I only have the default XP themes to play with... do third-party
themes allow you to have custom popup backgrounds too?

Comment 108

12 years ago
No, they are limited to solid colours. What doesn't make sense from your
comments is what drawing them opaquely would affect - it would simply waste time
drawing the background colour on the background.
Transparancy requires two BitBlts but opacity only requires one. But the reason
I was interested in drawing the menu parts opaquely is that it makes themes more
portable to OSes that don't have native theme rendering, as they can just
specify a fallback image. I believe ordinary checkboxes and radio buttons work
like that, although that may require an individual appearance for each part.

Comment 110

12 years ago
Fallback images only work as backgrounds, since that is all the nsITheme stuff
can override. That is why the patch removes the image setting CSS - those images
end up ontop of *everything* the native code draws.

A solution - and this is something I have through about, but specifically not
tried to do on the branch - would be to add some extra theme "parts" to
nsITheme, for the various images. Then set -moz-appearance: menuitemcheckmark;
or whatever on the XBL's element, as well as setting the fallback image using
background-image.
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

sr=me for branch but see below for nits.

>Index: browser/base/content/browser.xul
>Index: browser/components/bookmarks/content/bookmarksMenu.js
Maybe you mentioned it in a comment that I've overlooked, but this looks like a
different patch.

>+#navigator-toolbox {
>+  border-bottom-width: 1px;
>+  -moz-border-bottom-colors: ThreeDShadow;
>+}
Out of interest what's special about this toolbox?

>+  mMenuActiveAtom = do_GetAtom("_moz-menuactive");
Ideally this belongs in nsNativeTheme.cpp where it can be shared with GTK2 and
future implementations.

>+  HRESULT rv;
Nit: BOOL SystemParametersInfo()

>+  // This will simply fail on Windows versions prior to XP, so we get
>+  // non-flat as desired.
[Trunk note: FYI NT3.51 desires flat menus, while Windows 95 desires flat
menubars. So it would seem that you can't write a Windows 95 theme for Windows
XP.]

>+    // state == 1 iff this toolbar is the first inside the toolbox, which
>+    // means we should omit the top border for correct rendering.
Would a toolbox top margin of -1 to push the top border under the non client
area be even hackier? It would seem to simplify the code in some places...

>+      if (isTopLevel) {
>+        (*aResult).top = (*aResult).bottom = 1;
>+        (*aResult).left = 3;
>+        (*aResult).right = 4;
>+      } else {
>+        (*aResult).top = 0;
>+        (*aResult).bottom = (*aResult).left = (*aResult).right = 1;
>+      }
[Trunk-only note: given separate menuitemcheck and menuitemradio appearance
parts it may simplify the code to create separate menuitem and menubaritem
appearances. It would also help to fix Windows 95 ;-) ]

>+    // XXXjgr We should ideally be caching these, but we wont be notified when
>+    // they change currently, so we can't do so easily. Same for the bitmap.
>+    int checkW = ::GetSystemMetrics(SM_CXMENUCHECK);
>+    int checkH = ::GetSystemMetrics(SM_CYMENUCHECK);
Do this in UpdateConfig() perhaps?

>+        // XXXjgr This will go pear-shaped if the image is bigger than the
>+        // provided rect. What should we do?
[Trunk note: all the more reason to implement this as a separate part!]

>+        COLORREF oldTextCol = ::SetTextColor(hdc, 0x00000000);
>+        COLORREF oldBackCol = ::SetBkColor(hdc, 0x00FFFFFF);
>+        ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCAND);
>+        ::SetTextColor(hdc, ::GetSysColor(aColor));
>+        ::SetBkColor(hdc, 0x00000000);
>+        ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCPAINT);
Nit: Seems to me that if you knew the solid background colour you could one
SRCCOPY.

>+        ::SetTextColor(hdc, oldTextCol);
>+        ::SetBkColor(hdc, oldBackCol);
Nit: I don't remember seeing it documented that you had to reset the colours
before deleting a DC.

> menu,
> menuitem {
>+  -moz-appearance: menuitem;
>   -moz-box-align: center;
>-  border: 1px solid transparent;
So, normal menuitems never have a border, right?

>+  min-width: 1px;
Any particular reason? [(menu)popup]
Attachment #197534 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 112

12 years ago
(In reply to comment #111)
> (From update of attachment 197534 [details] [diff] [review] [edit])
> sr=me for branch but see below for nits.
> 
> >Index: browser/base/content/browser.xul
> >Index: browser/components/bookmarks/content/bookmarksMenu.js
> Maybe you mentioned it in a comment that I've overlooked, but this looks like a
> different patch.

The changes here are absolutely *required* to make the bookmarks toolbar not
have a total fit trying to handle a border on the toolbar/toolbox. Without this,
it behaves even worse than usual.

> >+#navigator-toolbox {
> >+  border-bottom-width: 1px;
> >+  -moz-border-bottom-colors: ThreeDShadow;
> >+}
> Out of interest what's special about this toolbox?

This is a hack to make the content area running up against the toolbox not look
crap, which is entirely the result of the decision to not have a sunken border
on the content area.

> >+    // state == 1 iff this toolbar is the first inside the toolbox, which
> >+    // means we should omit the top border for correct rendering.
> Would a toolbox top margin of -1 to push the top border under the non client
> area be even hackier? It would seem to simplify the code in some places...

I'm not sure which is worse, to be honest - I don't like the code to alter the
rendering as it is, but a -1 margin doesn't sound any better.

> >+    // XXXjgr We should ideally be caching these, but we wont be notified when
> >+    // they change currently, so we can't do so easily. Same for the bitmap.
> >+    int checkW = ::GetSystemMetrics(SM_CXMENUCHECK);
> >+    int checkH = ::GetSystemMetrics(SM_CYMENUCHECK);
> Do this in UpdateConfig() perhaps?

I believe I already commented on this, but I left that out for now as the
UpdateConfig call is not made in all cases it should be, and I didn't wish to
try and fix that on branch. On trunk, I can work in a fix for that and actually
cache a few other things too.

> >+        COLORREF oldTextCol = ::SetTextColor(hdc, 0x00000000);
> >+        COLORREF oldBackCol = ::SetBkColor(hdc, 0x00FFFFFF);
> >+        ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0,
SRCAND);
> >+        ::SetTextColor(hdc, ::GetSysColor(aColor));
> >+        ::SetBkColor(hdc, 0x00000000);
> >+        ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0,
SRCPAINT);
> Nit: Seems to me that if you knew the solid background colour you could one
> SRCCOPY.

I can change it if you like, but I'm not sure I want to change any logic at this
stage.

> >+        ::SetTextColor(hdc, oldTextCol);
> >+        ::SetBkColor(hdc, oldBackCol);
> Nit: I don't remember seeing it documented that you had to reset the colours
> before deleting a DC.

I don't know if you are supposed to, but the code I've seen for this kind of
drawing does it. I can remove them if you want.

> > menu,
> > menuitem {
> >+  -moz-appearance: menuitem;
> >   -moz-box-align: center;
> >-  border: 1px solid transparent;
> So, normal menuitems never have a border, right?

Not in the CSS - all the borders and stuff is native because, well, it depends
on things CSS simply doesn't know a thing about. :)

> >+  min-width: 1px;
> Any particular reason? [(menu)popup]

That's in the original if you read up a few lines. ;)

Comment 113

12 years ago
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

Requesting approval on behalf of silver. It appears the css was reviewed by
neil, so it seems additional review is no longer needed by mconnor.

I don't see why this shouldn't go in after b2 is released (after all, there's
already been heavy and successful testing done with this patch), and the sooner
this patch is landed, the  more time there is to back it out in the unlikely
event serious regressions arise.
Attachment #197534 - Flags: approval1.8rc1?

Updated

12 years ago
Flags: blocking1.8rc1+
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

r=me, subject to appropriate updates to the patch.  

Clearing approval request until this gets trunk landing and baking.  This is
what we are doing for anything that isn't a blocker, and isn't low to zero
risk.  Landing on branch for testing and backing out is not acceptable at this
stage of the cycle.
Attachment #197534 - Flags: review?(mconnor)
Attachment #197534 - Flags: review+
Attachment #197534 - Flags: approval1.8rc1?

Comment 115

12 years ago
Oh for Pete's sake guys. The patch is for branch only. *Now* you're suddenly
saying a banch-only patch must land on trunk first! WTF?

Comment 116

12 years ago
James, the patches would have to be landed on the trunk sooner or later anyways.

Comment 117

12 years ago
No, they wont. Did you not read the bug? The current patch is nothing more than
a band-aid to make Firefox 1.5 not suck. See comment 94.

Comment 118

12 years ago
(In reply to comment #117)
> No, they wont. Did you not read the bug? The current patch is nothing more than
> a band-aid to make Firefox 1.5 not suck. See comment 94.
Ya, I read it...just forgot eveything that has been said in this bug.

James has a point, no one said anything a few weeks ago when he stated this is
for branch only and stand no chance of landing on the trunk becuase of
differerences. His builds with the patch included was tested by many people and
there are no bugs  left that any of us could find and we tested this with a fine
toothed comb with all different windows OSes, a lot of themes and fonts/font sizes.

Please land this and if anything nasty is found back it out, not talking about
backing it out a few weeks from now, just give it a day or two.

Comment 119

12 years ago
Created attachment 198735 [details] [diff] [review]
Trunk version of silver's patch

Though I, too, believe that this should really be strictly a branch patch, if
the "bandaid" requires testing on trunk, this should suffice.  I've tested it
at least a bit, and it both compiles and accurately emulates IE for Classic and
Luna.  Not sure about the other esoteric themes that people can test, but
that's why there are trunk testers, no?

Since the code is essentially the same (+ unbitrotting), methinks the flags
should carry over (correct me if I'm wrong and this requires another two-week
review).  Since the code on trunk changes rapidly, I recommend that this be
checked in fairly soon.  That will also give a bigger window for finding
regressions.

Silver, if you disagree with this patch being checked into trunk at all, even
if it's going to be checked out later, feel free to cancel the request for
review, etc.  But if this is the only way it's going to have a chance to get on
branch, I think this is worth it.
Attachment #198735 - Flags: superreview?(mconnor)

Updated

12 years ago
Attachment #198735 - Flags: superreview?(mconnor)

Updated

12 years ago
Attachment #198735 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 120

12 years ago
I strongly object to a few things that drivers have done here:

a) Waiting until we'd spent 2.5 weeks getting r/sr on a branch patch before
adding random conditions, like "land on trunk and we'll think about branch".
b) Requiring landing a branch-specific band-aid patch on trunk at all.

If drivers really are now requiring a trunk landing before branch, then you are
welcome to get it landed. You do not have my backing in the trunk landing, but I
wont object to it actually happening.

I have way too much crap going on right now to have to worry about drivers
pissing me about, so I want a straight answer right now that WONT CHANGE
TOMORROW - do you (drivers) require that this patch be landed on trunk before
being granted approval for branch? The only valid answers are "yes" and "no".

Comment 121

12 years ago
Silver, it's already landed (by timeless) and I'm watching the tree as we speak.
 Again, I agree with your sentiment, but if this is what's required for it to
get on branch I'm willing to do it.  Again, after the whole 1.5 mess is done a
real fix can happen.

Comment 122

12 years ago
Who gave sr for that to land [1]? Why didn't they or you say in this bug? Did
people just throw out common sense on this bug or what?

[1] Yes I can read Bonsai. I can also read Bugzilla flags. Inconsitency is bad.

Comment 123

12 years ago
That SR request was somewhat whimsical.  Neil was too tired to check it in, and
(as I had compiled it and it is virtually identical to your patch, save the
locations of the nsNativeThemeWin.* files and some bitrot) it had already
largely gone through the r+sr process.  If the tree burns, I'll (try) to fix it.

I apologize for any confusion.

Updated

12 years ago
Attachment #198735 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 124

12 years ago
After testing a build that has the patch built in it seems that the colour of
the menu text on hover stays black instead of turning into white.

Comment 125

12 years ago
(In reply to comment #124)
> After testing a build that has the patch built in it seems that the colour of
> the menu text on hover stays black instead of turning into white.

You're using Luna a presume?
Looking great on my Classic theme (winXP), good work guys!

Comment 126

12 years ago
(In reply to comment #125)
> You're using Luna a presume?
> Looking great on my Classic theme (winXP), good work guys!

Using a cusotm theme.

Comment 127

12 years ago
(In reply to comment #126)
> Using a cusotm theme.

It would be helpful to know which theme, and a download location if possible.

Comment 128

12 years ago
(In reply to comment #127)
> (In reply to comment #126)
> > Using a cusotm theme.
> 
> It would be helpful to know which theme, and a download location if possible.

It doesn't matter, because I've just tried with Luna and I get the same problem.

Comment 129

12 years ago
Caleb, I would have hoped you would compare with the native rendering... in Luna
at least, the menu text hover colour IS THE SAME as the non-hover colour. You
can configure it to be different in Apperances > Advanced, if you like, and that
works fine here (on my 1.5 branch build - no update available for trunk yet).

Comment 130

12 years ago
(I am assuming the trunk port of the patch was correct - I really don't have the
time to check)

Comment 131

12 years ago
(In reply to comment #129)
> Caleb, I would have hoped you would compare with the native rendering... in Luna
> at least, the menu text hover colour IS THE SAME as the non-hover colour. You
> can configure it to be different in Apperances > Advanced, if you like, and that
> works fine here (on my 1.5 branch build - no update available for trunk yet).

Yeah I was vague.

It seems that even in the custom theme I'm using (Royale Glass as part of the
crappy Longhorn Transformation Pack which I don't recommend installing) shows
the right behaviour in IE6 and the wrong one in Firefox.

Hovering over menu items in IE changes the text colour to white while in Firefox
it stays black.

I dug up that build you posted in the MozillaZine forums and it shows the same
behaviour.

In Appearance > Advanced the colour for "Selected Items" is white.

Comment 132

12 years ago
The text colour is entirely controlled by CSS anyway, and I only changed the bit
for menu items directly on a menu bar. Is it those top-level items or the ones
in popups? (screenshot!)

Comment 133

12 years ago
(In reply to comment #132)
> The text colour is entirely controlled by CSS anyway, and I only changed the bit
> for menu items directly on a menu bar. Is it those top-level items or the ones
> in popups? (screenshot!)

Top-level, bottom-level, ANY-level:
http://img13.imageshack.us/img13/4990/screenshot4yd.jpg

This font colour of the "Selected Items" property in the Appearance options
seems to control the font colour of these hovered items.

Comment 134

12 years ago
(In reply to comment #133)
> This font colour of the "Selected Items" property in the Appearance options
> seems to control the font colour of these hovered items.

Yes, that's correct. It all works perfectly over here (1.5 branch), and more to
the point, for the non-top-level items, was not changed by this patch (on
branch) anyway. It is possible something's changed on trunk that conflicts with
it, which is just another reason I stuck to branch for the patches.

Comment 135

12 years ago
Created attachment 198788 [details]
Menues on hover

It works ok here on an almost new profile (only NTT to see the build id)
I'm guessing it's a trunk issue since I'm seeing the same thing as Caleb. Other
than that, things are looking great!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051006
Firefox/1.6a1

Comment 137

12 years ago
It must be a very clever trunk issue, as it doesn't happen on the tinderbox
build I got. :P
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051007 Firefox/1.6a1
{Build ID: 2005100704}

Looks wonderful! Great work!
But, this can not use on Windows 9X. 
The GDI resource is consumed at once when displaying the part of the menu and
the toolbar increases and the reaction of UI dulls. 
75% GDI resource that was when Windows starts becomes 10% in less than no time. 
However, when this shuts the browser window, the GDI resource is released. 

Comment 139

12 years ago
I know Windows 9x is dodgy at the best of times, but I'm not quite sure how it
can persistently consume resources like that, when it destroys the bitmap,
objects, etc. all as soon as it is done drawing.

Does the resource usage level vary when you open or close menus or submenus?
Does it go down at all, for that matter (except closing the window)?
The GDI resource is consumed in addition and never liberated until the browser
window is shut. 
When the bookmark of the menubar that is sets of a lot of menu items is
displayed many times, the consumption of the GDI resource is reproduced. 

Comment 141

12 years ago
Ok, the bookmarks toolbar and menu are different... is the usage actually worse
than previously when you have the bookmarks toolbar hidden and only use normal
menus? Is it then made a lot worse by opening either bookmarks things?

Some proper resource usage numbers in various configurations would be good to
judge what it actually going on... if someone has the time and patience to try
all these configurations both before and after the patch, keeping the same
Firefox settings throughout:
  - Bookmarks toolbar off at startup, don't open any menus except to quit after
    recording the usage.
  - Same, but flick through all the menus *except* the bookmarks one, then
    record usage.
  - Same, but also flick through the bookmarks menu before recording usage.
  - This time start with the bookmarks toolbar on, and record usage without
    using any menus.
  - Same, but flick through all the menus *except* the bookmarks one, then
    record usage.
  - Same, but flick through all the menus before recording usage.

It would be advisable to disable or uninstall all extensions before gathering
any data, too, as they may adversly affect the resource usage.

That's 12 different samples in total, and more than that in restarts of Firefox,
so don't feel you have to do it if you don't - I'll be linking this comment on
MozillaZine and asking for any help they can provide in gathering this data.
The bookmark of the menubar is one remarkable mere example. 
The bookmark toolbar is not related directly. 
The basis is a repetition of the opening of the menu(context menu) and shutting it. 
+      // for this item. We will pass any nessessary information via aState,

that should be "necessary"...

Comment 144

12 years ago
http://tonglebeak.com/1008_branch_theme_aboutDialog_patches.zip

There's the 10/08 branch I built with silver's branch patch applied. In the
mozillazine thread, this build according to users who were affected on the
trunk, has no problems with GDI resource, so it's something just trunk related,
meaning the GDI problem shouldn't stop this patch from 1.5

Comment 145

12 years ago
If this is now in the trunk, I have one question: why do context menus have no
drop-shadows now that they are natively drawn (no, it's not bug 281709)?

Comment 146

12 years ago
Actually, it is precicely bug 281709. The menu itself does not include the
drop-shadow inside the window, it mearly tells Windows to apply one when it
appears - which is what bug 281709 is about. This bug, and native rendering in
general, is only about the contents of windows, not how they externally interact
with the OS.
Mr. Aaron,
Yes, that bild works fine on Windows 98SE. 
But, the problem on trunk caused with this patch is unsettled.
The possibility that the problem occurs in potential also on branch is had. 
The user of Windows 9x doesn't hope for the reproduction of the resource bug on 
Mozilla Suite 1.4 (Bug 204374). 
Napoleon said, 'I am hurrying up very much. Therefore, please report slowly.'
Blocks: 311798
I filed bug 311798. Maybe, this one is regression of this.

Updated

12 years ago
Flags: blocking1.8b5-
Flags: blocking-aviary2.0?
Flags: blocking-aviary1.5-
Flags: blocking-aviary1.0-

Updated

12 years ago
No longer blocks: 311798
Depends on: 311798

Comment 149

12 years ago
(In reply to comment #120)
> I strongly object to a few things that drivers have done here:
> 
> a) Waiting until we'd spent 2.5 weeks getting r/sr on a branch patch before
> adding random conditions, like "land on trunk and we'll think about branch".
> b) Requiring landing a branch-specific band-aid patch on trunk at all.
> I have way too much crap going on right now to have to worry about drivers
> pissing me about, so I want a straight answer right now that WONT CHANGE
> TOMORROW - do you (drivers) require that this patch be landed on trunk before
> being granted approval for branch? The only valid answers are "yes" and "no".

Then "Yes" :). For large scale changes such as this one, we have always required
patches like this to land on the trunk. The patch must then bake on the trunk so
any regressions can be addressed there before we will consider it for the
branch. This has been a long standing policy when we reach this point of the
release cycle that we've employed for many many years. It's not something new
nor should it be a surprise. The trunk is used to test risky changes, we don't
want to be doing our regression fixes directly on the branch this close to final
release. 

Comment 150

12 years ago
The problem is, it *was* a surprise. Both how long it took for anyone to do
anything, which meant it missed Beta 2 when it didn't need to miss it, and
secondly because I stated very clearly in this bug that I was doing a
branch-only patch and /not a single person objected or even hinted that it might
need to go onto trunk/. While you may not have changed you (undocumented, as
ever) policy, you still took me by surprise, and as such you now have a patch I
wrote checked-in to trunk that is not going to get any support or fixing from me.

Comment 151

12 years ago
Created attachment 199109 [details] [diff] [review]
Fix the bad assumptions in browser.css with some more assumptions

This is basically just an updated version on attachment 197534 [details] [diff] [review], but with a fix
for bug 311798 in (so will not regress that on branch).

Whether or not the patch in bug 311798 gets review, this is here so I don't
lose it, and other people can see just how evil it is. Should the patch in bug
311798 get review and branch approval, and attachment 197534 [details] [diff] [review] does too, it will
be *this* attachment that lands.

Comment 152

12 years ago
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

Silver, I think we should get this onto the branch on Tuesday or Wednesday.
I'll huddle with the other drivers tomorrow and see if we can get this
approved.
Attachment #197534 - Flags: approval1.8rc1?

Comment 153

12 years ago
From IRC:

<Silver> Anyway, I need to do other stuff. And to top everything off, I'm away
from tomorrow morning until Friday.

So I'm not sure this is the best time to land it.  On top of that, Silver's
current feedback line patch doesn't work quite properly (at least on trunk,
which is identical except for a line that I had to move up two lines), whereas
the one that has SR right now does.  If we're going to land, we should combine
that one with this one and (hopefully) Tonglebeak's patch for changing the
location bar flex in browser.xul, rather than use his 'branch patch + fix bad
assumptions' patch.

We should also wait until we've made sure that there are no GDI resource
concerns from Win9x and ME users on branch.  While they don't appear to, it's
possible that canvas caused this, so we need to confirm this with the canvas builds.

So, while it's conceivable that this could land on Tuesday or Wednesday, it'd be
much more doable by the time Silver was back (on Friday).

Updated

12 years ago
Comment on attachment 199109 [details] [diff] [review]
Fix the bad assumptions in browser.css with some more assumptions

+#personal-bookmarks {
+  min-width: 21px; /* size of the chevron button */
+}
+

what about pinstripe?

Comment 155

12 years ago
Re comment 153: I've already fixed the bookmark toolbar foder issue.

Re comment 154: trivial to fix, what other themes in CVS need that hack?

(and why the heck aren't they all based on one with platform overrides?)
Pinstripe is a totally different theme (default on mac), so platform overrides
wouldn't do. Gnomestripe probably doesn't need the fix, IIRC it doesn't override
browser.css.

Hope that helps ;)

Comment 157

12 years ago
Ah, ok. I can only see a winstripe and pinstripe folder in
mozilla/browser/themes, so just those two got updated and hopefully that'll
cover all the themes used by default.

Also, re availability: I am away from my computer from 10 minute until 2pm
Friday, UK time (BST). I will be able to get bugmail, and remote access to my
computer, on Wed and Thu for the exact periods of 9:30am to 5:30pm UK time (BST).

I *can* land during that time, and can watch the tree and so on, but only if
everything (approvals, etc.) is ready on Wed or Thu *morning*. (If you want
someone else to land, make sure to update the final patch I attached with the
fix document in bug 311798 and the pinstripe update. :) )

Updated

12 years ago
Whiteboard: [needs approval]

Comment 158

12 years ago
Noticed "toolbar, menubar {border-style: none !important}" to remove toolbar
separators is broken on the trunk probably since 2005-10-06.
This is visible using the default winstripe theme.

Comment 159

12 years ago
(In reply to comment #158)
> Noticed "toolbar, menubar {border-style: none !important}" to remove toolbar
> separators is broken on the trunk probably since 2005-10-06.
> This is visible using the default winstripe theme.
> 
this works
/* Remove seperator between toolbars*/
#toolbar-menubar {
 min-height: 12px !important;
 padding: 0px !important;
 margin: 0px !important;
 border: none !important;
}
#nav-bar {
 border: none !important;
 padding: 0px !important;
}
#PersonalToolbar {
 border: none !important;
}
#navigator-toolbox {
  border-bottom-width: 0px !important;}

Comment 160

12 years ago
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

This was going to be landed by mconnor earlier today.  His build environment
didn't come together in time, though.  Plussing per triage meeting.  Will land
on behalf of mconnor on behalf of patch author.
Attachment #197534 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 161

12 years ago
Comment on attachment 197534 [details] [diff] [review]
Updated with comments [checked in]

Landed on the Mozilla 1.8 branch.

Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.268.2.5; previous revision: 1.268.2.4
done
Checking in browser/components/bookmarks/content/bookmarksMenu.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarksMenu.js,v  <-- 
bookmarksMenu.js
new revision: 1.48.2.5; previous revision: 1.48.2.4
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <-- 
browser.css
new revision: 1.17.2.4; previous revision: 1.17.2.3
done
Checking in gfx/src/windows/nsNativeThemeWin.cpp;
/cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.cpp,v  <-- 
nsNativeThemeWin.cpp
new revision: 3.45.4.1; previous revision: 3.45
done
Checking in gfx/src/windows/nsNativeThemeWin.h;
/cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.h,v  <-- 
nsNativeThemeWin.h
new revision: 3.16.20.1; previous revision: 3.16
done
Checking in layout/style/nsCSSKeywordList.h;
/cvsroot/mozilla/layout/style/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.74.6.3; previous revision: 3.74.6.2
done
Checking in layout/style/nsCSSProps.cpp;
/cvsroot/mozilla/layout/style/nsCSSProps.cpp,v	<--  nsCSSProps.cpp
new revision: 3.130.6.3; previous revision: 3.130.6.2
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <-- 
autocomplete.css
new revision: 1.5.8.1; previous revision: 1.5
done
Checking in toolkit/themes/winstripe/global/global.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/global.css,v  <--  global.css
new revision: 1.8.4.1; previous revision: 1.8
done
Checking in toolkit/themes/winstripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/menu.css,v  <--  menu.css
new revision: 1.5.2.5; previous revision: 1.5.2.4
done
Checking in toolkit/themes/winstripe/global/popup.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/popup.css,v  <--  popup.css
new revision: 1.4.2.4; previous revision: 1.4.2.3
done
Checking in toolkit/themes/winstripe/global/toolbar.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/toolbar.css,v	<-- 
toolbar.css
new revision: 1.7.2.3; previous revision: 1.7.2.2
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--	nsILookAndFeel.h
new revision: 1.47.2.1; previous revision: 1.47
done
Checking in widget/src/windows/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/windows/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.50.2.1; previous revision: 1.50
done

I'll respin our Firefox and Thunderbird build systems this evening.
Attachment #197534 - Attachment description: Updated with comments → Updated with comments [checked in]

Comment 162

12 years ago
fixed1.8
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

12 years ago
Whiteboard: [needs approval]

Comment 163

12 years ago
Did you deal with the proposal of comment #159?

Comment 164

12 years ago
Did you deal with the proposal of comment #159? Is this an issue on the branch
at all?

Comment 165

12 years ago
(In reply to comment #164)
> Did you deal with the proposal of comment #159? Is this an issue on the branch
> at all?

I landed the attachment that was cited in the bug.  Nothing else.
Chase, please read comment 157 (esp. the last paragraph).
Status: RESOLVED → REOPENED
Keywords: fixed1.8
Resolution: FIXED → ---

Comment 167

12 years ago
(In reply to comment #166)
> Chase, please read comment 157 (esp. the last paragraph).

Sure, I read it.  No offense meant but what's that mean to me?  I don't have
time to work on this bug anymore than I already have.  I landed the patch
(repeating, here) on behalf of mconnor on behalf of the triage meeting.  mconnor
can sort out the rest tomorrow.

Comment 168

12 years ago
Argh. Argh. Argh. If you're going to land something on someone else's behalf, 
and least get the damn thing right.

You've missed both the pinstripe fix, and regressed bookmark dragging per bug 
311798 (see attachment 199109 [details] [diff] [review]). Way to go.

I hope someone's planning to fix both ASAP.

Comment 169

12 years ago
(In reply to comment #164)
> Did you deal with the proposal of comment #159? Is this an issue on the branch
> at all?

Was this comment meant for me?
I´ve edited userchrome.css but it doesn´t work.
Haven´t tried this on the branch

Comment 170

12 years ago
That comment was not meant for you, Judha. To be honest, no-one here should 
care about what you said in comment 158, because it is obvious that this bug 
changes the behaviour of it - that's the entire point of this bug. It is also 
not the responsibility of people on the bug to tell you how to make your CSS 
work again, you should instead use the MozillaZine forums or a similar place.

Also, Chase, marking this bug FIXED was completely and utterly the wrong thing 
to do. If you'd actually read the bug before commiting random patches on it, 
you'd know this, and know why - I would suggest drivers be way more careful 
about forcing stuff through without understanding it.

Comment 171

12 years ago
James, so how about fixing what was done. If this doesn't happen pretty damn
fast, it's not going to happen. Drivers (and chase) were working to try to give
this patch (set of patches) a shot at making Firefox 1.5. You were away, our
time was short. If you're back, please help fix it. If it doesn't happen today,
it's very likely that we won't be able to take this. 

Comment 172

12 years ago
(In reply to comment #171)
> James, so how about fixing what was done. If this doesn't happen pretty damn
> fast, it's not going to happen. Drivers (and chase) were working to try to give
> this patch (set of patches) a shot at making Firefox 1.5. You were away, our
> time was short. If you're back, please help fix it. If it doesn't happen today,
> it's very likely that we won't be able to take this. 

I've explained my situation - I have 1.5 hours max access to my box. I am not
back, and what is so special about today? It's still 2 weeks 'til RC1...

I need approval for attachment 199109 [details] [diff] [review] before I can fix this, and I do NOT want
to think what the CVS conflicts on branch are going to be like in my tree...
(you've made it MUCH harder for me to land).

Updated

12 years ago
Attachment #199109 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199109 - Flags: review?(mconnor)
Attachment #199109 - Flags: approval1.8rc1?

Comment 173

12 years ago
Ok, local tree is ok. Waiting for approval to land...

Comment 174

12 years ago
Should we be creating new bugs for issues that are discovered?  I just noticed
that the 3D border around the "Object - DOM Node" in the DOM Inspector is
missing (at least the top portion of the border) using the latest branch build.

BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051012
Firefox/1.4.1 ID:2005101203

~B

Comment 175

12 years ago
That'll be DOM I assuming what borders exist on the header thing, probably. And
frankly, you can do whatever you want - everyone else is! (Note that *standard
practise* is to file followup bugs about minor issues like that.) Drivers need
to make a call and set the flag. I will land or not according to that. I will
not make any more changes to anything on this bug, as drivers are already making
up random deadlines that are almost impossible to meet.

Comment 176

12 years ago
Comment on attachment 199109 [details] [diff] [review]
Fix the bad assumptions in browser.css with some more assumptions

james, I'm giving you approval to land. we can get these additional reviews
after landing. thanks for jumping right in this morninng. I know we have two
more weeks, but this is the kind of change that really needed to happen before
beta 2 and we're making a very rare exception in taking something this large
this late in the game so we wanted it in as soon as was possible.  Again, sorry
for making things more difficult. We were just trying to give this patch a
chance.
Attachment #199109 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 177

12 years ago
Ok, will land as soon as CVS has cleaned up the mess (sigh) that fast-update
just created. Please note: I CAN NOT WATCH ANY TREE AFTER 5:30pm BST - that is
40 minutes from now. If anything occurs after that, someone else will need to
handle it.

Updated

12 years ago
Depends on: 311675

Comment 178

12 years ago
this bug appears to have caused regression Bug 311675 which is now on the branch.

Comment 179

12 years ago
Which, if you read my patch and this bug, is dealt with by the patch I'm just
about to land (it sets a min-width on the toolbar, which although not ideal [the
flex change would be better], it was what we came up with at the time).
Comment on attachment 199109 [details] [diff] [review]
Fix the bad assumptions in browser.css with some more assumptions

The drag-drop indication changes are a bit hairy, maybe that idea of altering
the border on the label would be simpler (or you could implement native drag
feedback... maybe not). Either way try to use > in style rules wherever
possible - .bookmark-item[dragover-bottom="true"] .menu-drop-marker-box causes
the style system to check all ancestors of all .menu-drop-marker-box elements
in case they're a bookmark item.

>Index: gfx/src/shared/nsNativeTheme.cpp
>+  mMenuActiveAtom = do_GetAtom("_moz-menuactive");
>Index: gfx/src/shared/nsNativeTheme.h
>+  nsCOMPtr<nsIAtom> mMenuActiveAtom;
I don't see these being removed from the windows/gtk2 versions, which would
have been the point.

>+      // for this item. We will pass any necessary information via aState,
Ah yes, I only just noticed the misspelling in the previous patch :-)
Attachment #199109 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199109 - Flags: superreview+
Attachment #199109 - Flags: approval1.8rc1?
Attachment #199109 - Flags: approval1.8rc1+

Comment 181

12 years ago
If you read the comments, both issues have been addressed in the patch I'm still
trying to check in (using >*> and the spelling).

Comment 182

12 years ago
Ok, checked in. Still updating and checking I've not missed a bit, after the
mess fast-update made, though.

Comment 183

12 years ago
Landed one bit I missed. I think it is all in now, and I'm going off-line until
9:30am BST tomorrow morning. Although I'll be around then, I'd suggest anything
that needs urgent fixing is done today by someone else (missing part of the
check-in demonstrates what state I'm in this week).

Updated

12 years ago
Attachment #199109 - Flags: approval1.8rc1? → approval1.8rc1+

Updated

12 years ago
Attachment #199109 - Flags: review?(mconnor) → review+

Comment 184

12 years ago
Also caused bug 312205, I think.
Created attachment 199330 [details]
screenshot

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051012
Firefox/1.4.1 ID:2005101210 (win2k/classic)

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20051012+0932&maxdate=20051012+0940&cvsroot=%2Fcvsroot

09:32 and 09:40 checkins caused 2 regressions:
1. vertical align:middle for menu buttons changed to :top ?
2. favicons aren't resized anymore
(In reply to comment #181)
>If you read the comments, both issues have been addressed in the patch I'm still
>trying to check in (using >*> and the spelling).
Sorry, I'm still backlogged with bugmail, so I could easily have missed or
overlooked the comment to which you appear to refer. The spelling was not an
issue per se, I was just glad someone was sufficiently awake to spot it.
Attachment 199109 [details] [diff] caused a signficant number of regressions by changing the
anonymous content structure of menu.xml, especially the "menu-iconic" binding:
many instances of CSS in the tree use selectors of the form

http://lxr.mozilla.org/mozilla/source/mail/themes/pinstripe/mail/compose/messengercompose.css#760
or
http://lxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/browser.css#138

We are going to need to back out these changes to menu.xml. We need to decide
whether this means backing out the entire checkin here, or making targeted
changes to menu.xml that do not affect this style of selector.
Created attachment 199378 [details]
screenshot

Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1
{Build ID: 2005101216}

Menu arrow is not displayed in the rightmost of menuitem. 
It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem'
,in a word, almost all themes. 
This happens only by branch. It doesn't happen in trunk.

Comment 189

12 years ago
(In reply to comment #188)
> Created an attachment (id=199378) [edit]
> screenshot
> 
> Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1
> {Build ID: 2005101216}
> 
> Menu arrow is not displayed in the rightmost of menuitem. 
> It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem'
> ,in a word, almost all themes. 
> This happens only by branch. It doesn't happen in trunk.

Aren't the arrow there to show that there's a submenu for the menuitem? I don't
see how there's a problem here (and I assume you meant "now displayed" instead
of "not displayed," else I'd think you were cuckoo :P)

Comment 190

12 years ago
(In reply to comment #189)
> (In reply to comment #188)
> > Created an attachment (id=199378) [edit] [edit]
> > screenshot
> > 
> > Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1
> > {Build ID: 2005101216}
> > 
> > Menu arrow is not displayed in the rightmost of menuitem. 
> > It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem'
> > ,in a word, almost all themes. 
> > This happens only by branch. It doesn't happen in trunk.
> 
> Aren't the arrow there to show that there's a submenu for the menuitem? I don't
> see how there's a problem here (and I assume you meant "now displayed" instead
> of "not displayed," else I'd think you were cuckoo :P)

Not displayed *in the rightmost of menuitem*.  In other words, they are supposed
to be right justified.

Comment 191

12 years ago
Well, maybe if he disables his Flatmenus extension, there will be no problem :)

Updated

12 years ago
Depends on: 312274

Comment 192

12 years ago
Created attachment 199394 [details]
screenshot (wrong menu color)

I'm still not convinced that this patch gets the menu color always right. If
you change the menu color in the display properties, Firefox simply ignores
this change. While you can argue about the color of the menu bar (which
Explorer also sees more as toolbar than as menu), the menus itself are for all
apps I've tested consistently in the chosen color. And this is not after some
wild themeing, but after a change even Joe User might make intentionally. BTW
this seems to be reproducible under both Classic and Luna (see my comment #73
above).

Comment 193

12 years ago
(In reply to comment #187)
> Attachment 199109 [details] [diff] [edit] caused a signficant number of regressions by 
changing the
> anonymous content structure of menu.xml, especially the "menu-iconic" 
binding:
> many instances of CSS in the tree use selectors of the form

If you refuse to have the menu.xml changes, you can't have the entire thing. 
It is required to prevent regressing the bookmark dragging markers.

You are welcome to fix the other places that make silly assumptions about the 
anonymous structure of menu.xml, I don't care either way.

Updated

12 years ago
Depends on: 312272

Updated

12 years ago
Depends on: 312194

Comment 194

12 years ago
(In reply to comment #187)
> many instances of CSS in the tree use selectors of the form
> 
http://lxr.mozilla.org/mozilla/source/mail/themes/pinstripe/mail/compose/messen
gercompose.css#760
> or
> 
http://lxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/browser.
css#138

Since the only places that are broken are the emoticon popup in Tbird compose 
(the *other* popups are fine...), and the bookmarks menu favicons (plus a 
little bit in the toolkit themes, which seems to not have been very important 
anyway, as it hasn't made any difference now it doesn't apply), why not let 
someone fix it to cope? I'm quite happy to scan the tree for CSS that touches 
any of the CSS classes I've moved about, and update them, but it's your call.

I know that changing the anon XBL is 'scary', but that doesn't mean it 
shouldn't be done - it only serves to highlight the shortfalls in what you 
lovingly call the 'platform'.

If there has still been no acceptance to actually make this work, I will back 
out every single part of this bug from branch on Friday, at around 4PM BST.

Comment 195

12 years ago
Seeing that this is also put into the beta/rc releases of FF 1.5, the
regressions caused by this very late checkin of so many changes, particulary the
menu.xml, which also causes regressions for allmost all third party themes, I
respectfully ask to back this out, or at least the menu.xml stuff.

Note that the changes required to themes because of this change may well make
the themes incompatible with the older builds and older releases.

This bug is all about UI candy, and should not have such dramatic regressions
just before a major release! 

At the very least, provide some help for those poor themers, having to fix their
themes again, especially for the 'right arrow' not being right aligned...

Comment 196

12 years ago
Forgive me my ignorance, but shouldn't you draw a menu background for non-flat
menus as well (from attachment 199109 [details] [diff] [review]):
+    case NS_THEME_MENUPOPUP:
+      if (mFlatMenus) {
+        ::FillRect(hdc, &widgetRect, (HBRUSH) (COLOR_MENU+1));
+        ::FrameRect(hdc, &widgetRect, ::GetSysColorBrush(COLOR_BTNSHADOW));
+      } else {
+        ::DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_RECT | BF_MIDDLE);
+      }
+      return NS_OK;

Removing -moz-appearance: menupopup from the menus' styles corrects the issue
(then we're back with the CSS classes which correctly apply the menu colors).

Comment 197

12 years ago
Yes, though the chance to fix that was weeks ago, when it was under review. 
I'm happy to fix it, but drivers, who knows...

Comment 198

12 years ago
James, if you can fix the regressions this introduced, there's still a chance we
 can keep this on the branch. 

Comment 199

12 years ago
The regressions I see in testing (maybe all the same issue) are that favicons
aren't sized properly, menu submenu expand arrows are not right aligned, the
smiley images are missing from the smiley toolbar menu in HTML compose in
Thunderbird, and one I'm not terribly concerned about, that the menus are
aligned to top rather than middle when you make the menubar taller by adding,
say, a large icon. 

Comment 200

12 years ago
I will attempt to fix all those issues by tomorrow evening (I'm only at my 
computer from about 1PM BST tomorrow). Watch this space.

Comment 201

12 years ago
Solved some of the menu issues with my 'non-native themes' as follows:
menuitem > stack, 
menu > stack,
.menu-inner-content { -moz-box-flex: 1; }

This makes these extra boxes behave, but may be there are better ways to this?

Comment 202

12 years ago
To fix the favicon sizing problem I changed the selectors from browser.css that
 Ben S pointed out.  I put the altered rule in my userChrome.css and all of the
problem favicons are 16 x 16 like they should be.

Here's the selector I used:
.bookmark-item > stack > .menu-inner-content > .menu-iconic-left >
.menu-iconic-icon 

I think a similar change could be done for Thunderbird's smiley problem.
http://lxr.mozilla.org/mozilla/search?string=menu-iconic-left There are many
more CSS selectors affected than just bookmark icon sizes and tbird smilies.

I do not think that the XBL structure change is acceptable on the branch. We
need to at least back out that part of the patch and figure out a different
workaround for whichever bug it was solving.
Created attachment 199429 [details] [diff] [review]
Patch to fix the CSS selectors

Fix for the CSS selectors. 

[13-10-2005 18:31] <Hannibal> suppose I succeed in building TB
[13-10-2005 18:31] <Hannibal> and the patch works
[13-10-2005 18:31] <[Not]Silver> Yes...
[13-10-2005 18:31] <Hannibal> can I just attach it and set r? + sr?
[13-10-2005 18:32] <[Not]Silver> Set a? too, and say that I approved it as part
of the regression fixing.

bsmedberg requested me to put this up already. I'm still trying to get TB to
build and test these fixes myself. Requesting r, sr and a before having tested
it because several people on the bug stated that similar fixes worked for them.
Attachment #199429 - Flags: superreview?
Attachment #199429 - Flags: review?
Attachment #199429 - Flags: approval1.8rc1?
Even if you patch all the in-tree uses, the problem remains for extension
authors, theme creators and anyone else relying on the old structure. I really
don't think such a change is a good idea this late in the game, especially after
releasing two betas with the previous behavior.

Comment 206

12 years ago
Well, this patch certainly breaks all my themes.

Mostly Crystal:

http://gamergod.users.btopenworld.com/crystal.JPG


Mrtechs Local install

http://gamergod.users.btopenworld.com/mrtech.JPG
http://gamergod.users.btopenworld.com/mrtech2.JPG

Safire:

http://gamergod.users.btopenworld.com/bk.JPG

Hope this helps in your bugfixing.

Comment 207

12 years ago
(In reply to comment #205)
> Even if you patch all the in-tree uses, the problem remains for extension
> authors, theme creators and anyone else relying on the old structure. I really
> don't think such a change is a good idea this late in the game, especially after
> releasing two betas with the previous behavior.

That's one of the reasons we have the 1.4.1 internal version, which will be
bumped to 1.5 and make all the themes incompatible anyway.
> That's one of the reasons we have the 1.4.1 internal version, which will be
> bumped to 1.5 and make all the themes incompatible anyway.

Except for those marked as compatible up to the trunk (1.6a1 or 1.6 on AMO), and
they probably still work on the trunk too.
(In reply to comment #206)
> Well, this patch certainly breaks all my themes.
> 
> Mostly Crystal:
> http://gamergod.users.btopenworld.com/crystal.JPG
> Mrtechs Local install
> http://gamergod.users.btopenworld.com/mrtech.JPG
> http://gamergod.users.btopenworld.com/mrtech2.JPG
> Safire:
> http://gamergod.users.btopenworld.com/bk.JPG
> Hope this helps in your bugfixing.

I should think that theme authors *will* have to fix themselves. They had to do
that anyway, and as Caleb said, the version number will still be in their way
*anyway*. Lastly, let's not forget that without this patch, the *default* theme
will look horriffic on a large amount of Windows systems. As will quite a number
of the available themes out there.
(In reply to comment #207)
> That's one of the reasons we have the 1.4.1 internal version, which will be
> bumped to 1.5 and make all the themes incompatible anyway.

Just because there is an easy way to indicate that changes were made that could
potentially break extensions/themes doesn't excuse making such changes late in a
release cycle. Yes, they already have to bump up their version numbers and
ensure compatibility, but that doesn't mean it's OK to throw in last minute
changes that increases their burden.

Comment 211

12 years ago
This late in the game, I hoped that following the branch I only had to change
the version number so that the themers and extensionmakers (extenders?) are able
to very quickly release new versions, thoroughly tested and all, because of the
availability of beta1 and beta2. 

But this change, changes a lot within one of the more core widgets of the
system, forcing us to recode, retest, analyse and fix issues, retest again, all
in a very short timeframe before 1.5 is released.

As far as I understand the main part of this bug is indeed needed, but the
change to menu.xml, adding stack, .menu-content-inner and other stuff, was a
late addition to these series of patches...

I am not convinced that this late change itself is really required, and that
there are no other ways to solve. Please remember there are many other bugs with
complete and tested patches put on hold to make sure 1.5 will be very stable
release. Allmost 100.000.000 people will be very disappointed if it may look
nice on Windows, but that all themes and extensions are broken and released very
late, let alone any other to be detected regressions caused by this.

Please note that the last patch introducted very complex selectors:
.menulist-menupopup > menuitem > * > * > .menu-iconic-left,
This will for sure have a performance impact!

The last patch really shows the kind of mess that is caused by this all...

Comment 212

12 years ago
Well, there's a CSS-only patch in bug 311798 that has r+sr.  I know Neil didn't
like it because it mixed a few types of selectors, but taking any of them out
makes the patch useless (I've tried), probably due to either CSS weight or
position, and I don't want to use ! important because (as I recall) that sucks
lots for anyone trying to modify the CSS after the fact (e.g. in
userChrome.css).  Besides, I don't want the changes to apply to all of them...

The patch produces slightly different behavior, but doesn't change any XML files
or whatnot.  If you want to back out the menu.xml part of the patch, there IS an
alternative.  Fixing this one will mean that the behavior is identical for users
coming from 1.0, though.

Comment 213

12 years ago
(In reply to comment #211)

> Allmost 100.000.000 people will be very disappointed if it may look
> nice on Windows, but that all themes and extensions are broken and released very
> late, let alone any other to be detected regressions caused by this.

I would be very surprised if the majority of Firefox users use a different theme then the default theme.

IMO its more important to have the default Theme look great on all OS Versions than to maintain 
compatibility with 3rd party Themes. 

Comment 214

12 years ago
So alot of themes broke. What is the fuss all about, really?

Do you realize how many times core changes were made that broke extensions?
Many. So why should this be treated any differently? It's up to the theme
authors to be compatible with Firefox, not the other way around, so really the
complaining about "omg this breaks 3rd party themes" shouldn't be taken
seriously, as it's the theme author's responsibility to keep up.

Comment 215

12 years ago
> So alot of themes broke. What is the fuss all about, really?

The fuss is justified given where we are in the release cycle.  The same fuss
would be reasonable if it were coming from extension authors.  I, for one, agree
with Alfred.

Comment 216

12 years ago
(In reply to comment #214)
> So alot of themes broke. What is the fuss all about, really?

The fuss is that they're being broken *again* in the cycle less than 2 weeks
before RC1 not at some random point during alpha or beta. b1 and b2 were both
advertised towards these developers as being at least mostly stable, so they
could update their themes and extensions. Expecting not to be broken at the last
minute, isn't unreasonable at all. Particularly themes that were *already* made
compatible with 1.5b2.

Comment 217

12 years ago
One thing it might be good to consider is we're not choosing between a nice and
bug-free default theme and breaking some themes. We're choosing between a buggy,
less-than-100%-Windows-compliant default theme and breaking some themes. I would
pick the second alternative any day of the week.
I'd like to ask everyone to just back off from the advocacy stances and let this
go forward.  James has stepped up and contributed a large amount of great work,
and he's been flamed from many directions for his trouble.  This is a fix we
want, and while the timing is bad, for a number of reasons, we don't want to
just back everything out.

If there is a better way of fixing the regressions (not requiring the changes to
menu.xml) then great, but we're going to drive this in for the branch if at all
possible.  API-like compatibility from beta onward is a great principle, but
there is always a case where breaking that is worth it.  

From a product standpoint, this change is needed to better serve 35-40% of our
users, whereas a much smaller group (1% is the estimate) actually download and
install themes.  I would much rather ask that the theme authors do a bit of
extra work in the next few weeks before the release than ask all of our non-Luna
windows users  to live with a non-standard menu look and feel until Firefox 2.

Updated

12 years ago
Depends on: 312390

Comment 219

12 years ago
This caused regressions for Thunderbird. Most notably: Bug 312390 and Bug 312274

Comment 220

12 years ago
(In reply to comment #218)
Take a position and stick with it, seriously. First it was don't want it on the
branch until trunk testing, now its "we want it on branch" pretty much no matter
what. Even though there's some sharply conflicting comments (Comment 171 vs.
Comment 203). Its almost RC1, so let's break the world with a non-trivial
change... Sigh.

Comment 221

12 years ago
If you're not working on code related to this bug, please stop spamming the bug.
The right people are already involved here and we don't need the extra noise.
Unless you're writing code to improve this, reviewing code, or a member of the
driver team managing this release, please take your comments to mozillazine or
the newsgroup. Thanks.

Comment 222

12 years ago
I've one question about the menu.xml changes. Wouldn't it be possible to do
without <stack> (it would still need changes to the themes, but this would mean
less elements)? I mean say for example in the <menu> binding it could be
something like:

        <xul:box class="menu-inner-content menu-drop-marker-box">
          <xul:label class="menu-text" flex="1"
xbl:inherits="value=label,accesskey,crop" crop="right"/>
          <xul:hbox anonid="accel">
            <xul:label class="menu-accel" xbl:inherits="value=acceltext"/>
          </xul:hbox>
          <xul:hbox align="center" class="menu-right" chromedir="&locale.dir;"
                    xbl:inherits="_moz-menuactive,disabled">
            <xul:image/>
          </xul:hbox>
        </xul:box>
      <children includes="menupopup"/>

Comment 223

12 years ago
(In reply to comment #222)

then again, there's no outline-(left/right/top/bottom). Sigh. Forget what I said.

Comment 224

12 years ago
Created attachment 199518 [details]
Firefox vs IE6 SP2 with WindowBlinds

This new patch reverts the menu style to Win2K/Win9x style when using
Stardocks' Windowblinds 4.6.  

I use Windows XP SP2 with Windowblinds 4.6, and this patch makes the themes
considerably more ugly (and look anything but native to users with
WindowBlinds).	

IE 6.0 SP2 is on the right, Oct 13th Firefox branch on the left.

Comment 225

12 years ago
afaik this isn't much different from it was with firefox 1.0, am i right?

therefore, i would say due to the fact that is doesn't make it much worse than
before, we should ignore that and create an additional bug to get compatability
with the 3rd party window styles.

Comment 226

12 years ago
Created attachment 199535 [details]
comparing firefox to IE with WinXP Royale theme

using Windows XP theme "Royale" (available from
http://www.microsoft.com/nz/windowsxp/downloads/bliss/newbliss.aspx ) and
default Firefox theme

firefox 20051013 on left, IE6 on right

Comment 227

12 years ago
(In reply to comment #226)
See comment #89.

----

In 20051013 toolbars with no icons on collapse to 1px high when there are no
icons on them (for example, when adding a new toolbar or moving all icons off
the navigation toolbar).

Comment 228

12 years ago
I think the correct way to do this is to first read the Windows registry to
check if the user is in Windows Classic mode or if visual styles is applied.
Under Current User, look under "Control Panel\\Appearance" if "Current" has a
value in it. If yes, then he is in Windows Classic. If no, then styles are
applied (i.e., he is using either Luna or Royale, or whatever themes in the future).

We should then let the Firefox theme handle the look of the menus if the user is
in Windows Classic, and let the OS handle it if window styles are applied.

This will also give theme developers more room to define how the menus will look
under their theme if the user is in classic, while leaving the look to the OS in
either Windows XP style or Media Center Style.

Comment 229

12 years ago
(In reply to comment #227)
> (In reply to comment #226)
> See comment #89.
See comment #196 for why this patch might get it wrong as well.

(In replay to comment #228)
Using SystemParametersInfo instead of accessing the registry is indeed the
preferred way (since registry locations might change). Furthermore the
difference between appearances is not a binary choice between Luna/Royale/etc.
and Classic. You could e.g. be using non-flat menus with styled elements (which
is why there are reports such as comment #226).

Comment 230

12 years ago
Please redirect any 'non-native theme' discussion to:
   Bug 311798 Feedback line in bookmark menu during drag-and-drop is gone
That is the place where the menu.xml change took place, without any reviews BTW.
Assignee: silver → nobody
Status: REOPENED → NEW
QA Contact: themes

Comment 231

12 years ago
Created attachment 199549 [details]
screenshot of 20051013 firefox with black menu bg classic Windows theme

Reporting that while the checkmarks and arrows behave correctly, the colours
are not respected even as much as they were before the patch.

Classic theme on Windows XP, Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8b5) Gecko/20051013 Firefox/1.4.1
Bug 312247 seems to be caused by this bug :?

Updated

12 years ago
Assignee: nobody → silver

Comment 233

12 years ago
I have backed out all of this bug which has landed on the 1.8 branch. There are
a few reasons for this, including:
  - Some regressions that are not easy to fix
  - Signal-to-noise ration on relvant bugs being so bad actually finding what
    needs fixing is impossible.
  - Certain ignorant and annoying comments by people I wont name.

Any comments about this back-out DO NOT BELONG ON THIS BUG. Either flame an
appropriate newsgroup or forum, or e-mail me privately with USEFUL comments.
Drivers wishing to lynch me or otherwise can find me on IRC as usual.
Flags: blocking1.8rc1+

Updated

12 years ago
Flags: blocking1.8rc1+

Updated

12 years ago
Flags: blocking1.8rc1+

Updated

12 years ago
Flags: blocking1.8rc1+

Comment 234

12 years ago
(In reply to comment #227)
> In 20051013 toolbars with no icons on collapse to 1px high when there are no
> icons on them (for example, when adding a new toolbar or moving all icons off
> the navigation toolbar).

That is bug 265798, for which I wrote a patch yesterday. Although I didn't
realize it affects the main navigation bar too, I will need to change it
slightly in light of that.

Updated

12 years ago
Attachment #196472 - Attachment is obsolete: true

Updated

12 years ago
Attachment #197534 - Attachment is obsolete: true

Updated

12 years ago
Attachment #199109 - Attachment is obsolete: true

Updated

12 years ago
Attachment #199429 - Flags: approval1.8rc1?

Updated

12 years ago
Flags: blocking1.8rc1+ → blocking1.8rc1-

Updated

12 years ago
Depends on: 312298

Comment 235

12 years ago
I downloaded the build with the patch applied, and the menus don't seem to fade
or slide like native ones...is this planned for later, or not at all?

Comment 236

12 years ago
(In reply to comment #235)
> I downloaded the build with the patch applied, and the menus don't seem to fade
> or slide like native ones...is this planned for later, or not at all?

As the bug summary says, this bug is for rendering the menus natively. That does
not relate to fading or sliding them - both those would be a separate bug.

Comment 237

12 years ago
Is it a separate thing? All native menus slide and fade if told to do so. Is
there a separate API for that or something?

Comment 238

12 years ago
they do fade in but it happens so fast, it's not really a fade in
i tested this by pressing print screen right after i right-clicked to bring up
the context menu and when i pasted the screenshot, i got a faint outline where
the context menu appeared a split second later.

Comment 239

12 years ago
btw, where's the bug for the sliding and fading effects?

Comment 240

12 years ago
As far as I know, no menu ever fades in in Mozilla or Firefox - what you saw may
have been it after having drawn the border but not the inside. There does not
appear to be any bug for fading/slding the menus.

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.8beta5 → mozilla1.9alpha

Updated

12 years ago
Depends on: 313388
Depends on: 313674
*** Bug 313930 has been marked as a duplicate of this bug. ***

Comment 242

12 years ago
Created attachment 201358 [details] [diff] [review]
fix GDI resource leaks

You must call RestoreDC() before returning from
nsNativeThemeWin::ClassicDrawWidgetBackground().
This should solve the GDI resource issue on Win9x.

Updated

12 years ago
Attachment #201358 - Flags: review?(emaijala)

Comment 243

12 years ago
Comment on attachment 201358 [details] [diff] [review]
fix GDI resource leaks

Wouldn't it be cleaner and more future proof to change the returns to breaks and do the cleanup after the switch? You could have rv that would default to NS_OK and add default case that would set rv to NS_ERROR_FAILURE.

Comment 244

12 years ago
*** Bug 314438 has been marked as a duplicate of this bug. ***

Comment 245

12 years ago
Created attachment 201409 [details] [diff] [review]
fix GDI resource leaks v2

updated to comments.
Attachment #201358 - Attachment is obsolete: true
Attachment #201409 - Flags: review?(emaijala)
Attachment #201358 - Flags: review?(emaijala)

Comment 246

12 years ago
(In reply to comment #244)
> *** Bug 314438 has been marked as a duplicate of this bug. ***
> 

That one was about firefox top menu rendering with an xp look'n feel
instead of the expected windows 2000 default.

Updated

12 years ago
Depends on: 314524

Comment 247

12 years ago
(In reply to comment #245)
> Created an attachment (id=201409) [edit]
> fix GDI resource leaks v2
> 
> updated to comments.
> 
Can the GDI resource leak fix be partially checked in, or possibly a new bug be created for this? It would help Win 9x users.
Attachment #199429 - Attachment is obsolete: true
Attachment #199429 - Flags: superreview?
Attachment #199429 - Flags: review?

Comment 248

12 years ago
Comment on attachment 201409 [details] [diff] [review]
fix GDI resource leaks v2

Nice :)
Attachment #201409 - Flags: review?(emaijala) → review+

Updated

12 years ago
Attachment #201409 - Flags: superreview?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #201409 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 249

12 years ago
I apologize if this should be clear, but I've become lost by all the comments this way and that. Is there a separate bug for the fact that the menu bar is 23% taller in Firefox than other applications and has a groove at the top, or is that part of this bug (and to be fixed before this bug is marked as such)?

Comment 250

12 years ago
*** Bug 315945 has been marked as a duplicate of this bug. ***

Comment 251

12 years ago
Need checkin for attachment 201409 [details] [diff] [review].

Comment 252

12 years ago
(In reply to comment #251)
> Need checkin for attachment 201409 [details] [diff] [review] [edit].
> 

Apparently Neil checked this in 2005-11-16 05:51 on Trunk

Comment 253

12 years ago
Is this still targeted at 2.0?  James is going to be working on trunk, and it might take some work to backport it, so I'm curious as to whether this is considered a blocker or not given the new release plan.
Flags: blocking-aviary2?
I don't think its a blocker, but its going to be a core change more than anything and that is the big gating factor.
Flags: blocking-aviary2?

Comment 255

12 years ago
(In reply to comment #254)
> I don't think its a blocker, but its going to be a core change more than
> anything and that is the big gating factor.

That should be in firefox 1.5 at least. Cause actually it is really
a bad change for users under win2k (the top menu doesnt match the
rest of OS colors, looks flat and is slower).

Comment 256

12 years ago
Ashar: too late for 1.5, it was so at the "beginning" even.  No idea who had the bright idea to suddenly change the menus to what they are like now in 1.5 and got away with it... but ah well, this is what happened.  Here's for hoping that Silver can fix this until 1.9

Mike: Why isn't this a blocker?  Especially if it's something we seemed to have decided to fix until the next big release (as far as I can remember from the bug discussion at least) -- I think it only serves good if this remains a blocking bug; all the more will people feel the need to fix (and test, and report, and so on) this damn thing for once and all.
A blocker is something we will not ship without fixing.  We're shipping Firefox 1.5 without this, and if this isn't a safe change to Gecko we won't take it for Firefox 2.

Comment 258

12 years ago
(In reply to comment #256)
>No idea who had
> the bright idea to suddenly change the menus to what they are like now in 1.5
> and got away with it... 

see Bug 306893 ;)

Comment 259

12 years ago
(In reply to comment #256)
> Ashar: too late for 1.5, it was so at the "beginning" even.  No idea who had
> the bright idea to suddenly change the menus to what they are like now in 1.5
> and got away with it... but ah well, this is what happened.  Here's for hoping
> that Silver can fix this until 1.9

Is there any chance the top menu will be fixed in a a later minor release such
as 1.5.1 or will we have to wait for firefox 2.0 ? :o)

Comment 260

12 years ago
(In reply to comment #254)
> I don't think its a blocker, but its going to be a core change more than
> anything and that is the big gating factor.
> 

Shouldn't it read - in the blocker field then, for clarity's sake; so someone else doesn't try to nominate it and to show that it was actively denied?

Comment 261

12 years ago
*** Bug 317788 has been marked as a duplicate of this bug. ***
It seems that Bug 319222 has been caused by this bug (?)
So, there have been regressions from this bug on the trunk now for weeks (menus show up as classic-styled on windows xp/luna on many WXP installs, such as mine), and there appears to be no movement on fixing them. This is very high profile, very user facing.

What's the status? 

Comment 264

12 years ago
The regressions on trunk caused by chase landing bits of the stuff on this bug are SEP. If someone really wants to back out the half-a-fix that did get checked in to trunk without my approval they can. I don't care either way (except that backing it out will cause mega conflicts in my work) - I'm working on fixing the whole thing in pieces (in other bugs, because this one is a bloody mess now).
Depends on: 319222

Comment 265

12 years ago
(In reply to comment #264)
> The regressions on trunk caused by chase landing bits of the stuff on this bug
> are SEP. If someone really wants to back out the half-a-fix that did get
> checked in to trunk without my approval they can. I don't care either way
> (except that backing it out will cause mega conflicts in my work) - I'm working
> on fixing the whole thing in pieces (in other bugs, because this one is a
> bloody mess now).

This bug and the Bonsai logs show I never landed anything from here on the trunk.  Try try again.

Comment 266

12 years ago
s/chase/timeless/ then, it makes no difference, I'm not having any responsibility over the trunk checkin.
Hi James, I commented in this bug since it's still opened and ASSIGNED. What bugs are you pursuing the newer work in? 

Comment 268

12 years ago
It's still open and assigned because I am still working on it. However, it is totally useless for patches and comments now - to me it is nothing more than a meta-bug now, in effect. All the bugs I'm going to actually use for work to fix this are marked as depends. Specifically, bug 313388 and bug 313391 - I almost have a patch ready for the former one.

I don't think it got mentioned here before, but my plan was posted here:
  http://twpol.dyndns.org/weblog/2005/11/01/01

Any comments or questions about that plan can be directed either at the comments box on that page, or directly via e-mail.

I will also mention (to everyone) that I want absolutely nothing but proper technical comments, and no discussions, on these other bugs. Working on this is hard enough already!

Updated

12 years ago
Whiteboard: NON-TECHNICAL COMMENTS GO HERE: http://forums.mozillazine.org/viewtopic.php?t=317569

Updated

11 years ago
Flags: blocking1.8.1?
Depends on: 323839
*** Bug 328786 has been marked as a duplicate of this bug. ***

Comment 270

11 years ago
In way, I'm happy to get my bug report (328786) duped over to this one.  It IS
somewhat related.  This bug has a LOT more votes than mine would ever get.  The
CC list is MUCH longer, meaning that the chance is much higher that it will be
noticed by someone who understands the issue AND has time to look into it.

Alas, this is not the bug I'm reporting.  The one I'm reporting is specific,
narrow and cropped up only within the last week.  They both fall into the
same general arena of Mozilla not reflecting GUI preferences/settings/etc.
that are intended to be systemwide.  But all I am asking is that the menu text
be displayed in a color distinct from the menu background, as it has been
through years of Mozilla milestones, Phoenix, Firebird and Firefox trunk builds,
up until some time in the last week.

Comment 271

11 years ago
In way, I'm happy to get my bug report (328786) duped over to this one.  It IS
somewhat related.  This bug has a LOT more votes than mine would ever get.  The
CC list is MUCH longer, meaning that the chance is much higher that it will be
noticed by someone who understands the issue AND has time to look into it.

Alas, this is not the bug I'm reporting.  The one I'm reporting is specific,
narrow and cropped up only within the last week.  They both fall into the
same general arena of Mozilla not reflecting GUI preferences/settings/etc.
that are intended to be systemwide.  But all I am asking is that the menu text
be displayed in a color distinct from the menu background, as it has been
through years of Mozilla milestones, Phoenix, Firebird and Firefox trunk builds,
up until some time in the last week.

Comment 272

11 years ago
Any movement on this and is this going to make branch in time for 2.0?

~B

Comment 273

11 years ago
Until a decision is made on bug 313674, no, it cannot go forward. That decision critically affects the resulting appearance, and I can't call stage 1 (CSS-only version of the theme) done until that is decided.

Updated

11 years ago
Blocks: 333484
No longer depends on: 313674
(In reply to comment #273)
> Until a decision is made on bug 313674, no, it cannot go forward. That decision
> critically affects the resulting appearance, and I can't call stage 1 (CSS-only
> version of the theme) done until that is decided.

Decision made. If Hixie's suggestion in bug 313674 comment 5 can be accomplished, that's great. Otherwise that bug will be WONTFIXED. As you were, continue on, via con dios, etc.

Comment 275

11 years ago
(In reply to comment #274)
> (In reply to comment #273)
> > Until a decision is made on bug 313674, no, it cannot go forward. That decision
> > critically affects the resulting appearance, and I can't call stage 1 (CSS-only
> > version of the theme) done until that is decided.
> 
> Decision made. If Hixie's suggestion in bug 313674 comment 5 can be
> accomplished, that's great. Otherwise that bug will be WONTFIXED. As you were,
> continue on, via con dios, etc.
> 

Can someone please step in for Kevin and review the patch in bug https://bugzilla.mozilla.org/show_bug.cgi?id=313388 ?  It's been two weeks and no attempt at review.  I'm sure Kevin is busy but the clock is ticking and it would be a shame for this bug to miss the FF 2.0 bus.

~B

Updated

11 years ago
Depends on: 337771
This isn't going to be ready in time, at this time.
Flags: blocking1.8.1? → blocking1.8.1-

Updated

11 years ago
Assignee: silver → nobody
Status: ASSIGNED → NEW

Comment 277

11 years ago
Still exists in Firefox 2.0 Beta 2

Comment 278

11 years ago
(In reply to comment #277)
> Still exists in Firefox 2.0 Beta 2

     Could you be a little more specific.  E.g., what I see in

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060831 Minefield/3.0a1

is that menu bar, menu and scroll bar backgrounds don't match native controls,
rendering grayed-out menu items invisible in the scheme I'm using (but I
wasn't going to select those items anyway:-)

Comment 279

11 years ago
It is fixed in Minefield but not in Firefox 2.0 Beta 2. Menus look flat in Firefox Beta 2, and look native in the latest Minefield.

Comment 280

11 years ago
As showed above the text field, this bug depends on bug 337771, so it needs to be fixed before this one to be fixed.
And that bug is precisely about menus appearance.

Comment 281

11 years ago
The developers know this isn't fixed in Firefox 2, or even completely on the trunk, hence it not being marked FIXED. If you read through the discussion in this bug, you can see that it was ruled too late/risky for Firefox 2. Even if a perfect patch materialized right now, I doubt such a major change would be accepted this late in the game.

I agree that Firefox looks really screwy on XP Classic, but the drivers have spoken and this is going to stay broken.
Created attachment 236286 [details]
trunk, branch and native app compared with aluminium alloy skin

I don't know what you all complain about. It looks perfectly with branch builds but is broken with trunk ones, at least for me...

Comment 283

11 years ago
(In reply to comment #282)
> I don't know what you all complain about. It looks perfectly with branch builds
> but is broken with trunk ones, at least for me...
> 
It's because you're using the Luna theme, where the others prefer the Classic one.
It's true that the Luna theme support was broken in Firefox 1.0 (but not in 1.5) and of course works with 2.0.
But the Classic theme support is now broken until this bug is fixed.

Comment 284

11 years ago
(In reply to comment #279)
> It is fixed in Minefield but not in Firefox 2.0 Beta 2. Menus look flat in
> Firefox Beta 2, and look native in the latest Minefield.
> 

For this exact problem only, use Classic Menus for Winstripe extension...
https://addons.mozilla.org/firefox/1208/
No longer blocks: 333484

Comment 285

10 years ago
has this landed on the trunk yet?

Comment 286

10 years ago
has this landed on the trunk?

Comment 287

10 years ago
Declaring this bug FIXED. You should see correctly looking menus starting with tomorrows Minefield nightlies.

Please file regressions or edge-cases (e.g. where we're a pixel off) as new bugs and make them depend on this bug. Don't forget to attach screenshots of both Minefield and Windows Explorer (resp. IE6) for comparison.
Status: NEW → RESOLVED
Last Resolved: 12 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9alpha1 → mozilla1.9beta1

Updated

10 years ago
Blocks: 388317
(Assignee)

Updated

9 years ago
Product: Core → SeaMonkey

Updated

8 years ago
Component: Themes → Themes
Flags: blocking1.8.1-
Product: SeaMonkey → Toolkit
QA Contact: themes → themes
You need to log in before you can comment on or make changes to this bug.