Firefox button is the wrong height when built with VS2012

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: TimAbraldes, Assigned: emk)

Tracking

unspecified
mozilla19
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 2 obsolete attachments)

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.

Updated

7 years ago
Depends on: 758280
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.
*can't land
Assignee

Comment 3

7 years ago
I tested the tryserver build (built with vs10) from
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-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

7 years ago
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

7 years ago
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
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

7 years ago
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
(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.
This is simple patch.
But Only check on Win7 & Win8, and VC11 build. no check VC10 build.
There is one difference thing.
When we play DX9 game on win7, Firefox button's height is low.
Assignee

Comment 11

7 years ago
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.
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.
I build fx using vc11 without patch, and compare it.
Top frame height is different when win7 and dx9 game.

Comment 14

7 years ago
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
Assignee

Comment 15

7 years ago
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

7 years ago
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
Assignee

Comment 17

7 years ago
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

7 years ago
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!
Assignee

Comment 19

7 years ago
(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.
(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

7 years ago
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).
(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

7 years ago
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?
(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

7 years ago
This one should apply cleanly... If not, tell me what I'm doing wrong?
Attachment #678012 - Attachment is obsolete: true

Updated

7 years ago
Attachment #678026 - Attachment is patch: true

Comment 26

7 years ago
(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.
(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
Assignee

Comment 28

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

Comment 29

7 years ago
Mark, are you still working on?

Comment 30

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

Comment 31

7 years ago
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

7 years ago
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.
Assignee

Comment 33

7 years ago
I just added Mark's name to the header.
Assignee: nobody → VYV03354
Attachment #678026 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #682404 - Flags: review?(jmathies)

Updated

7 years ago
Attachment #682404 - Flags: review?(jmathies) → review+
(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?
Assignee

Comment 35

7 years ago
I didn't see any obvious problems.
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
https://hg.mozilla.org/mozilla-central/rev/1b01cb2b0cd7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.