783338 - Firefox button is the wrong height when built with VS2012

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Image of Firefox button too tall

It actually looks pretty cool when the window is maximized, since it fills the empty space in the NC area.  When windowed, the button overlaps the tab bar and looks decidedly uncool.  See attachment.

Comment 1

[Jim Mathies [:jimm]]

With your changes, do we see this on nightly w/vs10? In which case we can land bug 758280 on mc until we understand the cause of this and can fix it in between trains.

Comment 2

[Jim Mathies [:jimm]]

*can't land

Comment 3

[Masatoshi Kimura [:emk]]

I tested the tryserver build (built with vs10) from
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c3e04fa482f2/try-win32/firefox-17.0a1.en-US.win32.installer.exe
and the Firefox button height was normal. So we can land bug 758280 separately.

Comment 4

[Mark Straver]

I'm having the same issue when building with VS2012 and the patch for bug #758280 applied. Tabs on top overlap the Firefox button, tabs on bottom has the button exactly flush with the top edge of the navigation controls. The button is decidedly larger than the previous builds.

This is on Win 7 x64, default theme, etc.

Comment 5

[Mark Straver]

An additional comment: Using a persona (especially visible with a dark one) the window controls top-right also have an issue: below the buttons there is a grey bar of the right shape that seems to have the same height difference as the appmenu bar. Perhaps the new logic in bug #758280 messes up margins in the title bar layout of the theme or browser css/xul?

The "color" of this extra margin is transparent glass if aero glass is enabled.

https://dl.dropbox.com/u/1466747/FFvs2012-windowscontrols-error.png

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

To be clear, the patch in bug 758280 has no effect on whether the Firefox button is the wrong height.  Masatoshi verified this in comment 3.  The Firefox button is the wrong height in all VS2012 builds, regardless of whether the patch in bug 758280 is applied.  Similarly, the button is the correct height in all VS2010 builds (with or without the patch).

In regards to the grey bar that appears when using a persona; we should file a separate bug for it, determine if it is present in VS2010 builds as well as VS2012 builds, and determine also if it is affected by the patch in bug 758280.

Comment 7

[Mark Straver]

But that comment only confirmed that the patch when built with vs10 had no negative effect on the button height. It says nothing about the results of the patch on the platform it was actually written for, and it seems to me that it would be the root cause of the issue as the height is increased in that patch? It is also quite possible I misunderstand how drawing in the title bar factually works.

In any case, by the looks of it, the button height and the erroneously drawn margin below the window controls are closely related and caused by the same bug - the Appmenu button and the area reserved for window controls are both too tall (it could be the 5 pixels that is referred to in the other bug), and I already verified that VS2010 builds both has the correct button height and window control area. So it is VS2012 specific and likely related to what is described in https://bugzilla.mozilla.org/show_bug.cgi?id=758280#c2

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Mark Straver from comment #7)
> But that comment only confirmed that the patch when built with vs10 had no
> negative effect on the button height. It says nothing about the results of
> the patch on the platform it was actually written for, and it seems to me
> that it would be the root cause of the issue as the height is increased in
> that patch?

The firefox-button-too-big issue is known to occur in all VS2012 builds, even without the patch in bug 758280 is applied.  That patch did not increase the height of the firefox button.

> It is also quite possible I misunderstand how drawing in the
> title bar factually works.

I don't think anyone fully has a grasp on how that works ;)

> In any case, by the looks of it, the button height and the erroneously drawn
> margin below the window controls are closely related and caused by the same
> bug - the Appmenu button and the area reserved for window controls are both
> too tall (it could be the 5 pixels that is referred to in the other bug),
> and I already verified that VS2010 builds both has the correct button height
> and window control area. So it is VS2012 specific and likely related to what
> is described in https://bugzilla.mozilla.org/show_bug.cgi?id=758280#c2

I agree; the grey bar below the caption buttons and the firefox-button-too-big issue are probably the same underlying bug.

Comment 9

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: sample patch for correcting button height

This is simple patch.
But Only check on Win7 & Win8, and VC11 build. no check VC10 build.

Comment 10

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: difference of this patch

There is one difference thing.
When we play DX9 game on win7, Firefox button's height is low.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 674471 [details] [diff] [review]
sample patch for correcting button height

>+      if (nsUXThemeData::CheckForCompositor())
Why did you add the compositor check here? Maybe this is the cause of the difference.

Comment 12

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: erase checking compositor

If erase to check compositer, there is a little distance between Firefox button and tab.

This patch is SAMPLE patch. not RIGHT patch.
First, I searched a part of drawing Firefox Button code, but I didn't.
There was no help for it, I raised the position of the tab towards the top.

Comment 13

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: difference between VC10 build and VC11 build

I build fx using vc11 without patch, and compare it.
Top frame height is different when win7 and dx9 game.

Comment 14

[Mark Straver]

I'm wondering: could all of this be related to using the Win SDK version 8.0? If so, then a solution to this would be to use an earlier SDK (7.0A or 7.1) - according to Microsoft, you would have to do that anyway if you are going to target Windows XP, since the 8.0 SDK has dropped support for XP and 2003 (see link) - combining it with the fact that the DirectX SDK is integrated, it would account for the oddness when playing DX9 games.

Ref: http://blogs.msdn.com/b/vcblog/archive/2012/10/08/10357555.aspx

Comment 15

[Masatoshi Kimura [:emk]]

These are related with subsystem version. For < 6.0 apps (default on VS2010), GetSystemMetrics(SM_CXFRAME) and GetSystemMetrics(SM_CYFRAME) will return larger values by padded-border size, and GetSystemMetrics(SM_CXPADDEDBORDER) will always return zero.
For >= 6.0 apps (default on VS2012), GetSystemMetrics() will return original values.

Comment 16

[Mark Straver]

Ahh.. okay - but, shouldn't we pass a subsystem of 5.1 to the linker anyway if we're going to build for Windows XP? Otherwise the app would probably not run at all to begin with; meaning, if that solves the whole title bar issue then both this bug and bug #758280 would be moot :P

Comment 17

[Masatoshi Kimura [:emk]]

VS2012 cannot build binaries running on WinXP anyway until VS2012 update 1 has been released.
https://blogs.msdn.com/b/vcblog/archive/2012/10/08/10357555.aspx?Redirected=true

Comment 18

[Mark Straver]

I know - I'm just looking ahead here, seems a little silly to implement bugfixes for something that might be N/A in a very short while when the build system would be set up for v110_xp (if selecting the subsystem needed for the intended target avoids the issue altogether).

Of course, if these patches are going to fix building for any possible scenario (vs2010/vs2012, subsystem <6 and >=6), then I'm all for it!

Comment 19

[Masatoshi Kimura [:emk]]

(In reply to Mark Straver from comment #18)
> Of course, if these patches are going to fix building for any possible
> scenario (vs2010/vs2012, subsystem <6 and >=6), then I'm all for it!
The patch should have no side-effect on VS2010 (subsystem < 6.0) builds because GetSystemMetrics(SM_CXPADDEDBORDER) will always return zero.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Mark Straver from comment #18)
> I know - I'm just looking ahead here, seems a little silly to implement
> bugfixes for something that might be N/A in a very short while when the
> build system would be set up for v110_xp (if selecting the subsystem needed
> for the intended target avoids the issue altogether).
> 
> Of course, if these patches are going to fix building for any possible
> scenario (vs2010/vs2012, subsystem <6 and >=6), then I'm all for it!

FWIW, we won't migrate to vs 2012+ for a while. Could be a year or two, maybe longer.

Comment 21

[Mark Straver]

Attachment: Patch to correct widget height on VS2012

Corrected patch to fix widget heights for VS2012.
This fixes both the Firefox button height and the widget drawing height of windows controls (min/max/close).

Comment 22

[Jim Mathies [:jimm]]

(In reply to Mark Straver from comment #21)
> Created attachment 678012 [details] [diff] [review]
> Patch to correct widget height on VS2012
> 
> Corrected patch to fix widget heights for VS2012.
> This fixes both the Firefox button height and the widget drawing height of
> windows controls (min/max/close).

Great!

try builds for people to test:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=42848adde61d

Note this patch didn't apply cleanly, I had to do it manually. you might want to check the formatting.

Comment 23

[Mark Straver]

Quite possible I did something wrong creating the patch - you'll have to excuse me; first patch and all. ;) Any way for me to check the formatting? Do you have a doc somewhere that might help supplying patches to bugzilla?

Comment 24

[Jim Mathies [:jimm]]

(In reply to Mark Straver from comment #23)
> Quite possible I did something wrong creating the patch - you'll have to
> excuse me; first patch and all. ;) Any way for me to check the formatting?
> Do you have a doc somewhere that might help supplying patches to bugzilla?

If you're working with a local repo, 

hg diff -p -U 8

should generate a suitable patch. You can also put this in the hg rc file to make it automatic, in C:\Users\(username)\AppData\.hgrc add:

[defaults]
diff=-p -U 8
qdiff=-U 8 
commit = -v

Comment 25

[Mark Straver]

Attachment: Patch to correct widget height on VS2012 v2

This one should apply cleanly... If not, tell me what I'm doing wrong?

Comment 26

[Mark Straver]

(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Mark Straver from comment #23)
> If you're working with a local repo, 
> 
> hg diff -p -U 8

I'm working with a local repo but this is a custom build with a LOT more changes and only using the repo as a "base" code snapshot, so I have to diff'it manually, with files of different versions, not against the hg checkout, if I want to create patches. I'll figure it out though. thanks.

Comment 27

[Jim Mathies [:jimm]]

(In reply to Mark Straver from comment #26)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > (In reply to Mark Straver from comment #23)
> > If you're working with a local repo, 
> > 
> > hg diff -p -U 8
> 
> I'm working with a local repo but this is a custom build with a LOT more
> changes and only using the repo as a "base" code snapshot, so I have to
> diff'it manually, with files of different versions, not against the hg
> checkout, if I want to create patches. I'll figure it out though. thanks.

You might check out mercurial patch queues, they make managing multiple projects in the same repo pretty easy.

http://mercurial.selenic.com/wiki/MqExtension

Comment 28

[Masatoshi Kimura [:emk]]

https://developer.mozilla.org/en-US/docs/Mercurial_Queues
will help for things specific to Mozilla development.

Comment 29

[Masatoshi Kimura [:emk]]

Mark, are you still working on?

Comment 30

[Mark Straver]

No, I've applied my v2 patch and it works, as far as I'm concerned this is fixed.

Comment 31

[Masatoshi Kimura [:emk]]

It's insufficient to commit the patch to mozilla-central.
If you are not interested in completing the patch, I'll take over your work.

Comment 32

[Mark Straver]

Go ahead - I have no clue what else is required besides submitting the patch.
I don't have a working setup to check the patch in as a hg changeset nor the knowledge how to properly do this from a custom tree, and MQ seems to be a bit too dangerous to touch, reading the MDN doc above.

For future ref, if there are any things I need to indicate, set or change in bugzilla to have it processed further by someone familiar with the Mozilla commit procedures, let me know and I'll do that from now on.

Comment 33

[Masatoshi Kimura [:emk]]

Attachment: Correct widget height on VS2012 (subsystem >= 6)

I just added Mark's name to the header.

Comment 34

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #22)
> (In reply to Mark Straver from comment #21)
> > Created attachment 678012 [details] [diff] [review]
> > Patch to correct widget height on VS2012
> > 
> > Corrected patch to fix widget heights for VS2012.
> > This fixes both the Firefox button height and the widget drawing height of
> > windows controls (min/max/close).
> 
> Great!
> 
> try builds for people to test:
> 
> https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=42848adde61d
> 
> Note this patch didn't apply cleanly, I had to do it manually. you might
> want to check the formatting.

Has anyone taken these for a spin on xp to be sure everything looks right?

Comment 35

[Masatoshi Kimura [:emk]]

Attachment: Test build with Firefox Menu enabled

I didn't see any obvious problems.

Comment 36

[Masatoshi Kimura [:emk]]

Attachment: Test build with Firefox Menu disabled

Comment 37

[Masatoshi Kimura [:emk]]

Also, green on try.
https://tbpl.mozilla.org/?tree=Try&rev=25e308c1793c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b01cb2b0cd7

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b01cb2b0cd7