330231 - Firefox should use Common Controls 6.0 dialogs

Status: VERIFIED FIXED

(Core :: Widget: Win32, enhancement)

Description

[Régis Caspar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060311 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060311 Firefox/1.6a1

Firefox dialogs like File->Save Page As, File->Open File, ... are not XP styled. 

Reproducible: Always

Steps to Reproduce:
1. Cast a Firefox dialog (example: File > Save Page As)
2. Cast a WinXP dialog (example: [Win]+R > Browse
3. Compare (note the buttons)


Actual Results:  
Firefox dialog buttons are squared and not styled according to current theme

Expected Results:  
Firefox dialog buttons styled according to OS theme

Comment 1

[Régis Caspar]

Attachment: Actual Firefox dialog

Comment 2

[Régis Caspar]

Attachment: Actual OS dialog (winXP, Royale theme)

Comment 3

[Régis Caspar]

Attachment: patch adding Common Controls 6 as dependency

This patch resolve this problem by adding a Common-Controls 6.0 dependencies to the manifest. Tested on WinXP with msvc 8.0 express edition. Will attach screenshot of result.

Comment 4

[Régis Caspar]

Attachment: Result

Screenshot of Firefox save dialog after patch

Comment 5

[u88484]

Looks nice, are you going to ask for review?

Comment 6

[Régis Caspar]

Sorry but i don't know how to I'm a beginner in patch submission. BTW, I'm not sure this bug is set with right Product/Component..
Advices are welcome ;) ?

Comment 7

[u88484]

Comment on attachment 214824 [details] [diff] [review]
patch adding Common Controls 6 as dependency

Requesting review by mike beltzner...I think your the right guy to review this?

Comment 8

[Régis Caspar]

(In reply to comment #7)
> (From update of attachment 214824 [details] [diff] [review] [edit])
> Requesting review by mike beltzner...I think your the right guy to review this?
Thanks Kurt, 
I was thinking about Mike Beltzner, because of the moz.dev.themes thread but was not sure. 

Comment 9

[(not currently active) Ted Mielczarek]

Oh hm, this is a regression from 1.5.  :-(  I presume this is because we're specifying a manifest when building with VC8, but it doesn't specify the common controls?

I don't know if this is the best patch, we should probably put this into the Makefile as a linker param.

Also, beltzner isn't really a reviewer for code-level stuff.

Comment 10

[Régis Caspar]

(In reply to comment #9)
> Oh hm, this is a regression from 1.5.  :-(  I presume this is because we're
> specifying a manifest when building with VC8, but it doesn't specify the common
> controls?
Yes, if nothing is specified in the manifest, an old version without uxtheme support is used. (some doc here: http://msdn.microsoft.com/library/en-us/dnwxp/html/xptheming.asp)
 
> I don't know if this is the best patch, we should probably put this into the
> Makefile as a linker param.
Yes, the pragma is probably a bad. 

> Also, beltzner isn't really a reviewer for code-level stuff.
Ok :)

Comment 11

[Régis Caspar]

(In reply to comment #10)
> Yes, the pragma is probably a bad. 

I meant "bad idea" here sorry

Comment 12

[(not currently active) Ted Mielczarek]

Working on a simple build system patch...

Comment 13

[Régis Caspar]

(In reply to comment #12)
> Working on a simple build system patch...
Thanks Ted, if it could help I've made some modifications to /mozilla/browser/app/Makefile.in and it works here. I can upload a diff if needed. My problem is the define equivalent (should winCE really be excluded in this change? and how?)

Comment 14

[(not currently active) Ted Mielczarek]

If you've got something that works, please attach it and I'll help you clean it up.  Feel free to reassign to yourself as well.  :)

Comment 15

[Régis Caspar]

Attachment: Makefile.in way

(In reply to comment #14)
> If you've got something that works, please attach it and I'll help you clean it
> up.  Feel free to reassign to yourself as well.  :)
Here is a patch. The libpath thing is a tweak because MSVC2005 Express here doesn't like the -L to set libpath. Then if OS is WINNT and compiler isn't gcc we add the manifest dependency to Common Control 6 (if I didn't misread anything).

Comment 16

[(not currently active) Ted Mielczarek]

That's basically the same patch I have.  I can't get mine to compile though, I get 
"LINK : fatal error LNK1181: cannot open input file 'name='Microsoft.Windows.Comm
on-Controls''"

Did you do anything else to make that work?  I'm playing with it now.

Comment 17

[Régis Caspar]

(In reply to comment #16)
> I can't get mine to compile though, I get 
> "LINK : fatal error LNK1181: cannot open input file
> 'name='Microsoft.Windows.Common-Controls''"
> 
> Did you do anything else to make that work?  I'm playing with it now.
No nothing else but your problem looks like a quoting problem do you have the double quotes around all the thing ?

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: Makefile.in changes

This is what I did.  You can't go making that other change like you did, unless that's compatible with all compilers.  This patch should only have an effect on VC8 builds.  I still can't get it to compile.  I wonder if this doesn't work on VC2k5 express.  Can anyone confirm?

Comment 19

[Régis Caspar]

(In reply to comment #18)
> Created an attachment (id=215207) [edit]
> Makefile.in changes
> 
> This is what I did.  You can't go making that other change like you did, unless
> that's compatible with all compilers.  This patch should only have an effect on
> VC8 builds.  I still can't get it to compile.  I wonder if this doesn't work on
> VC2k5 express.  Can anyone confirm?

I tested your patch and it works nicely here (VC2005 Express). Here's my output:
[ cut ]
link -NOLOGO -OUT:firefox.exe -PDB:firefox.pdb -SUBSYSTEM:WINDOWS -LTCG   "-OPT:REF,ICF,NOWIN98" /HEAP:0x40000 "/manifestdependency:type='win32' name='Microsoft.Windows.Common-Controls' version='6.0.0.0' processorArchitecture='X86' publicKeyToken='6595b64144ccf1df'" nsBrowserApp.obj nsStaticComponents.obj ./module.res ../../dist/lib/xulapp_s.lib -L../../dist/lib/components [ cut ]
LINK : warning LNK4044: unrecognized option '/L../../dist/lib/components'; ignored
   Creating library firefox.lib and object firefox.exp
Generating code
c:\mozilla\mozilla\widget\src\windows\nswindow.cpp(8458) : warning C4700: uninitialized local variable 'hAlphaBitmap' used
c:\mozilla\mozilla\browser\components\migration\src\nsbrowserprofilemigratorutils.cpp(326) : warning C4715: 'ImportBookmarksHTML' : not all control paths return a value
c:\mozilla\mozilla\browser\components\migration\src\nsieprofilemigrator.cpp(1231) : warning C4700: uninitialized local variable 'keywordsFolder' used
Finished generating code
echo not_strip firefox.exe
[ cut ]

Method:
- checked out a clean copy of Makefile.in
- patch -p0 < bug330231.patch
- ./configure
- cd browser/app
- make

Versions:
- shell: GNU bash, version 2.04.0(1)-release (i686-pc-msys)
- cl: Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 14.00.50727.42 for 80x86
- link: Microsoft (R) Incremental Linker Version 8.00.50727.42

Comment 20

[(not currently active) Ted Mielczarek]

Attachment: Better way, rules.mk change

I had a better idea in the interim, and this works on my system.  Since there's already a firefox.exe.manifest in the source dir that specifies the common controls dependency, this patch just merges that manifest with the linker-generated manifest into the exe if they both exist.

This is nice because this will work for any exe in the build system.

Comment 21

[Régis Caspar]

(In reply to comment #20)
> I had a better idea in the interim, and this works on my system.  Since there's
> already a firefox.exe.manifest in the source dir that specifies the common
> controls dependency, this patch just merges that manifest with the
> linker-generated manifest into the exe if they both exist.
> 
> This is nice because this will work for any exe in the build system.

Seems nice, I need to try it. As a remark, after some thinking about that, there's a remaining problem: mingw (ie gcc) builds on WinXP should add the common control 6 dependency as well. So perhaps the best way is to use an additionnal non-embedded manifest. This works here with official nightly builds.

Comment 22

[Régis Caspar]

(In reply to comment #21)
> So perhaps the best way is to use an additionnal non-embedded manifest. 
Or embedd via the rc file...

Comment 23

[Benjamin Smedberg]

Comment on attachment 215286 [details] [diff] [review]
Better way, rules.mk change

Please add quotes around $(srcdir)/$@.manifest and $(win_srcdir)/$@.manifest

Comment 24

[(not currently active) Ted Mielczarek]

Attachment: rules.mk change, with quotes

Updated.

Comment 25

[Benjamin Smedberg]

Fixed on trunk

Comment 26

[Régis Caspar]

Since yesterday builds, the Common Control 6 dependencies is not anymore in the manifest so something is wrong..

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

Comment 27

[Régis Caspar]

Problem confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060324 Firefox/1.6a1 ID:2006032404
Both updater.exe and firefox lack manifest are bad.

-> reopening (not sure if this is right)

I think that ( test -f "$(srcdir)/$@.manifest"; ) is somehow false and thus common control 6 dependency is not added. Additionnally, the patch is not nice if you build without a dedicaced objects directory. 
Possible solution:
1/ move browser/app/firefox.exe.manifest to config/common-control.manifest
2/ in rules.mk:
ifdef MSMANIFEST_TOOL 
  @if test -f $@.manifest; then \
    mt.exe -NOLOGO -MANIFEST "$(topsrcdir)/config/common-control.manifest" $@.manifest -OUTPUTRESOURCE:$@\;1; \
  fi
endif # MSVC with manifest tool
3/ remove all remaining exe manifest from source tree

Comment 28

[Régis Caspar]

(In reply to comment #27)
> Both updater.exe and firefox lack manifest are bad.

Should be 'Both updater.exe and firefox.exe manifests are bad.'
Sorry

Comment 29

[(not currently active) Ted Mielczarek]

So tinderboxes don't use an objdir.  Dunno why I didn't think about that.  I think that means that the firefox.exe.manifest in the srcdir will get overwritten with the one produced by the linker.  That sucks.  We probably need to either a) put a copy of this somewhere else in the tree, or b) rename it.

Comment 30

[Benjamin Smedberg]

b) firefox.exe.manifest-combined

Comment 31

[(not currently active) Ted Mielczarek]

bsmedberg proposes firefox.exe.src-manifest.  Sounds good to me.

Comment 32

[Régis Caspar]

(In reply to comment #29)
> So tinderboxes don't use an objdir.  Dunno why I didn't think about that.  I
> think that means that the firefox.exe.manifest in the srcdir will get
> overwritten with the one produced by the linker.  That sucks.  We probably need
> to either a) put a copy of this somewhere else in the tree, or b) rename it.
(In reply to comment #30)
> b) firefox.exe.manifest-combined
IMHO, it's time to think to gcc builds too (cross building, mingw) so the combined proposal of Benjamin is not suitable but moving or renaming seems nice. In addition to the rules.mk changes I proposed on #27, perhaps a fallback "else" (os_target=winxp; compiler!=msvc) should copy the renamed manifest into dist/bin. This way every possibility seems covered.

Comment 33

[Régis Caspar]

Attachment: rules.mk new version

This is the patch corresponding to what I said in #32. However I don't know how to neither rename nor delete a file from a patch so firefox.exe.manifest need to be moved in config/common-controls-6.manifest and *all* other existing exe's manifests removed from the CVS tree.

Perhaps a test for (OS_TARGET == WIN_XP) after the 2nd else would be clever

Comment 34

[Régis Caspar]

Attachment: rules.mk new version (2)

There was missing some changes to the ver files used to generates exe's RC

Comment 35

[Régis Caspar]

Attachment: rules.mk new version (3)

Corrected a small file path mistake.

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 218555 [details] [diff] [review]
rules.mk new version (3)

I'm not a build system peer.  This patch does look pretty sensible to me though.

Comment 37

[Régis Caspar]

(In reply to comment #36)
> (From update of attachment 218555 [details] [diff] [review] [edit])
> I'm not a build system peer.  This patch does look pretty sensible to me
> though.
Thanks for you reply. The main problem with this patch is the need to add a new file (i.e. config/common-controls-6.manifest) and to remove existing exe manifest from cvs. 

Comment 38

[Benjamin Smedberg]

Comment on attachment 218555 [details] [diff] [review]
rules.mk new version (3)

in rules.mk we can ditch the "if" altogether:

mt.exe -NOLOGO -MANIFEST "$(topsrcdir)/config/common-controls-6.manifest" "$(wildcard $(srcdir)/$@.manifest)" -OUTPUTRESOURCE:$@\;1

why do we need the "else cp" bit? We don't use manifests on MSVC6 or 7, do we?

Comment 39

[Régis Caspar]

(In reply to comment #38)
> (From update of attachment 218555 [details] [diff] [review] [edit])
> in rules.mk we can ditch the "if" altogether:
> 
> mt.exe -NOLOGO -MANIFEST "$(topsrcdir)/config/common-controls-6.manifest"
> "$(wildcard $(srcdir)/$@.manifest)" -OUTPUTRESOURCE:$@\;1
Awesome, just one question, if widlcard return nothing will mt be happy with:
mt.exe -NOLOGO -MANIFEST "path/to/config/common-controls-6.manifest" "" -OUTPUTRESOURCE:$@\;1 ?
 
> why do we need the "else cp" bit? We don't use manifests on MSVC6 or 7, do we?
Yes I think we do, that's my main problem and as I suggested on #33 it perhaps need an additionnal if:
On windows XP, if there is an embedded manifest, windows use it to find dependencies. Additionnally, if there is a prog.exe.manifest, windows use it aswell (an probably override embedded one). Now if a manifest is here and declare a dependency against Common Control 6, windows will use themed widgets. If no manifest declare such dependency, windows will use non themed aka "Classic" widgets.
Back to firefox, a version used on windows XP could be build with gcc-mingw, MSVC 6, 7 or 8 (+ gcc cross compilation ?). My "common-controls-6.manifest" (will attach it) only include a dependency against Common Control 6 so that during a mingw or msvc6 building, the "cp" copy this manifest and build will use Common Control 6 if possible (on a themed WinXP).

Perhaps something is still missing to "install" this manifest or
cp "$(topsrcdir)/config/common-controls-6.manifest" "$(DIST)/bin/$@.manifest"
and to include it in zips, setups, ...

Comment 40

[Régis Caspar]

Attachment: common-controls-6.manifest

Comment 41

[Régis Caspar]

Attachment: rules.mk new version (4)

I made changes following Benjamin's remarks on #38. I removed the "cp thing" from 'rules.mk' and added some change to 'Makefile.in' for both 'firefox.exe' and 'updater.exe' so that when OS_ARCH is WIN_NT and MSMANIFEST_TOOL is not defined we do the copy of common-controls-6.manifest. I also added the two manifests to 'browser/installer/windows/packages-static' in order to have them included when making installer or zip.

Comment 42

[(not currently active) Ted Mielczarek]

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

Comment 43

[Scott MacGregor]

Regis, do we need to make similar changes for Thunderbird? I can help out withthat if need be. 

Comment 44

[Régis Caspar]

(In reply to comment #43)
> Regis, do we need to make similar changes for Thunderbird? I can help out
> withthat if need be. 

It depends on the compiler used for Thunderbird (I didn't check). If the compiler is msvc 8 then yes similar changes will be needed. However the 'rules.mk new version (4)' isn't bulletproof here. The 'wildcard' sometimes fails and I got some error because of 'processorArchitecture="*"' in 'common-controls-6.manifest'. I need to do a fresh build to see how things are now, because I didn't built Firefox since more than a month. I will do that quickly. 

Comment 45

[Régis Caspar]

(In reply to comment #44)
> I will do that quickly. 
Not before this week end though

Comment 46

[Régis Caspar]

(In reply to comment #45)
> (In reply to comment #44)
> > I will do that quickly. 
> Not before this week end though

Sorry for the delay, I don't have a lot of free time these days. I thought about this and I think the first solution I used (see first patch) was not so bad. However, adding linker flag in a source file is probably not a good idea. So I think we can add this /manifestdependency flag in a LDFLAGS2 var into rules.mk (MSMANIFEST_TOOL) protected by a ifdef and then add $(LDFLAGS2) after $(LDFLAGS). I'm currently testing that.   

Comment 47

[Régis Caspar]

(In reply to comment #46)
> I'm currently testing that.   

After some testing, this method works for everything (all exe) but firefox.exe ?! 
So I don't know. Feel free to take this one..

Comment 48

[(not currently active) Ted Mielczarek]

Fixed in today's nightly by bug 377100.

Comment 49

[(not currently active) Ted Mielczarek]

Yeah, I had to back that out, should have reopened this.

Comment 51

[(not currently active) Ted Mielczarek]

This should be fixed on trunk, since the Firefox tinderboxen use objdirs now.

Comment 52

[Régis Caspar]

Verified ; Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090704 Minefield/3.0a8pre ID:2007090704