User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 When the statusbar is disabled through the View menu, it becomes difficult to resize the window because of the small size of the window frame. This breaks Windows XP expected behavior. Reproducible: Always When the statusbar is disabled and both scrollbars are active, a resizer gripper should appear in the otherwise wasted space of the "scrollcorner" element. When the statusbar is disabled and only one scrollbar is active then the resizer gripper should be inserted into the corner and the scrollbar should be nudged to make room for it. In Windows XP's file explorer, when the statusbar is disabled and both scrollbars are active, then a resizer does appear in the corner. Windows file explorer does not add the resizer if only one scrollbar is active, but Firefox should.

The current plan for Firefox 4 is remove the status bar entirely. Requesting uiwanted for feedback from the UX team on how to resolve the above problem on each platform.

This blocks the status bar replacement in bug 574688, so is also blocking+.

Pushing this to betaNext. Needs to happen before we release, but not going to hold statusbar removal for it.

Not sure how to create a test for this.

+ if (frameContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::showresizer, NS_LITERAL_STRING("true"), eCaseMatters)) { Just use HasAttr? + if (frameContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::showresizer, NS_LITERAL_STRING("true"), eCaseMatters)) { + reflowScrollCorner = mInner.mCollapsedResizer; + mInner.mCollapsedResizer = PR_FALSE; + } + else { + reflowScrollCorner = !mInner.mCollapsedResizer; + mInner.mCollapsedResizer = PR_TRUE; + } More obvious to write PRBool hasResizer = frameContent->HasAttr(...); reflowScrollCorner = hasResizer != mInner.mCollapsedResizer; mInner.mCollapsedResizer = hasResizer;

+ if (mIsRoot) { + nsIContent* content = mOuter->GetContent(); + if (content) { + nsIDocument* doc = content->GetCurrentDoc(); + if (doc) { + nsPIDOMWindow* win = doc->GetWindow(); + if (win) { + nsCOMPtr<nsIContent> frameContent = do_QueryInterface(win->GetFrameElementInternal()); + if (frameContent && frameContent->NodeInfo()->Equals(nsGkAtoms::browser, kNameSpaceID_XUL)) { + mCollapsedResizer = + !frameContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::showresizer, NS_LITERAL_STRING("true"), eCaseMatters); + } + } + } Maybe the code to set mCollapsedResizer should just be factored out into a helper method, then you can write the previous logic as PRBool hadCollapsedResizer = mInner.mCollapsedResizer; UpdateResizerCollapseState(); reflowScrollCorner = mInner.mCollapsedResizer != hadCollapsedResizer; I expected to see some code that triggers a reflow of the subdocument when the showresizer attribute changes, say in nsSubDocumentFrame::AttributeChanged.

> I expected to see some code that triggers a reflow of the subdocument when the > showresizer attribute changes, say in nsSubDocumentFrame::AttributeChanged. I did too, but I think that showing and hiding the statusbar causes the viewport to resize anyway. I could add an extra check in nsSubDocumentFrame::AttributeChanged in case someone sets showresizer directly though.

Yeah, I don't think we should rely on something else triggering the reflow. That could easily confuse someone who sets showresizer for some reason that doesn't involve the status bar.

Comment on attachment 474792 [details] [diff] [review] updated patch >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >+ <property name="showWindowResizer"> Seems like maybe you should add a property on <browser>s for this, and then make this setter use: this.browsers.forEach(function (b) b.showResizer = val); >+#ifndef XP_MACOSX >+ if (this._showWindowResizer) >+ b.setAttribute("showresizer", "true"); >+#endif >+#ifndef XP_MACOSX >+ if (document.getElementById("status-bar").hidden) >+ this.mCurrentBrowser.setAttribute("showresizer", true); >+#endif Why are these different?

> >+#ifndef XP_MACOSX > >+ if (document.getElementById("status-bar").hidden) > >+ this.mCurrentBrowser.setAttribute("showresizer", true); > >+#endif > > Why are these different? The binding hasn't applied yet I believe.

This patch: - addresses Gavin's comments, and further simplifies code as a result - fixes the accessibility tests - fixes a crash because resize:both was on the wrong element in ua.css - fixes an issue where the collapsed resizer was visible when both of a page's scrollbars were visible (change in nsGfxScrollFrameInner::LayoutScrollbars)

Comment on attachment 476137 [details] [diff] [review] updated patch + nsIDocument* doc = mContent->GetCurrentDoc(); + if (doc) { + nsPIDOMWindow* win = doc->GetWindow(); + if (win) { I don't think you need these null checks. + if (mIsRoot) { + nsIContent* content = mOuter->GetContent(); + if (content) { + nsIDocument* doc = content->GetCurrentDoc(); + if (doc) { + nsPIDOMWindow* win = doc->GetWindow(); + if (win) { Or here.

Comment on attachment 476137 [details] [diff] [review] updated patch >+// Mac has it's own native resizer Nit: its

Comment on attachment 476137 [details] [diff] [review] updated patch This no longer applies now that bug 574688 landed. I guess it should be based on the presence/absence of addon-bar now.

(In reply to comment #16) > Comment on attachment 476137 [details] [diff] [review] > updated patch > > This no longer applies now that bug 574688 landed. I guess it should be based > on the presence/absence of addon-bar now. The add-on bar doesn't have a resizer. There's a hidden one in the mock status bar, though. Not sure if that would be functional.

(In reply to comment #17) > The add-on bar doesn't have a resizer. Does this mean that this bug is no longer relevant? Did I waste my time? Why was this bug created?

I think it's two bugs now: There's no resizer when the add-on bar is hidden, and there's no resizer when the add-on bar is show. I think this bug was created for the former case.

Correct. We need a resizer when the add-on bar is visible. And when it is not. I apologize, it's my fault for not also filing a bug for making a resizer available when the add-on bar is visible.

This patch updates to use the Addon bar instead of the statusbar. Also, I fixed a crash bug by changing resizer.xml to not call getComputedStyle. For some reason this asserts on the root element's resizer. The only minor disadvantage is that if for some reason someone changes the scrollbar side preference, the mouse cursor will be reversed.

I'd pull this back into beta7 if it gets reviewed in time, but otherwise, beta8!

Now that I can get useful information from the tryserver, I noticed couple of failures with this patch. One is that test docshell/test/test_bug404548.html as well as another chrome test crashes without the null-check mentioned in comment 14, so I've added it back in. The second is that the test layout/base/test/chrome/test_chrome_content_integration.xul fails because a resizer is now visible. I don't know what that test is checking for, so I've changed the resizer to only be visible if one of the scrollbars is visible which is perhaps more correct anyway. This patch applies on top of the previous one.

(In reply to comment #23) > The second is that the test > layout/base/test/chrome/test_chrome_content_integration.xul fails because a > resizer is now visible. I don't know what that test is checking for, so I've > changed the resizer to only be visible if one of the scrollbars is visible > which is perhaps more correct anyway. Surely we want to be able to resize the browser window whether or not the content viewport has scrollbars? You could work around that test failure by adding a black element to the XUL window that covers the resizer.

Comment on attachment 480099 [details] [diff] [review] crash and test failure fix See comment #24

I don't think we want the resizer on all platforms. On Windows 7/aero the window border is apparently big enough that it isn't useful (I don't know whether this patch makes it show up there, though). On Linux, it looks kind of strange: http://grab.by/6S1d . It also only appears when there are scroll bars, which isn't exactly what I was expecting, but I guess it makes sense. I wonder whether this should be limited to only Windows XP?

(In reply to comment #26) > I don't think we want the resizer on all platforms. On Windows 7/aero the The resizer isn't the issue; the issue is a large enough target for resizing the window, IMO. On OSX I think we *do* want the resizer beneath the scrollbar; it's what's apropos for that platform. On Linux and Windows we should similarly pick what seems to be de rigeur.

This bug is about adding back resizers, because the drag target is too small, so the resizer is the issue! :) This bug doesn't affect OS X because it already has its own resizers.

Drag target is no smaller than any other windows that just drag based on the window border.

Comment 0 seems to be implying that such windows aren't very common on Windows XP. If that's not a compelling argument, I have no objection to WONTFIXing this bug!

>When the statusbar is disabled and both scrollbars are active, a resizer >gripper should appear in the otherwise wasted space of the "scrollcorner" >element. When the statusbar is disabled and only one scrollbar is active then >the resizer gripper should be inserted into the corner and the scrollbar should >be nudged to make room for it. In Windows XP's file explorer, when the >statusbar is disabled and both scrollbars are active, then a resizer does >appear in the corner. Windows file explorer does not add the resizer if only >one scrollbar is active, but Firefox should. I'm fine with implementing these changes just on XP, my comment #29 was based on Vista and 7, but comment #0 has a point that these types of windows aren't incredibly common on XP (and on XP the window border hover area is rather small target). Placing it under the vertical scroll bar when there is no horizontal scrollbar isn't completely native, but I think it's fair to say that is more of a flaw in the XP design than a feature that we should be consistent with.

In both XP and Windows 2K the window borders are very thin and very difficult to grasp. There are NO native windows which automatically lack a statusbar. If a user chooses to hide the statusbar then you get this as an option. I am fairly sure that this will also effect Mac OSX if a theme chooses -moz-appearance:none to create custom scrollbars. There are bugs in the 3.x (which I assume have not been fixed) that mean that there are not very many good cross-platform solutions to getting native scrollbars to work properly, which means many theme developers use custom scrollbars to work around this problem. For over a year now I keep playing around with hiding the statusbar because I don't have any extensions that live there, and I always end up turning it back on because resizing the window becomes frustrating without the resizer gripper. And I have fairly good eyesight... so there may be some accessibility issues to not having a resizer, at least for Windows 2000, XP and OSX users.

So what do you want me to do here? Alex?

>So what do you want me to do here? Alex? I'm not sure what state the patch is in, but from a UX perspective I'm fine with the solution proposed: On XP and 2000 (but not Vista and 7), if there is a vertical scroll bar but no horizontal scroll bar, still continue to display the window resizer under the vertical scroll bar (where it normally appears if you have both a vertical and horizontal scroll bar).

(In reply to comment #34) > On XP and 2000 (but not Vista and 7), if there is a vertical scroll bar but no > horizontal scroll bar, still continue to display the window resizer under the > vertical scroll bar (where it normally appears if you have both a vertical and > horizontal scroll bar). Actually, I now wonder if the resizer should always be displayed, regardless of whether scrollbars are visible. Consider the case where one is using the resizer to resize the window and the window becomes large enough to hide the scrollbar. The resizer will then disappear, despite that the user is still dragging it. Although resizing will continue, the resizing UI being used is not visible. Thoughts?

I agree. Also, a user may wish to shrink a window that doesn't have scrollbars, or even enlarge it (since it's possible to have overflow where the scrollbars are suppressed by the page).

In the state that there are no scroll bars, if we can overlay a partially transparent resizer similar to OS X, that would be ideal. We don't want to have a floating fully native square resizer just hanging out in the corner though.

We can copy the approach of bug 595180 for Windows.

(In reply to comment #37) > In the state that there are no scroll bars, if we can overlay a partially > transparent resizer similar to OS X, that would be ideal. We don't want to > have a floating fully native square resizer just hanging out in the corner > though. What does it look like on XP with the current patch? Does it have a square resizer? It can be changed to use the transparent resizer image if it does.

Here are some Windows builds of an updated patch: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/neil@mozilla.com-b833da8132e8/ Could someone post some images of what the resizer looks like (hide the statusbar first) on XP?

Various states of the resizer without and with the addons bar. A few questions: 1. Shouldn't the resizer have a background when the scrollbars are shown so that it replaces the scrollcorner element? 2. Shouldn't the resizer be hidden when the addons bar is shown if it's going to have it's own resizer?

>1. Shouldn't the resizer have a background when the scrollbars are shown so >that it replaces the scrollcorner element? It should appear native in that situation (even though there is no horizontal scroll bar). >2. Shouldn't the resizer be hidden when the addons bar is shown if it's going >to have it's own resizer? Yeah, I think so

(In reply to comment #41) > 1. Shouldn't the resizer have a background when the scrollbars are shown so > that it replaces the scrollcorner element? How does the screenshot you posted differ in that regard? I'm not sure what background you are referring to. What does, for instance, a textbox resizer look like (with/without scrollbars)? > 2. Shouldn't the resizer be hidden when the addons bar is shown if it's going > to have it's own resizer? I will fix this one. It only occurs when both scrollbars are visible (Image 5)

The scrollcorners appear to be a box with no border and no shading that has a background color of -moz-Dialog.

This patch creates a resizer and scrollcorner separately.

(In reply to comment #43) > > 2. Shouldn't the resizer be hidden when the addons bar is shown if it's going > > to have it's own resizer? > > I will fix this one. It only occurs when both scrollbars are visible (Image 5) I have only partially fixed this one. There scrollcorner background doesn't appear when only one scrollbar is visible. I think I'd rather fix this in a followup bug or patch.

+ nsIContent* content = mOuter->GetContent(); + if (content) { + nsIDocument* doc = content->GetCurrentDoc(); + nsPIDOMWindow* win = doc->GetWindow(); + if (win) { + nsCOMPtr<nsIContent> frameContent = do_QueryInterface(win->GetFrameElementInternal()); + if (frameContent && frameContent->NodeInfo()->Equals(nsGkAtoms::browser, kNameSpaceID_XUL)) { + mCollapsedResizer = !frameContent->HasAttr(kNameSpaceID_None, nsGkAtoms::showresizer); + } + } + } Factor out this duplicate code. Why do we need to have separate resizer and scrollcorner elements?

(In reply to comment #49) > Why do we need to have separate resizer and scrollcorner elements? If one turns on and off the statusbar, we switch between the resizer and scrollcorner. Also, drawing the resizer on top of the scrollcorner looks more correct in the platform/theme situations when the transparent resizer is used. When the native resizer is used, it covers the scrollcorner anyway.

Comment on attachment 487044 [details] [diff] [review] Resizer patch OK, r+ with the common code factored out

Comment on attachment 487044 [details] [diff] [review] Resizer patch >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css >-/* Remove the resizer from the statusbar compatibility shim */ >-#status-bar > .statusbar-resizerpanel { >- display: none; >-} This has the effect of re-introducing the addons-bar resizer for Windows Vista/7, which I don't think is desired. Also it's rather odd to depend on the resizer in a "compatibility shim" for desired functionality, so it would probably be better to add a separate resizer. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >+ <method name="updateWindowResizers"> >+ let ver = parseFloat(sysInfo.getProperty("version")); >+ if (ver >= 5.0 && ver < 6) { This only needs to check ver < 6, right? Perhaps there should be a _showWindowResizers helper to avoid duplicating the code (it can be #ifdefed to return false if not XP_WIN). >+ updateWindowResizers(); this.updateWindowResizers(); >diff --git a/toolkit/content/widgets/resizer.xml b/toolkit/content/widgets/resizer.xml > <binding id="resizer"> > <constructor> >+ if (this.parentNode == this.ownerDocument.documentElement) >+ return; I don't understand this change.

(In reply to comment #53) > >-/* Remove the resizer from the statusbar compatibility shim */ > >-#status-bar > .statusbar-resizerpanel { > >- display: none; > >-} > > This has the effect of re-introducing the addons-bar resizer for Windows > Vista/7, which I don't think is desired. Also it's rather odd to depend on the > resizer in a "compatibility shim" for desired functionality, so it would > probably be better to add a separate resizer. OK, so I'll need to change this to only happen on XP and 2000. > This only needs to check ver < 6, right? The request was to make this change Windows XP and 2000 only. > > <binding id="resizer"> > > > <constructor> > > >+ if (this.parentNode == this.ownerDocument.documentElement) > >+ return; > > I don't understand this change. My memory suggests that this was to prevent the direction handling from occuring for the resizer in an html document, as it causes a crash otherwise. Related to bug 563665 I believe.

(In reply to comment #54) > The request was to make this change Windows XP and 2000 only. We don't run on versions older than 2000, so the greater-than-5 check seems redundant.

(In reply to comment #27) > (In reply to comment #26) > The resizer isn't the issue; the issue is a large enough target for resizing > the window, IMO. -> Bug 619408 (I'm not sure this depends on 619408 or vv)

Comment on attachment 496535 [details] [diff] [review] Resizer on statusbar patch, address Gavin's comments >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ if (!shouldShowPageResizers()) >+ document.getElementById("status-bar").setAttribute("hideresizer"); missing an argument here >+function shouldShowPageResizers() XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {...}) would be more efficient, given that this can't change and gets called more than once. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >+ <method name="updateWindowResizers"> >+ <body><![CDATA[ >+ if (shouldShowPageResizers()) { prefer early return instead of indenting the whole function >diff --git a/toolkit/content/widgets/resizer.xml b/toolkit/content/widgets/resizer.xml > <binding id="resizer"> >+ if (this.parentNode == this.ownerDocument.documentElement) >+ return; (In reply to comment #54) > My memory suggests that this was to prevent the direction handling from > occuring for the resizer in an html document, as it causes a crash otherwise. > Related to bug 563665 I believe. This should get a comment (FIXME bug 563665?). r=me with those addressed.

Using "window.gShowPageResizers" for the checks in tabbrowser.xml will also weaken the dependency on browser.js, which might be needed for those accessibility tests.

The re-sizer is now shown when maximized which shouldn't be the case. BUILD: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20101222 Firefox/4.0b9pre ID:20101224193418 ~B

This looks potentially responsible for a Txul regression on Linux/Linux64: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/bb78db4eddfc2cde The other two changes seem less likely, so I'm going to try backing it out and see what happens.

Backed out: https://hg.mozilla.org/mozilla-central/rev/935cc79d8693

It also caused bug 621423.

It's a bit hard to tell from the graphs, but it does look like backing this out reverted the Txul hit, despite the regression script not noticing: http://grab.by/86ee http://grab.by/86ei It was small (1-2ms on a baseline of ~93.5ms). The frontend changes can't really have had an impact on Linux, so presumably the other changes are responsible (nsHTMLScrollFrame::Reflow?).

What about Windows 7 Classic Theme or Windows 7 Basic Theme? I noticed IE9 beta has resizers for both of the themes.

Likely because with this patch, two elements (scrollcorner and resizer) are used, rather than just the one. This didn't have any impact on other platforms or other tests?

The script didn't flag any regressions on other platforms, but Txul on those may not be reliable enough to detect a regression of this size.

STOP REMOVING FUNCTIONALITY FROM FIREFOX, PUT THE STATUS BAR BACK.

AND PLEASE STOP TRYING TO DUMB IT DOWN, WE'RE NOT ALL DUMB. HOW ABOUT YOU LET USERS WISE-UP INSTEAD.

(In reply to comment #71) > AND PLEASE STOP TRYING TO DUMB IT DOWN, WE'RE NOT ALL DUMB. > > HOW ABOUT YOU LET USERS WISE-UP INSTEAD. There is an easy way to get the status bar back. I think you'll get smarter if you find out how, by yourself. Please understand, though, that we don't want posts like those here. This bug is not relevant for that discussion, and this change has already been discussed. Finally, how about you locate the CAPS-LOCK button on your keyboard?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110102 Firefox/4.0b9pre ID:20110102030355 The resizer is there and working, but the 'gripper' image is missing. Is this the right place to note that, or should I file a separate bug for OS X only?

The missing image on Mac is bug 595180.

Performance testing suggests that adding resize: both to ua.css might be the cause of the slowdown, so this patch removes the change to that file on Linux. We'll see.

I’m probably commenting a bit late but, I’d like to make a small remark, currently the choice was to have no resizer at all in Windows 7 (If I’m not wrong here). But if we look system wide (especially the well known Windows File Manager or even things like Notepad, Paint, etc.) The status-bar has a resizer, and if the Status-bar is removed the resizer isn’t present, even when scrollbar are visible. Btw, about that in aero the window border is big enough, this is only for default, border size can be reduced, or other theme can be used that don’t have thick border (witch include like said before; classic theme). Therefore enabling resizer only when add-on bar is visible would be a good compromise and totally conform to windows 7 UI design. Thanks for reading.

Verified fixed with builds on Windows like Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110120 Firefox/4.0b10pre. We only show the resizer on Win2000 and Windows XP. Neil, I assume that we don't need manual tests because the automated tests cover all or nearly all areas? (In reply to comment #77) > Btw, about that in aero the window border is big enough, this is only for > default, border size can be reduced, or other theme can be used that don’t have > thick border (witch include like said before; classic theme). > Therefore enabling resizer only when add-on bar is visible would be a good > compromise and totally conform to windows 7 UI design. Thanks for reading. Can you please file a new bug, so we can get this sorted out? As comments on this bug explained we didn't wanted to show the resizer on Windows 7 but given that borders can be modified we should probably re-consider this option. Thanks.

Filed bug627051

The nsIMenuBoxObject interface is now documented, with the new attribute listed: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMenuBoxObject

sheppy, I think that you meant for that comment to be in bug 607224.

Why did dev-doc-needed get put back on this? The only doc issue involved is the one about the new attribute in nsIMenuBoxObject, which is covered. Yes, that's actually in bug 607224, but this was the bug Neil linked to on his blog when he mentioned the addition of that attribute, so it's where I noted the change had been made. Restoring doc-complete; switch back to doc-needed if there really is a new doc issue here.

Attn: bug 629560 and bug 629567.

Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre