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)
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
|
jimm
:
superreview+
|
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.
Reporter | ||
Updated•14 years ago
|
Priority: -- → P1
Reporter | ||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•14 years ago
|
Version: unspecified → Trunk
Reporter | ||
Comment 4•14 years ago
|
||
(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.
Comment 5•14 years ago
|
||
Jimm, Is this related to bug 575222?
Updated•14 years ago
|
Summary: Problems caused by new Titlebar → Z-Level issues with the windows taskbar and browser windows that display the firefox button
Comment 6•14 years ago
|
||
(In reply to comment #5) > Jimm, Is this related to bug 575222? No, strange bug though! I can confirm.
Updated•14 years ago
|
blocking2.0: --- → ?
Reporter | ||
Comment 7•14 years ago
|
||
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.
Reporter | ||
Comment 8•14 years ago
|
||
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.
Comment 9•14 years ago
|
||
(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.
Comment 10•14 years ago
|
||
(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!
Comment 11•14 years ago
|
||
Assignee: nobody → jmathies
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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-
Updated•14 years ago
|
blocking2.0: ? → beta2+
Comment 14•14 years ago
|
||
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)
Reporter | ||
Comment 15•14 years ago
|
||
Jim. Does the handle the situation of the taskbar appearing under a non-maximized window?
Comment 16•14 years ago
|
||
(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 17•14 years ago
|
||
Comment on attachment 454645 [details] [diff] [review] patch v.2 found a bug.
Attachment #454645 -
Attachment is obsolete: true
Attachment #454645 -
Flags: review?(tellrob)
Comment 18•14 years ago
|
||
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)
Comment 19•14 years ago
|
||
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.)
Comment 20•14 years ago
|
||
(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
Comment 21•14 years ago
|
||
for reference, google chrome, which is another app with a custom client area: -8,-8,2568,1608 8,0,2568,1606
Assignee | ||
Comment 22•14 years ago
|
||
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.
Comment 23•14 years ago
|
||
(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.
Reporter | ||
Comment 24•14 years ago
|
||
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?
Comment 25•14 years ago
|
||
(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.
Comment 26•14 years ago
|
||
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.
Assignee | ||
Comment 27•14 years ago
|
||
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.
Assignee | ||
Comment 28•14 years ago
|
||
Oh, I should have mentioned the above screenshot was from Windows 7 so the issue is not Vista only.
Reporter | ||
Comment 29•14 years ago
|
||
Where are you getting the builds with these patches. I'd like to try it out.
Comment 30•14 years ago
|
||
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)
Assignee | ||
Comment 31•14 years ago
|
||
(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.
Assignee | ||
Comment 32•14 years ago
|
||
(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/
Assignee | ||
Comment 33•14 years ago
|
||
(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.
Assignee | ||
Comment 34•14 years ago
|
||
(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.
Assignee | ||
Comment 35•14 years ago
|
||
(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.
Reporter | ||
Comment 36•14 years ago
|
||
Test build works like it should.
Comment 37•14 years ago
|
||
(In reply to comment #36) > Test build works like it should. I'll second that.
Comment 39•14 years ago
|
||
Well that of course assumes that the taskbar is at the bottom...
Comment 40•14 years ago
|
||
(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.
Comment 41•14 years ago
|
||
(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.
Comment 42•14 years ago
|
||
Just for reference, this is what I'm seeing.
Updated•14 years ago
|
Attachment #455177 -
Attachment is patch: false
Attachment #455177 -
Attachment mime type: text/plain → image/png
Reporter | ||
Comment 43•14 years ago
|
||
(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.
Assignee | ||
Comment 44•14 years ago
|
||
(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.
Reporter | ||
Comment 45•14 years ago
|
||
Sorry Bill I tried to reproduce what you are seeing and I can't. Everything appears fine on my end.
Assignee | ||
Comment 46•14 years ago
|
||
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.
Reporter | ||
Comment 47•14 years ago
|
||
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.
Assignee | ||
Comment 48•14 years ago
|
||
Kind of what i think. Land this patch and I will file a new bug on this issue.
Comment 49•14 years ago
|
||
(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?
Reporter | ||
Comment 50•14 years ago
|
||
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.
Comment 51•14 years ago
|
||
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.
Reporter | ||
Comment 52•14 years ago
|
||
(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.
Assignee | ||
Comment 54•14 years ago
|
||
(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.
Assignee | ||
Comment 55•14 years ago
|
||
Oh I forgot to mention, I tired fresh profile, default toolbar setup no extensions etc. and I still see the issue.
Reporter | ||
Comment 56•14 years ago
|
||
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
Comment 57•14 years ago
|
||
(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.
Assignee | ||
Comment 58•14 years ago
|
||
(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.
Reporter | ||
Comment 59•14 years ago
|
||
(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.
Reporter | ||
Comment 60•14 years ago
|
||
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.
Comment 61•14 years ago
|
||
(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.
Comment 62•14 years ago
|
||
(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
Assignee | ||
Comment 63•14 years ago
|
||
(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.
Assignee | ||
Comment 64•14 years ago
|
||
(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.
Assignee | ||
Comment 65•14 years ago
|
||
(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.
Reporter | ||
Comment 66•14 years ago
|
||
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.
Assignee | ||
Comment 67•14 years ago
|
||
(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.
Assignee | ||
Comment 69•14 years ago
|
||
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.
Reporter | ||
Comment 70•14 years ago
|
||
If you are confident with the patch let's get it approved and land it on Trunk ASAP.
Updated•14 years ago
|
Attachment #456505 -
Flags: review?(neil)
Updated•14 years ago
|
Assignee: jmathies → bill
Comment 71•14 years ago
|
||
I'm currently on a vista laptop, this new patch fixes the old auto hide problem there.
Assignee | ||
Comment 72•14 years ago
|
||
(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.
Comment 73•14 years ago
|
||
+ 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.
Assignee | ||
Comment 74•14 years ago
|
||
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.
Assignee | ||
Comment 75•14 years ago
|
||
(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.
Assignee | ||
Comment 78•14 years ago
|
||
(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.
Assignee | ||
Comment 79•14 years ago
|
||
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)
Assignee | ||
Comment 80•14 years ago
|
||
indentation fix
Attachment #456957 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Attachment #456960 -
Flags: review?(neil)
Reporter | ||
Comment 81•14 years ago
|
||
So...... are we almost ready to land this?
Comment 82•14 years ago
|
||
(In reply to comment #81) > So...... are we almost ready to land this? It needs reviews first.
Comment 83•14 years ago
|
||
(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 84•14 years ago
|
||
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-
Assignee | ||
Comment 85•14 years ago
|
||
(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.
Assignee | ||
Comment 87•14 years ago
|
||
(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.
Comment 88•14 years ago
|
||
> > >+ // 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.
Assignee | ||
Comment 89•14 years ago
|
||
I think this addresses all the previous review comments.
Attachment #456960 -
Attachment is obsolete: true
Attachment #457445 -
Flags: review?(neil)
Assignee | ||
Comment 90•14 years ago
|
||
Oops. Noticed a typo.
Attachment #457445 -
Attachment is obsolete: true
Attachment #457447 -
Flags: review?(neil)
Attachment #457445 -
Flags: review?(neil)
Assignee | ||
Comment 92•14 years ago
|
||
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)
Comment 93•14 years ago
|
||
We should recheck this with the new functionality in UpdateNonClientMargins which triggers a reflow of content now.
Comment 94•14 years ago
|
||
This bug doesn't appear to have changed at all with that bug fixed.
Assignee | ||
Comment 95•14 years ago
|
||
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)
Comment 96•14 years ago
|
||
Does this patch take into account having the autohide taskbar on left, right or top instead of the bottom?
Assignee | ||
Comment 97•14 years ago
|
||
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.
Comment 98•14 years ago
|
||
Guys, this patch seems to be working!
Assignee | ||
Comment 99•14 years ago
|
||
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)
Comment 100•14 years ago
|
||
Moving to betaN for now, hopefully we'll get this in beta3.
blocking2.0: beta2+ → betaN+
Assignee | ||
Comment 102•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #458638 -
Flags: review? → review?(neil)
Reporter | ||
Comment 103•14 years ago
|
||
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 104•14 years ago
|
||
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?
Assignee | ||
Comment 105•14 years ago
|
||
(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.
Assignee | ||
Comment 106•14 years ago
|
||
Addresses previous review comments.
Attachment #458638 -
Attachment is obsolete: true
Attachment #458993 -
Flags: review?(neil)
Attachment #458638 -
Flags: review?(neil)
Assignee | ||
Comment 107•14 years ago
|
||
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)
Comment 108•14 years ago
|
||
(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.
Assignee | ||
Comment 109•14 years ago
|
||
(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.
Assignee | ||
Comment 110•14 years ago
|
||
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)
Reporter | ||
Comment 111•14 years ago
|
||
I know you are giving 100% on this bug. When do you think it might get approved and land on trunk?
Assignee | ||
Comment 112•14 years ago
|
||
(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
Assignee | ||
Comment 113•14 years ago
|
||
(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 114•14 years ago
|
||
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+
Assignee | ||
Comment 115•14 years ago
|
||
(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.
Comment 116•14 years ago
|
||
>+ // 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)
Assignee | ||
Comment 118•14 years ago
|
||
(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.
Assignee | ||
Comment 119•14 years ago
|
||
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)
Comment 120•14 years ago
|
||
(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.
Assignee | ||
Comment 121•14 years ago
|
||
(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.
Assignee | ||
Comment 123•14 years ago
|
||
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)
Comment 124•14 years ago
|
||
has it been fixed in nightly build??
Comment 125•14 years ago
|
||
(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.
Assignee | ||
Comment 128•14 years ago
|
||
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 129•14 years ago
|
||
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+
Assignee | ||
Comment 130•14 years ago
|
||
(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.
Assignee | ||
Comment 131•14 years ago
|
||
(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.
Assignee | ||
Comment 132•14 years ago
|
||
Carrying forward neil's r+ from previous patch.
Attachment #461075 -
Flags: superreview?(jmathies)
Assignee | ||
Comment 133•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #460981 -
Flags: superreview?(jmathies)
Updated•14 years ago
|
Attachment #461076 -
Flags: superreview?(jmathies) → superreview+
Updated•14 years ago
|
Attachment #461076 -
Flags: approval2.0?
Comment 134•14 years ago
|
||
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?
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Attachment #460981 -
Attachment is obsolete: true
Assignee | ||
Comment 135•14 years ago
|
||
I marked the version 12 patch as obsolete to ensure the correct patch is what gets checked in.
Assignee | ||
Comment 137•14 years ago
|
||
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.
Reporter | ||
Comment 138•14 years ago
|
||
Did some testing with the Tryserver version and found no problems. So let's get this checked in and onto Trunk ASAP.
Comment 139•14 years ago
|
||
Link for tryserver please? Will report back any issues.
Assignee | ||
Comment 140•14 years ago
|
||
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.
Reporter | ||
Comment 141•14 years ago
|
||
(In reply to comment #139) > Link for tryserver please? > Will report back any issues. http://www.wg9s.com/mozilla/firefox/
Comment 142•14 years ago
|
||
(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.
Comment 143•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/20fc55e14e9f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: checkin-needed
Comment 144•14 years ago
|
||
Thanks for all the work on this Bill, and thanks to everyone who helped test!
Reporter | ||
Comment 145•14 years ago
|
||
Lots of hard work will payoff. Thanks Bill.
Comment 146•14 years ago
|
||
wow, finally this bug is fixed! great job everyone!!
Comment 148•6 years ago
|
||
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.
Description
•