Closed Bug 575245 Opened 14 years ago Closed 14 years ago

Z-Level issues with the windows taskbar and browser windows that display the firefox button (auto-hide)

Categories

(Core :: Widget: Win32, defect, P1)

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b3
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: streetwolf52, Assigned: wgianopoulos)

References

Details

Attachments

(3 files, 17 obsolete files)

19.81 KB, image/png
Details
7.67 KB, image/png
Details
4.27 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100628 Minefield/3.7a6pre Firefox/3.6.4
Build Identifier: 20100628035917

1. When the Fx window is maximized the Windows Taskbar does not unhide if you set it to autohide.  This is done by moving the mouse pointer to the side of the screen where you have the Taskbar on, which by default is the bottom.

2. The Taskbar pops under a non-maximized window until you move focus to something else like the desktop.

These two are probably related.



Reproducible: Always



Expected Results:  
With Taskbar auto hide enabled (Set in 'Taskbar and Start Menu Properties') the Taskbar should rise from where it is docked (default is the bottom) over the window.  As it also should with a non-maximized window which is placed over where the Taskbar is located.
Priority: -- → P1
Problem #2 appears to happen only if you bring up Fx maximized and then reduce it's size by either aero snap or the resize title bar button.  If Fx is started non-maximized the taskbar works as expected.
Some more info for you.  If I non-maximize my Fx window, close it, and start it up again then maximize it, the taskbar will unhide until you close Fx.  It's only when Fx starts up maximized that I can't unhide the Taskbar.
Auto unhide of the taskbar also fails to work no matter what the sate of taskbar auto-hide or maximized window was on startup, if first unhide the taskbar and then hide it while firefox is maximized, this also triggers the issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
(In reply to comment #2)
> Some more info for you.  If I non-maximize my Fx window, close it, and start it
> up again then maximize it, the taskbar will unhide until you close Fx.  It's
> only when Fx starts up maximized that I can't unhide the Taskbar.

Doesn't always happen like this. All I can say with certainty is it's borked.
Jimm, Is this related to bug 575222?
Summary: Problems caused by new Titlebar → Z-Level issues with the windows taskbar and browser windows that display the firefox button
(In reply to comment #5)
> Jimm, Is this related to bug 575222?

No, strange bug though! I can confirm.
blocking2.0: --- → ?
Even stranger is that when I run non-maximized for ahwile and I made sure the taskbar popups over the window at some point I can maximize the window and the taskbar will unhide.

There are times when I maximize a window for about 2 seconds I see the top line of the taskbar and then it disappears.
What you might want to do, unless you already do it, is wherever you set the maximum height for a maximized window do it something like maxheight-1.  This will allow room for the taskbar to unhide.
(In reply to comment #8)
> What you might want to do, unless you already do it, is wherever you set the
> maximum height for a maximized window do it something like maxheight-1.  This
> will allow room for the taskbar to unhide.

looking at it, I think we might be extending our client frame down a little too far.
(In reply to comment #8)
> What you might want to do, unless you already do it, is wherever you set the
> maximum height for a maximized window do it something like maxheight-1.  This
> will allow room for the taskbar to unhide.

You should have taken the bug Gary, you had the right fix!
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
Comment on attachment 454603 [details] [diff] [review]
patch

This nixes the safety checks on offsets (who needs them!)  and sets the bottom client margin to -1 for maximized windows.
Attachment #454603 - Flags: review?(tellrob)
Comment on attachment 454603 [details] [diff] [review]
patch

scratch that, I should only be doing this for auto offset values.
Attachment #454603 - Flags: review?(tellrob) → review-
blocking2.0: ? → beta2+
Attached patch patch v.2 (obsolete) — Splinter Review
taskbar fix, plus a bottom offset fix when the bottom margin is set to auto size (-1) and the window is maximized. (plus a little formatting cleanup, spacing was off on a couple lines.)
Attachment #454603 - Attachment is obsolete: true
Attachment #454645 - Flags: review?(tellrob)
Jim.  Does the handle the situation of the taskbar appearing under a non-maximized window?
(In reply to comment #15)
> Jim.  Does the handle the situation of the taskbar appearing under a
> non-maximized window?

Seems to. To test, drag the window down so that the lower boundary intercepts the taskbar when visible. Double click to maximize, double click again to restore, then bring the taskbar up. Results: taskbar on top of the window. Is that right?
Comment on attachment 454645 [details] [diff] [review]
patch v.2

found a bug.
Attachment #454645 - Attachment is obsolete: true
Attachment #454645 - Flags: review?(tellrob)
Attached patch fix (obsolete) — Splinter Review
Neil, I've been dumping a ton of reviews on rob lately, care to dip your toes in non-client margin code and look this one over?
Attachment #454763 - Flags: review?(neil)
So, let's see if I've understood this correctly; Firefox sets the chromemargin to 0,-1,-1,-1 which removes the caption and top border, is that correct? What does Spy++ return for the client rect a) without glass b) without the patch c) with the patch? (Sorry, but I don't have easy access to glass.)
(In reply to comment #19)
> So, let's see if I've understood this correctly; Firefox sets the chromemargin
> to 0,-1,-1,-1 which removes the caption and top border, is that correct?

yes. (it removes these areas from the client area which content renders to)

> What
> does Spy++ return for the client rect a) without glass b) without the patch c)
> with the patch? (Sorry, but I don't have easy access to glass.)

maximized window client area and origins:

w/glass:

nightly:
-8,-8,2568,1608
8,0,2568,1608

w/patch:
-8,-8,2568,1608
8,0,2568,1607

non-glass:

nightly:
-8,-8,2568,1608
8,30,2568,1608

w/patch:
-8,-8,2568,1608
8,30,2568,1608
for reference, google chrome, which is another app with a custom client area:

-8,-8,2568,1608
8,0,2568,1606
I see an issue in builds with this patch applied.  With Vista aero glass and the menubar hidden, withthe firefox window maximized, if I do not have the taskbar hidden, it appears there are remnants of data in the pixel row just above the taskbar.

One way to reproduce this is to have the screen maximized with the taskbar hidden and then position the screen data by vertical scrolling such that when you move the mouse to the bottom of the screen to make the taskbar unhide a row of text is half visible.  Then if you leave the firefox window in that state and change the taskbar properties to not auto-hide the taskbar you can see a row of pixels with remnants of the dots from that line of text.
(In reply to comment #22)
> I see an issue in builds with this patch applied.  With Vista aero glass and
> the menubar hidden, withthe firefox window maximized, if I do not have the
> taskbar hidden, it appears there are remnants of data in the pixel row just
> above the taskbar.
> 
> One way to reproduce this is to have the screen maximized with the taskbar
> hidden and then position the screen data by vertical scrolling such that when
> you move the mouse to the bottom of the screen to make the taskbar unhide a row
> of text is half visible.  Then if you leave the firefox window in that state
> and change the taskbar properties to not auto-hide the taskbar you can see a
> row of pixels with remnants of the dots from that line of text.

I think that's supposed to be there. I see the same thing in IE and other apps.
FYI...  A Maximized window will show a thin line at the bottom which is the top of the taskbar.  In addition, any open icons on the taskbar will also show their tops.  I'm on Windows 7 so Vista might be a little different especially with the icons.

Just curious.  isn't all of this handled natively by Windows?  Are you performing, or trying to, these functions yourself?
(In reply to comment #24)
> FYI...  A Maximized window will show a thin line at the bottom which is the top
> of the taskbar.  In addition, any open icons on the taskbar will also show
> their tops.  I'm on Windows 7 so Vista might be a little different especially
> with the icons.
> 
> Just curious.  isn't all of this handled natively by Windows?  Are you
> performing, or trying to, these functions yourself?

nope, we're just a normal window.
Aha, so the real problem is that, effectively by hiding the caption, you've turned yourself into a full-screen window, which forces the taskbar to hide.
This does not look like it is supposed to be there.  See how under the word done int he status bar there are artifacts of the portion of the tinderbox sidebar previously displayed on that portion of the screen.  Also not the artifacts of text farther over to the right in the portion of the statusbar below the content area.  It seems that somehow in some situations when the window is maximized and the status bar is not in auto-hide, the bottom row of pixels in the Firefox window is not being painted.
Oh, I should have mentioned the above screenshot was from Windows 7 so the issue is not Vista only.
Where are you getting the builds with these patches.  I'd like to try it out.
Comment on attachment 454763 [details] [diff] [review]
fix

(In reply to comment #27)
> Created an attachment (id=454980) [details]
> a picture is worth a thousand words
> 
> This does not look like it is supposed to be there.  See how under the word
> done int he status bar there are artifacts of the portion of the tinderbox
> sidebar previously displayed on that portion of the screen.  Also not the
> artifacts of text farther over to the right in the portion of the statusbar
> below the content area.  It seems that somehow in some situations when the
> window is maximized and the status bar is not in auto-hide, the bottom row of
> pixels in the Firefox window is not being painted.

hrm, so I've compensated for the auto-hide feature, but screwed up the non-auto hide window. wonderful. :/
Attachment #454763 - Attachment is obsolete: true
Attachment #454763 - Flags: review?(neil)
(In reply to comment #29)
> Where are you getting the builds with these patches.  I'd like to try it out.

My builds available here: http://www.wg9s.org/mozilla/firefox/ include this patch.
(In reply to comment #31)
> (In reply to comment #29)
> > Where are you getting the builds with these patches.  I'd like to try it out.
> 
> My builds available here: http://www.wg9s.org/mozilla/firefox/ include this
> patch.

Oops. http://www.wg9s.com/mozilla/firefox/
(In reply to comment #30)
> hrm, so I've compensated for the auto-hide feature, but screwed up the non-auto
> hide window. wonderful. :/

It would seem that either we somehow need to detect that we are not at the bottom of the window and therefore not do the -1 thing or upon a resize resend or size including the minus 1 compensation back to the window manager so that the background is repainted for that a pixel row.
(In reply to comment #33)
> It would seem that either we somehow need to detect that we are not at the
> bottom of the window and therefore not do the -1 thing or upon a resize resend

I meant bottom of the screen not bottom of the window here.
(In reply to comment #33)

> It would seem that either we somehow need to detect that we are not at the
> bottom of the window and therefore not do the -1 thing or upon a resize resend
> or size including the minus 1 compensation back to the window manager so that
> the background is repainted for that a pixel row.

OK fixing enough typos to make this followable:

It would seem that either we somehow need to detect that we are not at the
bottom of the screen and therefore not do the -1 thing, or upon a resize resend
our size, including the minus 1 compensation, back to the window manager so that
the background is repainted for that pixel row.
Test build works like it should.
(In reply to comment #36)
> Test build works like it should.

I'll second that.
Well that of course assumes that the taskbar is at the bottom...
(In reply to comment #35)
> (In reply to comment #33)
> 
> > It would seem that either we somehow need to detect that we are not at the
> > bottom of the window and therefore not do the -1 thing or upon a resize resend
> > or size including the minus 1 compensation back to the window manager so that
> > the background is repainted for that a pixel row.
> 
> OK fixing enough typos to make this followable:
> 
> It would seem that either we somehow need to detect that we are not at the
> bottom of the screen and therefore not do the -1 thing, or upon a resize resend
> our size, including the minus 1 compensation, back to the window manager so
> that
> the background is repainted for that pixel row.

That's what the previous patch did, but I sense we are missing a recalculation on the margins when switching from maximized to normalized.
(In reply to comment #27)
> Created an attachment (id=454980) [details]
> a picture is worth a thousand words
> 
> This does not look like it is supposed to be there.  See how under the word
> done int he status bar there are artifacts of the portion of the tinderbox
> sidebar previously displayed on that portion of the screen.  Also not the
> artifacts of text farther over to the right in the portion of the statusbar
> below the content area.  It seems that somehow in some situations when the
> window is maximized and the status bar is not in auto-hide, the bottom row of
> pixels in the Firefox window is not being painted.

Hmm, maybe I'm misinterpreting the problem. What steps did you use to produce this? I don't use auto-hide. When I go to maximized with this patch, everything looks right.
Attached image status bars
Just for reference, this is what I'm seeing.
Attachment #455177 - Attachment is patch: false
Attachment #455177 - Attachment mime type: text/plain → image/png
(In reply to comment #42)
> Created an attachment (id=455177) [details]
> status bars
> 
> Just for reference, this is what I'm seeing.

The upper one with the tiny vertical lines are caused by icons on the taskbar that are opened.  They are the left and right borders of the icons.  Only a few pixels worth as it should be.  Not sure what the bottom one is all about.
(In reply to comment #41)
> (In reply to comment #27)
> > Created an attachment (id=454980) [details] [details]
> > a picture is worth a thousand words
> > 
> > This does not look like it is supposed to be there.  See how under the word
> > done int he status bar there are artifacts of the portion of the tinderbox
> > sidebar previously displayed on that portion of the screen.  Also not the
> > artifacts of text farther over to the right in the portion of the statusbar
> > below the content area.  It seems that somehow in some situations when the
> > window is maximized and the status bar is not in auto-hide, the bottom row of
> > pixels in the Firefox window is not being painted.
> 
> Hmm, maybe I'm misinterpreting the problem. What steps did you use to produce
> this? I don't use auto-hide. When I go to maximized with this patch, everything
> looks right.

Steps to reproduce.

1.  Set the taskbar to autohide.
2.  Maximize the browser window.
3.  Navigate to a content page that exceeds the vertical size of the screen.
4.  move the mouse to the bottom of the screen to unhide the taskbar and make sure the top edge of the taskbar comes to be in the middle of a line of text or in the middle of an image or a colored background segment.  If not, use the vertical scrollbar to reposition the content an retry moving the mouse to bottom edge of screen until this condition is met.
5.  Unhide the taskbar.  You will now see artifacts from the previous display along the bottom edge of the Firefox window below the statusbar.
Sorry Bill I tried to reproduce what you are seeing and I can't.  Everything appears fine on my end.
That seems odd.  I can reproduce this on both my windows 7 and Vista laptops rather easily.  I do normally have my taskbar double the normal height which does make it easier to reproduce.  I wonder if it is a screen resolution thing?  The Vista laptop is set at 1680X1050 not sure about the windows 7 one cause it is at home and I am not at the moment.

I also verified that D2D is not an issue here, i can reproduce this both with it enabled and disabled.

I am also fairly certain that the first time i saw these artifacts I was not playing with toggling between hiding and unhiding the taskbar, but I could be wrong there.

This is the only method I have found to reliably reproduce this.
I tried doubling my taskbar but still can't reproduce what you are seeing.  I run at 1920x1200 btw.

Why not submit the patch and see what others users experience?  After all it's not working now anyways.
Kind of what i think.  Land this patch and I will file a new bug on this issue.
(In reply to comment #48)
> Kind of what i think.  Land this patch and I will file a new bug on this issue.

Bill, do you have spy++ on your system? I'd like to get some measurements of your client area. Also, what are your dpi settings?
Bill, could your problem be related to your taskbar under Windows 7 rather then a Fx problem? I'm mentioning this because others and myself stated that they don't have a problem.  Is there something different about your setup especially the taskbar?  Anything on it that might be the culprit?

I tried a DPI of 120 (my default) and 96 as well as changing resolutions but I still can't duplicate what Bill is seeing.
OK, so I was able to get hold of Word 2007 running on Vista, and discovered that its hack is just as bad as Jim's, if not worse: it simply will not allow the client area to touch the bottom of the screen, whether the taskbar is set to autohide or not. And it doesn't seem to paint it either, so you get random artifacts. In the default case (non-auto-hide bottom taskbar) its client area doesn't need to touch the bottom of the screen, so the hack doesn't get used, which is presumably how they manage to get away with it.
(In reply to comment #51)
> OK, so I was able to get hold of Word 2007 running on Vista, and discovered
> that its hack is just as bad as Jim's, if not worse: it simply will not allow
> the client area to touch the bottom of the screen, whether the taskbar is set
> to autohide or not. And it doesn't seem to paint it either, so you get random
> artifacts. In the default case (non-auto-hide bottom taskbar) its client area
> doesn't need to touch the bottom of the screen, so the hack doesn't get used,
> which is presumably how they manage to get away with it.

What do you mean by Word 2007's hack?  I've never had a problem with Word or any other program with the taskbar on Vista or W7.  I do have a number of hotfixes on my system I'll take a look and see if any pertain to the taskbar.
(In reply to comment #49)
> (In reply to comment #48)
> > Kind of what i think.  Land this patch and I will file a new bug on this issue.
> 
> Bill, do you have spy++ on your system? I'd like to get some measurements of
> your client area. Also, what are your dpi settings?

I do not have spy++, I am running VC express and not the full product.  I tried running managed spy but it does not run correctly possible because it is a 64-bit OS I am not sure.

I really don't understand why I see this when others do not.  I ma seeing this on both my personal computer which is Windows 7 64-bit with an AMD processor and ATI graphics, and on my work computer which is Vista 32-bit on a 645-bit Intel processor with NVIDIA graphics.  I use the work computer both undocked and with 2 completely different monitors with totally different aspect ratios and resolutions.

I am beginning to think that the issue might be specific to my build.  Is there a tryserver build someplace with this patch that I can try?  I will try crating a new build later on today when I get time that includes just this one patch without all the other patches I have in the build just in case one of them impacts this issue.
Oh I forgot to mention, I tired fresh profile, default toolbar setup no extensions etc.  and I still see the issue.
Land the patch and let the beta testers sort it out for you.  It's 3 to 1 that the patch works. 

Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100702 Minefield/4.0b2pre - Build ID: 20100702152157
(In reply to comment #54)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > Kind of what i think.  Land this patch and I will file a new bug on this issue.
> > 
> > Bill, do you have spy++ on your system? I'd like to get some measurements of
> > your client area. Also, what are your dpi settings?
> 
> I do not have spy++, I am running VC express and not the full product.  I tried
> running managed spy but it does not run correctly possible because it is a
> 64-bit OS I am not sure.
> 
> I really don't understand why I see this when others do not.  I ma seeing this
> on both my personal computer which is Windows 7 64-bit with an AMD processor
> and ATI graphics, and on my work computer which is Vista 32-bit on a 645-bit
> Intel processor with NVIDIA graphics.  I use the work computer both undocked
> and with 2 completely different monitors with totally different aspect ratios
> and resolutions.
> 
> I am beginning to think that the issue might be specific to my build.  Is there
> a tryserver build someplace with this patch that I can try?  I will try crating
> a new build later on today when I get time that includes just this one patch
> without all the other patches I have in the build just in case one of them
> impacts this issue.

Let me fire up a try build w/latest trunk and this patch. Will post back.
(In reply to comment #57)
> Let me fire up a try build w/latest trunk and this patch. Will post back.

For my part I tried a build from trunk with just this patch and it works the same for me.  Also, in trying to determine f this issue exists without the patch you previously had on this bug, I found a new way to reproduce it not dependent on auto-hiding the taskbar.

This is simpler, merely have the taskbar be normal height and display vertical color bars on the screen that are taller than the window height.

Then just maximize the Firefox window and increase the height of the taskbar.

With this patch included, I get artifacts of the color bars on the bottom pixel row of the firefox window,  Without this patch I do not.
(In reply to comment #58)
> (In reply to comment #57)
> > Let me fire up a try build w/latest trunk and this patch. Will post back.
> 
> For my part I tried a build from trunk with just this patch and it works the
> same for me.  Also, in trying to determine f this issue exists without the
> patch you previously had on this bug, I found a new way to reproduce it not
> dependent on auto-hiding the taskbar.
> 
> This is simpler, merely have the taskbar be normal height and display vertical
> color bars on the screen that are taller than the window height.
> 
> Then just maximize the Firefox window and increase the height of the taskbar.
> 
> With this patch included, I get artifacts of the color bars on the bottom pixel
> row of the firefox window,  Without this patch I do not.

I am finally able to reproduce the problem using your new method.  Pretty unlikely that folks will do this in real life.
Are you certain this isn't a new bug?  When resizing a window where the task bar is always visible the windows should adjust it's height so that the entire taskbar is visible.  In this case the height of the window is probably calculated incorrectly.  Seems to me this is different then the problem you were having originally that no one can duplicate.
(In reply to comment #60)
> When resizing a window where the task
> bar is always visible the windows should adjust it's height so that the entire
> taskbar is visible.  In this case the height of the window is probably
calculated incorrectly.

This part seems to be working ok in the most recent nightly. When the taskbar is set to the side, maximizing the browser, and unlocking the taskbar, adjust its width left and stop, the browser window adjusts, adjust the taskbar back to minimum widget, the browser adjusts to maximum width. In normal mode the browser would not increase in size to compensate for the making the taskbar width smaller, only adjusting to the taskbar getting bigger.

FWIW, I see the original problem on windows 7.

(In reply to comment #3)
> Auto unhide of the taskbar also fails to work no matter what the sate of
> taskbar auto-hide or maximized window was on startup, if first unhide the
> taskbar and then hide it while firefox is maximized, this also triggers the
> issue.

Autohide was working for me with a Maximized window on the bottom of the screen, changing its height didn't affect the Z-order.  It wasn't until I started testing Autohide from the left side of the screen with a maximized window and I the browser window has focus that I noticed this bug here.  

I then unmaximized the window, this alone didn't give the taskbar the z-order.  
I had to focus the taskbar to get it be on top again.  Then autohide worked on a normal window.

The taskbar min height seems be only 1 px so I see glowing running taskbar icons on the edge of the autohide taskbar.

So it seems that what neil said about the z-order, its like maximized, seems to give z-ordering like fullscreen does seems right.  

The screenshots only show the 1px edge of the taskbar items.  This I don't think is an issue.

With the test build, everything seems to be working and matches Word behavior.
(In reply to comment #61)
> (In reply to comment #60)
> > When resizing a window where the task
> > bar is always visible the windows should adjust it's height so that the entire
> > taskbar is visible.  In this case the height of the window is probably
> calculated incorrectly.
> 
> This part seems to be working ok in the most recent nightly. When the taskbar
> is set to the side, maximizing the browser, and unlocking the taskbar, adjust
> its width left and stop, the browser window adjusts, adjust the taskbar back to
> minimum widget, the browser adjusts to maximum width. In normal mode the
> browser would not increase in size to compensate for the making the taskbar
> width smaller, only adjusting to the taskbar getting bigger.

s/widget/width 

> With the test build, everything seems to be working and matches Word behavior.

build from 6be9f030c3f8 - Jun 2 - 23:33
(In reply to comment #60)
> Are you certain this isn't a new bug?  When resizing a window where the task
> bar is always visible the windows should adjust it's height so that the entire
> taskbar is visible.  In this case the height of the window is probably
> calculated incorrectly.  Seems to me this is different then the problem you
> were having originally that no one can duplicate.

I don't see why you think anything is different between that and my first steps to reproduce.  The only difference is that in my original case the starting height of the taskbar is zero.
(In reply to comment #63)
> (In reply to comment #60)
> > Are you certain this isn't a new bug?  When resizing a window where the task
> > bar is always visible.  In this case the height of the window is probably
> > calculated incorrectly.  Seems to me this is different then the problem you
> > were having originally that no one can duplicate.
> 
> I don't see why you think anything is different between that and my first steps
> to reproduce.  The only difference is that in my original case the starting
> height of the taskbar is zero.

I suspect you are NOT correctly following my steps to reproduce.

1.  Have the taskbar set to autohide and maximize the browser.
2.  move the mouse to screen bottom so that the taskbar unhides.
3   right click on the unhidden taskbar, select properties and uncheck "auto-hide the taskbar" so that, to quote you, "the windows should adjust it's height so that the entire taskbar is visible."

If you don't think this is really the same, I suspect you are NOT really following step 3.
(In reply to comment #59)
> I am finally able to reproduce the problem using your new method.  Pretty
> unlikely that folks will do this in real life.

Actually I would suspect it to be more likely someone would decide to increase the height of the taskbar as they open more windows, that that people would be toggleing the auto-hide taskbar option back and forth.
I still say land the patch and let the beta testers supply any bug problems.  We can argue all day about reproducing the problem.  When the patch is on my system I will surely run into it if it exists.
(In reply to comment #66)
> I still say land the patch and let the beta testers supply any bug problems. 
> We can argue all day about reproducing the problem.  When the patch is on my
> system I will surely run into it if it exists.

Well:

1.  It is well past the deadline for making beta1 with a fix for this.

2.  Although on occasion, wallpaper type fixes are taken for a release branch because we don't have a real fix that can make it in time for the release, in general only actual real fixes for issues are landed on the trunk.

3.  So the real push here is to get this fixed correctly for beta2 which is still quite a ways off.  Anything other than a real fix that does not cause regressions is unlikely to land at this point.  Please be patienter.  People are really working on a real fix that does not cause regressions.  I actually agree with you that the regression here is not as bad as the issue to be fixed, but there is really no reason for a rush to fix at this point.
Attached patch fix v2 (obsolete) — Splinter Review
This is the same patch that Jim posted earlier with the following changes.

1.  The adjustment to the offset in UpadteNonClientMargins is now only done when the taskbar is hidden.

2.  I added code to call UpdateNonClientMargins when a maximized window is resized.

This seems to fix the original issue without cuasing the regression introduced by the earlier patch.
If you are confident with the patch let's get it approved and land it on Trunk ASAP.
Attachment #456505 - Flags: review?(neil)
Assignee: jmathies → bill
I'm currently on a vista laptop, this new patch fixes the old auto hide problem there.
(In reply to comment #71)
> I'm currently on a vista laptop, this new patch fixes the old auto hide problem
> there.

Good to here.  I don't have my Vista laptop with me, so could only test on Windows 7 myself.
+    RECT r;
+    if (::GetWindowRect(mWnd, &r) &&
+       (r.bottom - r.top) > GetSystemMetrics(SM_CYSCREEN)) {

One possible issue here - SM_CYSCREEN returns the height of the primary display. So say we're maxed on a secondary monitor of smaller height, this code will not kick in. We might look at an api like MonitorFromWindow for getting the info for the display we're currently on.
I had already realized that.  I have a good setup for testing this with dual monitors that have different rssolutions and aspect ratios in my office at work.

I can probably wright code that should work correctly, but will not be able to really do thorough testing until next week.
(In reply to comment #74)
> I can probably wright code that should work correctly, but will not be able to
                 ^^^^^^
                 write
> really do thorough testing until next week.
(In reply to comment #73)
> One possible issue here - SM_CYSCREEN returns the height of the primary
> display. So say we're maxed on a secondary monitor of smaller height, this code
> will not kick in. We might look at an api like MonitorFromWindow for getting
> the info for the display we're currently on.

I ended up using a trick to not have to add anymore API calls.  Adding more API calls seemed problematic as the ones I wanted to add were not supported on Windows/CE so would have required special case ifdefs to get builds to work and I really had no good way to test.  It occurred to me that since the adjustment was to make taskbar auto-hide work, and the taskbar is only displayed on the primary monitor, the secondary monitor case was a simple never adjust.  It also occurred to me that we know that a maximized chromeless window on the primary monitor has an origin of (-8,-8) I could just test for that.

I made a new patch with a mess of debug prints in it and tested it with my 2 monitor setup at work and it works great!  I will clean it up and remove the debug prints and post it here by tomorrow.
Attached patch fix v3 (obsolete) — Splinter Review
This patch has been tested and works correctly in configurations including multiple monitors with differing resolutions and aspect ratios.  It also uses only Windows API calls which I have verified are available in Windows/CE 3.0.
Attachment #456505 - Attachment is obsolete: true
Attachment #456505 - Flags: review?(neil)
Attached patch fix v3 (obsolete) — Splinter Review
indentation fix
Attachment #456957 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #456960 - Flags: review?(neil)
So...... are we almost ready to land this?
(In reply to comment #81)
> So...... are we almost ready to land this?

It needs reviews first.
(In reply to comment #52)
> What do you mean by Word 2007's hack?  I've never had a problem with Word or
> any other program with the taskbar on Vista or W7.
Very simply, if for whatever reason Word 2007's window should touch the bottom of the (presumably primary, but I don't have access to multiple monitors either) screen, its client height is reduced by one pixel. Although this allows the taskbar to auto-unhide, it's overkill as it applies in unrelated cases, and it means that the bottom line of the screen never paints properly.
Comment on attachment 456960 [details] [diff] [review]
fix v3

>+  if (aSizeMode == nsSizeMode_Maximized) {
>+    // Addresses an issue with auto-hide taskbars which fall behind the
>+    // fx window due to our client area being 1 px too low.
Well, the "bug" is caused because disabling the caption increases our client area to the whole screen, which makes the taskbar think that we're full screen.

>+    RECT r;
>+    if (::GetWindowRect(mWnd, &r) && r.top == -8 && r.left == -8 &&
>+       (r.bottom - r.top) > GetSystemMetrics(SM_CYSCREEN)) {
>+      // Adjustment is only necessary if:
>+      // 1.  We are on the primay screen (which for a chomeless window can be
>+      //     detected by comparing the origin with -8, -8).
That depends on the left/right margins being -8. I don't think hardcoding them is a good idea.

>+      // 2.  the window height is > than the screen height (which only occurs
>+      //     in the taskbar auto-hide case).
You also need to check the window width. What you should really do is to subtract (mCaptionHeight, mHorResizeMargin, mVertResizeMargin, mHorResizeMargin) from the window rect, which gives you the standard client rect, then add on the non client offsets, and then see whether the resulting rectangle overlaps the primary monitor. If it does you need to reduce its height to fit.

>+      // The origin of the client area is equal to -mVertResizeMargin (above
>+      // the screen). Adjust the bottom rect to account for this.
>+      if (!mNonClientMargins.bottom)
>+        mNonClientOffset.bottom -= mVertResizeMargin;
Not sure what this bit is for.
Attachment #456960 - Flags: review?(neil) → review-
(In reply to comment #84)
> Comment on attachment 456960 [details] [diff] [review]
> fix v3
> 
> >+  if (aSizeMode == nsSizeMode_Maximized) {
> >+    // Addresses an issue with auto-hide taskbars which fall behind the
> >+    // fx window due to our client area being 1 px too low.
> Well, the "bug" is caused because disabling the caption increases our client
> area to the whole screen, which makes the taskbar think that we're full screen.

Actually, I think the "bug" is caused by a bug in the Windows Destop Manager, otherwise I really don't see why we don't have the same issue when the taskbar is positioned at screen right.  The other areas of this fix not related to the 1px adjustment being done here are sufficient to make taskbar auto-hide/unhide work correctly when the taskbar is on the left, right or top of the screen.

In any event that comment is copied from Jim's original patch, I can reword it as you suggest.  Maybe something simpler like.

//  Leave a 1px margin at the bottom of the screen to allow taskbar auto-hide to function properly.

> 
> >+    RECT r;
> >+    if (::GetWindowRect(mWnd, &r) && r.top == -8 && r.left == -8 &&
> >+       (r.bottom - r.top) > GetSystemMetrics(SM_CYSCREEN)) {
> >+      // Adjustment is only necessary if:
> >+      // 1.  We are on the primay screen (which for a chomeless window can be
> >+      //     detected by comparing the origin with -8, -8).
> That depends on the left/right margins being -8. I don't think hardcoding them
> is a good idea.

I was not really comfortable hardcoding this dependency either.  There are other areas of the code that depend on this and have the magic number 8 hardcoded, but that is Firefox UI code and this I guess would be the only place where we depend on this in Gecko code.  So I am back to doing this the less kludgy way that will probably break WINCE builds, so after I get a patch with an r+ here I will run it by the WINCE team to have them tell me how to change it so it will build for them.

> 
> >+      // 2.  the window height is > than the screen height (which only occurs
> >+      //     in the taskbar auto-hide case).
> You also need to check the window width. What you should really do is to
> subtract (mCaptionHeight, mHorResizeMargin, mVertResizeMargin,
> mHorResizeMargin) from the window rect, which gives you the standard client
> rect, then add on the non client offsets, and then see whether the resulting
> rectangle overlaps the primary monitor. If it does you need to reduce its
> height to fit.

I did not think Windows supported a maximized window that did not have it's origin on the primary monitor to extend onto the primary monitor.  But if I change this to not depend on the -8,-8 "trick", then it is probably not much harder to account for that even if it turns out that it is an impossible conditiaon under current windows versions, becuase it might become a feature in the future.

If you actually know of a way to have a maximized window behave this way under Windows 7 or Vista, please let me know so that I can test my next patch under those conditions as well.

> 
> >+      // The origin of the client area is equal to -mVertResizeMargin (above
> >+      // the screen). Adjust the bottom rect to account for this.
> >+      if (!mNonClientMargins.bottom)
> >+        mNonClientOffset.bottom -= mVertResizeMargin;
> Not sure what this bit is for.

This was part of Jim's original patch so he will have to answer for that.
(In reply to comment #85)
> > That depends on the left/right margins being -8. I don't think hardcoding them
> > is a good idea.
> 
> I was not really comfortable hardcoding this dependency either.  There are
> other areas of the code that depend on this and have the magic number 8
> hardcoded, but that is Firefox UI code and this I guess would be the only place
> where we depend on this in Gecko code.  So I am back to doing this the less
> kludgy way that will probably break WINCE builds, so after I get a patch with
> an r+ here I will run it by the WINCE team to have them tell me how to change
> it so it will build for them.
> 
> > 
> > >+      // 2.  the window height is > than the screen height (which only occurs
> > >+      //     in the taskbar auto-hide case).
> > You also need to check the window width. What you should really do is to
> > subtract (mCaptionHeight, mHorResizeMargin, mVertResizeMargin,
> > mHorResizeMargin) from the window rect, which gives you the standard client
> > rect, then add on the non client offsets, and then see whether the resulting
> > rectangle overlaps the primary monitor. If it does you need to reduce its
> > height to fit.
> 

Oh.  Silly me.  I see what you are saying.  If i take the rect add in the nonclient offsets and then compare this with the primary monitor coordinates which go from 0 top; 0 left; to SM_CYSCREEN bottom; SM_CXSCREN right,  and see if there is an intersection I can still do this without using any API calls not supported in WINCE.
> > >+      // The origin of the client area is equal to -mVertResizeMargin (above
> > >+      // the screen). Adjust the bottom rect to account for this.
> > >+      if (!mNonClientMargins.bottom)
> > >+        mNonClientOffset.bottom -= mVertResizeMargin;
> > Not sure what this bit is for.
> 
> This was part of Jim's original patch so he will have to answer for that.

Not sure if this is still needed. Originally when i tested this the lower client area was getting clipped. This might be addressed by the patch in a different way.

Also, don't worry about windows ce, we halted development on that platform a while back.
Attached patch fix v4 (obsolete) — Splinter Review
I think this addresses all the previous review comments.
Attachment #456960 - Attachment is obsolete: true
Attachment #457445 - Flags: review?(neil)
Attached patch fix v5 (obsolete) — Splinter Review
Oops.  Noticed a typo.
Attachment #457445 - Attachment is obsolete: true
Attachment #457447 - Flags: review?(neil)
Attachment #457445 - Flags: review?(neil)
Updated summary to try to prevent some of the dupes.
Summary: Z-Level issues with the windows taskbar and browser windows that display the firefox button → Z-Level issues with the windows taskbar and browser windows that display the firefox button (auto-hide)
We should recheck this with the new functionality in UpdateNonClientMargins which triggers a reflow of content now.
This bug doesn't appear to have changed at all with that bug fixed.
Attached patch fix v6 (obsolete) — Splinter Review
Un bit-rot the previous patch and also prevent an extra gecko resize event now that UpdateNonClientMargins can trigger a call to OnResize.
Attachment #457447 - Attachment is obsolete: true
Attachment #457811 - Flags: review?(neil)
Attachment #457447 - Flags: review?(neil)
Does this patch take into account having the autohide taskbar on left, right or top instead of the bottom?
With this patch applied, the taskbar auto-hides correctly regardless of it's position.  I have test builds that include this patch at http://www.wg9s.com/mozilla/firefox/  if you wish to do your own testing.
Guys, this patch seems to be working!
Comment on attachment 457811 [details] [diff] [review]
fix v6

I found one more thing that can be optimized here.  I will post what I hope will be the final patch once it passes my tests.
Attachment #457811 - Flags: review?(neil)
Moving to betaN for now, hopefully we'll get this in beta3.
blocking2.0: beta2+ → betaN+
Attached patch fix v7 (obsolete) — Splinter Review
This is the same patch but reordered to avoid defining the rect for the OnResize call in the case when we are not going to do the call.
Attachment #457811 - Attachment is obsolete: true
Attachment #458638 - Flags: review?
Attachment #458638 - Flags: review? → review?(neil)
Holding this off for Beta3, IMO, is going to cause somewhat of an uproar from those using Beta2 when released to the general public.  At the very least I think you will get negative press on this given the vast number of Windows users.
Comment on attachment 458638 [details] [diff] [review]
fix v7

>+      // Adjust window rect to account for non-client margins.
>+      // r.top += mVertResizeMargin;  (no need to adjust top as it is not used)
>+      r.left += mHorResizeMargin;
>+      r.bottom -= mVertResizeMargin;
>+      r.right -= mHorResizeMargin;
I think you then need to subtract the non client offset values, in case they are non-zero.

>+      // We need to leave the 1 pixel margin if the bottom of the window aligns
>+      // with the bottom of the primary monitor and the horizontal postion of
>+      // the window in any way overlaps the primary monitor.
>+      if (r.bottom == GetSystemMetrics(SM_CYSCREEN) &&
>+         ((r.left >= 0 && r.left < GetSystemMetrics(SM_CXSCREEN)) ||
>+         (r.left < 0 && r.right >= 0))) {
I'm not sure this is correct. I think the correct condition is that the window covers the primary monitor i.e. r.top <= 0 && r.left <= 0 && r.right >= GetSystemMetrics(SM_CXSCREEN). It would be nice if you could cope with r.bottom >= GetSystemMetrics(SM_CYSCREEN) as well (this varies the size of the adjustment) but I'll accept == if it's too hard.

>+      if (result = UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE))
What does this assignment achieve?
(In reply to comment #104)
> Comment on attachment 458638 [details] [diff] [review]
> fix v7
> 
> >+      // Adjust window rect to account for non-client margins.
> >+      // r.top += mVertResizeMargin;  (no need to adjust top as it is not used)
> >+      r.left += mHorResizeMargin;
> >+      r.bottom -= mVertResizeMargin;
> >+      r.right -= mHorResizeMargin;
> I think you then need to subtract the non client offset values, in case they
> are non-zero.

I think that makes sense.  I will test that.

> 
> >+      // We need to leave the 1 pixel margin if the bottom of the window aligns
> >+      // with the bottom of the primary monitor and the horizontal postion of
> >+      // the window in any way overlaps the primary monitor.
> >+      if (r.bottom == GetSystemMetrics(SM_CYSCREEN) &&
> >+         ((r.left >= 0 && r.left < GetSystemMetrics(SM_CXSCREEN)) ||
> >+         (r.left < 0 && r.right >= 0))) {
> I'm not sure this is correct. I think the correct condition is that the window
> covers the primary monitor i.e. r.top <= 0 && r.left <= 0 && r.right >=
> GetSystemMetrics(SM_CXSCREEN). 

Well I was trying to deal with chromeless windows and the Windows 7 snap feature.  Not sure if this is currently supported on windows 7 but thought it might be in future, even if it isn't now, so I was worried that if a Firefox chromeless window was on the left half of the screen and the  taskbar was at bottom hidden with another application on the right then you might only be able to un-autohide on the right hand half of the screen because Firefox would still need to leave the margin in that case.

> It would be nice if you could cope with r.bottom
> >= GetSystemMetrics(SM_CYSCREEN) as well (this varies the size of the
> adjustment) but I'll accept == if it's too hard.

I could try to handle that as well.

> 
> >+      if (result = UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE))
> What does this assignment achieve?

The idea was to set result (which is a PR_BOOL return value of OnWindowPosChanged) to PR_TRUE without needing to do it in a separate statement, as in:

          if (UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE)) {
            result = PR_TRUE;
            return;
          }

In the case where we were returning with doing the OnResize here.  Normally it would be set to the value returned by the OnResize call.
Attached file patch v8 (obsolete) —
Addresses previous review comments.
Attachment #458638 - Attachment is obsolete: true
Attachment #458993 - Flags: review?(neil)
Attachment #458638 - Flags: review?(neil)
Attached patch patch v8 (obsolete) — Splinter Review
Hmm. I must have screwed up uploading the patch.
Attachment #458993 - Attachment is obsolete: true
Attachment #458995 - Flags: review?(neil)
Attachment #458993 - Flags: review?(neil)
(In reply to comment #105)
> Well I was trying to deal with chromeless windows and the Windows 7 snap
> feature.  Not sure if this is currently supported on windows 7 but thought it
> might be in future, even if it isn't now, so I was worried that if a Firefox
> chromeless window was on the left half of the screen and the  taskbar was at
> bottom hidden with another application on the right then you might only be able
> to un-autohide on the right hand half of the screen because Firefox would still
> need to leave the margin in that case.
I might be wrong, but I don't think that counts as a maximised window.

> > >+      if (result = UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE))
> > What does this assignment achieve?
> The idea was to set result (which is a PR_BOOL return value of
> OnWindowPosChanged) to PR_TRUE without needing to do it in a separate
> statement, as in:
Sorry, I hadn't noticed that it was a return value.
(In reply to comment #108)
> (In reply to comment #105)
> > Well I was trying to deal with chromeless windows and the Windows 7 snap
> > feature.  Not sure if this is currently supported on windows 7 but thought it
> > might be in future, even if it isn't now, so I was worried that if a Firefox
> > chromeless window was on the left half of the screen and the  taskbar was at
> > bottom hidden with another application on the right then you might only be able
> > to un-autohide on the right hand half of the screen because Firefox would still
> > need to leave the margin in that case.
> I might be wrong, but I don't think that counts as a maximised window.

I did testing and it does not count as a maximized window.  So, this code would not handle that case anyway.

Therefore, I converted to your suggestion for testing if we were on the primary monitor.  I think I applied the offsets correctly based on code elsewhere.  The offsets were harder to figure out the correct add or subtract since for all the cases I tested they were zero except for the top one.  The margins were easy to figure out because they were both 8 and the rect had coordinates of (-8,-8) (1448, 908) and my screen resolution was 1440x900.  From code elsewhere it seemed the correct thing to do was to add the offset to the coordinates where you subtract the margin and vice-versa.

> 
> > > >+      if (result = UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE))
> > > What does this assignment achieve?
> > The idea was to set result (which is a PR_BOOL return value of
> > OnWindowPosChanged) to PR_TRUE without needing to do it in a separate
> > statement, as in:
> Sorry, I hadn't noticed that it was a return value.

That is OK.  I decided if it confused you, it was probably too confusing, so I avoided the assignment lest some future hacker decided it was wrong and changed the '=' to '==' and made it do something random.
Attached patch patch v9 (obsolete) — Splinter Review
Oh dear I just noticed that in my haste in removing the assignment from inside the if, I forgot to add the braces ... sigh.
Attachment #458995 - Attachment is obsolete: true
Attachment #459010 - Flags: review?(neil)
Attachment #458995 - Flags: review?(neil)
I know you are giving 100% on this bug.  When do you think it might get approved and land on trunk?
(In reply to comment #111)
> I know you are giving 100% on this bug.  When do you think it might get
> approved and land on trunk?
Target Milestone: --- → mozilla2.0b3
(In reply to comment #112)
> (In reply to comment #111)
> > I know you are giving 100% on this bug.  When do you think it might get
> > approved and land on trunk?

Oops meant to add I changed the target to beta3.  Hopefully, it will land well before then.
Comment on attachment 459010 [details] [diff] [review]
patch v9

>+      r.top = r.top + mVertResizeMargin + mCaptionHeight -
>+        mNonClientOffset.top;
mCaptionHeight already includes mVertResizeMargin, so you don't need both (r.top may be < 0 for a maximised captionless window). r=me with this fixed.

>+        PRInt32 screeny = GetSystemMetrics(SM_CYSCREEN);
Nit: Please name this screenY. Also this is an int rather than a PRInt32.

>+          mNonClientOffset.bottom = mNonClientOffset.bottom - r.bottom +
>+            screeny - 1;
This (and similarly += for the coordinates above) may look easier using -= :
mNonClientOffset.bottom -= r.bottom - screeny + 1;
Attachment #459010 - Flags: review?(neil) → review+
(In reply to comment #114)
> Comment on attachment 459010 [details] [diff] [review]
> patch v9
> 
> >+      r.top = r.top + mVertResizeMargin + mCaptionHeight -
> >+        mNonClientOffset.top;
> mCaptionHeight already includes mVertResizeMargin, so you don't need both
> (r.top may be < 0 for a maximised captionless window). r=me with this fixed.

Well, I originally was not adding the CaptionHeight.  However, when you said I needed to account for the offsets as well as the margins, my debug printing revealed that the top coordinate of the client rectangle when I have a full size maximized window with no taskbar was -8.  mVertResizeMargin was 8 so adding that got me to 0 which is the correct screen coordinate for the top of the screen so all is good.  However, the top offset is 28 so if i subtract that I am no longer at zero.  Adding in mCaptionHeight which is also 28 gets things back aligned with screen coordinates.  Therefore, I concluded that I do need to add mCaptionHeight here to properly convert form client to screen coordinates. 

The other comments all make sense and I will post a patch for check-in once we reach concensus on this mCaptionHeight issue.
>+    // If a Maximized window is resized, we must recalculate the non-client
>+    // margins and determine if we need to leave a 1 pixel margin at screen
>+    // bottom to allow taskbar unhiding to work properly.
>+    if (mSizeMode == nsSizeMode_Maximized) {
>+      if (UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE)) {
>+        // gecko resize event already sent by UpdateNonClientMargins.
>+        result = PR_TRUE;
>+        return;
>+      }
>+    }

Could this potentially go in OnWindowPosChanging? I think we already call UpdateNonClientMargins there in some cases. Might be good to combine them. Calling this in OnWindowPosChanged where we change the margins will trigger another resize event. Doing this during the changing even would prevent that I think.

(In reply to comment #115)
> (In reply to comment #114)
> > Comment on attachment 459010 [details] [diff] [review] [details]
> > patch v9
> > 
> > >+      r.top = r.top + mVertResizeMargin + mCaptionHeight -
> > >+        mNonClientOffset.top;
> > mCaptionHeight already includes mVertResizeMargin, so you don't need both
> > (r.top may be < 0 for a maximised captionless window). r=me with this fixed.
> 
> Well, I originally was not adding the CaptionHeight.  However, when you said I
> needed to account for the offsets as well as the margins, my debug printing
> revealed that the top coordinate of the client rectangle when I have a full
> size maximized window with no taskbar was -8.  mVertResizeMargin was 8 so
> adding that got me to 0 which is the correct screen coordinate for the top of
> the screen so all is good.  However, the top offset is 28 so if i subtract that
> I am no longer at zero.  Adding in mCaptionHeight which is also 28 gets things
> back aligned with screen coordinates.  Therefore, I concluded that I do need to
> add mCaptionHeight here to properly convert form client to screen coordinates. 

Also note, mCaptionHeight / mVertResizeMargin are variable. +/- 8 directly would be bad. Although I'm not seeing that in your patch.

There's an open bug on this related to custom resizing border heights. (bug 581023)
(In reply to comment #116)
> >+    // If a Maximized window is resized, we must recalculate the non-client
> >+    // margins and determine if we need to leave a 1 pixel margin at screen
> >+    // bottom to allow taskbar unhiding to work properly.
> >+    if (mSizeMode == nsSizeMode_Maximized) {
> >+      if (UpdateNonClientMargins(nsSizeMode_Maximized, PR_TRUE)) {
> >+        // gecko resize event already sent by UpdateNonClientMargins.
> >+        result = PR_TRUE;
> >+        return;
> >+      }
> >+    }
> 
> Could this potentially go in OnWindowPosChanging? I think we already call
> UpdateNonClientMargins there in some cases. Might be good to combine them.
> Calling this in OnWindowPosChanged where we change the margins will trigger
> another resize event. Doing this during the changing even would prevent that I
> think.

I tried this and it did not consistently, work especially in the case where you already have the taskbar hidden and open the browser with a non-maximized window and then click on the maximize window control.

I think what is happening is that sometimes it gets to the point where it gets the window rect before the resize is complete and therefore gets the non-maximized size.  So, by trying to deo this during the changing, we get the windows dimensions during the changing instead of the new windows dimensions.
Attached patch patch v10 (obsolete) — Splinter Review
This addresses all the previous review comments with the exception that it is still including mCaptionHeight in the top coordinate adjustment.  As I said in comment #115 my debug printing indicated that the current adjustments are correct.
Attachment #460132 - Flags: superreview?(jmathies)
Attachment #460132 - Flags: review?(neil)
(In reply to comment #115)
>(In reply to comment #114)
>>>+      r.top = r.top + mVertResizeMargin + mCaptionHeight -
>>>+        mNonClientOffset.top;
>>mCaptionHeight already includes mVertResizeMargin, so you don't need both
>>(r.top may be < 0 for a maximised captionless window). r=me with this fixed.
>Well, I originally was not adding the CaptionHeight.  However, when you said I
>needed to account for the offsets as well as the margins, my debug printing
>revealed that the top coordinate of the client rectangle when I have a full
>size maximized window with no taskbar was -8.  mVertResizeMargin was 8 so
>adding that got me to 0 which is the correct screen coordinate for the top of
>the screen so all is good.  However, the top offset is 28 so if i subtract that
>I am no longer at zero.  Adding in mCaptionHeight which is also 28 gets things
>back aligned with screen coordinates.  Therefore, I concluded that I do need to
>add mCaptionHeight here to properly convert form client to screen coordinates. 
Ah, but our non client margins are 0,-1,-1,-1 - this means that the right, bottom and left edges still have their borders, but the top edge has no caption or border. So I would expect the client area to extend above the screen, although I don't know how bug 581023 will affect this.
(In reply to comment #120)
> Ah, but our non client margins are 0,-1,-1,-1 - this means that the right,
> bottom and left edges still have their borders, but the top edge has no caption
> or border. So I would expect the client area to extend above the screen,
> although I don't know how bug 581023 will affect this.

Well, as it turns out, for the purpose of this bug because we are doing this comparison:

if (r.top <= 0

it will work either way.  So, since you think it is more correct I will do a new patch without adding mCaptionHeight.
Attached patch patch v11 (obsolete) — Splinter Review
This addresses all review comments.

However, it is kind of moot.  IT turns out that my assumption that the Windows taskbar could only be on the primary monitor is incorrect.  The patch here works fine for the case where the taskbar is onthe primary montor and also works for taskbar on a different monitor provided the taskbar is not hidden.  What does nto work is the case where the taskbar is at the bottom of a non-primary montor and taskbar autohide is enabled.

I will have a new patch ready and tested in a couple of days that uses MonitorFromWindow and then gets the screen coordinates for the returned monitor.
Attachment #459010 - Attachment is obsolete: true
Attachment #460132 - Attachment is obsolete: true
Attachment #460132 - Flags: superreview?(jmathies)
Attachment #460132 - Flags: review?(neil)
has it been fixed in nightly build??
(In reply to comment #124)
> has it been fixed in nightly build??

Lukes, please, when this bug is marked resolved then it will be posted to the nightlies.  Comment 123 says its not even ready for that and needs a new patch.
Attached patch patch v12 (obsolete) — Splinter Review
This addresses all previous review comments as well as fixing it so that unhiding the taskbar also works if the taskbar is on a monitor other than the primary monitor.

I felt this was too much of a change to carry forward neil's previous r+, so I am asking for new reviews.
Attachment #460532 - Attachment is obsolete: true
Attachment #460981 - Flags: superreview?(jmathies)
Attachment #460981 - Flags: review?(neil)
Comment on attachment 460981 [details] [diff] [review]
patch v12

>+    MONITORINFO info;
>+    info.cbSize = sizeof(MONITORINFO);
I always used to write this as
MONITORINFO info = {sizeof(MONITORINFO)};
Dunno if it makes any difference.

>+    if (::GetMonitorInfo(::MonitorFromWindow(mWnd, MONITOR_DEFAULTTOPRIMARY),
>+                         &info )) {
Why the space after &info?

>+        if (r.top <= info.rcMonitor.top && r.left <= info.rcMonitor.left && 
>+            r.right >= info.rcMonitor.right &&
>+            r.bottom >= info.rcMonitor.bottom)
This looks awkward. Just put each comparison on its own line.
Attachment #460981 - Flags: review?(neil) → review+
(In reply to comment #129)
> Comment on attachment 460981 [details] [diff] [review]
> patch v12
> 
> >+    MONITORINFO info;
> >+    info.cbSize = sizeof(MONITORINFO);
> I always used to write this as
> MONITORINFO info = {sizeof(MONITORINFO)};
> Dunno if it makes any difference.

I don;t know either.  I copied this from elsewhere in the code.
Do you want me to change this, or is this OK?
> 
> >+    if (::GetMonitorInfo(::MonitorFromWindow(mWnd, MONITOR_DEFAULTTOPRIMARY),
> >+                         &info )) {
> Why the space after &info?
> 
> >+        if (r.top <= info.rcMonitor.top && r.left <= info.rcMonitor.left && 
> >+            r.right >= info.rcMonitor.right &&
> >+            r.bottom >= info.rcMonitor.bottom)
> This looks awkward. Just put each comparison on its own line.

OK will do that.
(In reply to comment #130)
> (In reply to comment #129)
> > Comment on attachment 460981 [details] [diff] [review] [details]
> > patch v12
> > 
> > >+    MONITORINFO info;
> > >+    info.cbSize = sizeof(MONITORINFO);
> > I always used to write this as
> > MONITORINFO info = {sizeof(MONITORINFO)};
> > Dunno if it makes any difference.
> 
> I don;t know either.  I copied this from elsewhere in the code.
> Do you want me to change this, or is this OK?

Once again I decide wass is a stupid question.  Your way makes the code easier to follow.  I am doing a new build if my test still pass I will just do it that way.
Attached file lucky #13 (obsolete) —
Carrying forward neil's r+ from previous patch.
Attachment #461075 - Flags: superreview?(jmathies)
Attached patch lucky #13Splinter Review
once again forgot to set correct content type ... sigh
Attachment #461075 - Attachment is obsolete: true
Attachment #461076 - Flags: superreview?(jmathies)
Attachment #461075 - Flags: superreview?(jmathies)
Attachment #460981 - Flags: superreview?(jmathies)
Attachment #461076 - Flags: superreview?(jmathies) → superreview+
Attachment #461076 - Flags: approval2.0?
Comment on attachment 461076 [details] [diff] [review]
lucky #13

Found the rules on dev.planning, blocking 'betaN' looks to be ok for landings without patch approval.
Attachment #461076 - Flags: approval2.0?
Keywords: checkin-needed
Attachment #460981 - Attachment is obsolete: true
I marked the version 12 patch as obsolete to ensure the correct patch is what gets checked in.
Just to make it clear to whoever is processing checkin-needed, for the current patch I am carrying forward an r+ from neil for the version 12 patch.
Did some testing with the Tryserver version and found no problems.  So let's get this checked in and onto Trunk ASAP.
Link for tryserver please?
Will report back any issues.
These extraneous comments that are preventing my comment stating that the patch has an r+ form neil just make it less likely that this patch will land in time for beta3.
(In reply to comment #139)
> Link for tryserver please?
> Will report back any issues.

http://www.wg9s.com/mozilla/firefox/
(In reply to comment #141)
> (In reply to comment #139)
> > Link for tryserver please?
> > Will report back any issues.
> 
> http://www.wg9s.com/mozilla/firefox/

Thankyou.
http://hg.mozilla.org/mozilla-central/rev/20fc55e14e9f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Thanks for all the work on this Bill, and thanks to everyone who helped test!
Lots of hard work will payoff.  Thanks Bill.
wow, finally this bug is fixed! great job everyone!!
Depends on: 629267
I'm experiencing this problem again on my Win10 Desktop (but not on my Surface).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: