Closed Bug 328156 Opened 14 years ago Closed 10 years ago

Window menu: blank line for open About box

Categories

(Firefox :: Menus, defect, trivial)

All
macOS
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: mozilla, Assigned: markus.magnuson)

Details

Attachments

(1 file, 5 obsolete files)

1 Launch app
2 About
3 Window menu

Notice: checked blank item

Expected: About box does not appear in Window menu 

Firefox 1.5.0.1, 10.4.5 8G1454, iMac Intel/1.83G
Confirming; also on trunk.
Status: UNCONFIRMED → NEW
Component: General → Menus
Ever confirmed: true
QA Contact: general → menus
Version: 1.5.0.x Branch → Trunk
Attached patch Patch (obsolete) — Splinter Review
This is caused by an error in 'browser/base/content/aboutDialog.xul' which results in the about dialog not being assigned a title attribute for Mac OS X builds, this is needed for display purposes.
Actually, no: no title and not appearing in the Window menu may be stupid, but it's exactly according to Apple HIG: http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_17_section_5.html#//apple_ref/doc/uid/20000961-TPXREF18
Then the actual problem is that the About Window shouldn't appear in the Window menu at all - no blank line even.
(also it shouldn't have a minimize window control).


Attachment #213218 - Attachment is obsolete: true
Assignee: nobody → markus.magnuson
Status: NEW → ASSIGNED
Attachment #417489 - Flags: ui-review?(faaborg)
Attachment #417489 - Flags: review?(mano)
Here's a patch to hide the About window's item in the Window menu on Mac, according to the Apple HIG as mentioned in comment 3. The previous HIG link is outdated though, here is the current one: http://developer.apple.com/mac/library/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGWindows/XHIGWindows.html#//apple_ref/doc/uid/20000961-TPXREF18
Hardware: PowerPC → All
Comment on attachment 417489 [details] [diff] [review]
Hide the About window's item in the Window menu on Mac.

A more generic solution is needed (like an attribute on the window element).  Other apps may not use that windowtype.
Comment on attachment 417489 [details] [diff] [review]
Hide the About window's item in the Window menu on Mac.

ui-r+ based on described functionality and it being backed up by an apple HIG
Attachment #417489 - Flags: ui-review?(faaborg) → ui-review+
Asaf, my first solution was to use the window name instead, i.e:

if (win.name == "About")

Would you consider this more generic, or should I go through with a solution based on window attribute instead?
Also, I added cc'd you on this one and 508757, I hope that's no problem.
Here's a new patch that uses a window attribute instead, as suggested in comment 7.
Attachment #417489 - Attachment is obsolete: true
Attachment #417947 - Flags: review?(mano)
Comment on attachment 417947 [details] [diff] [review]
Hide window from menu by using a new window attribute


 <dialog xmlns:html="http://www.w3.org/1999/xhtml"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
         id="aboutDialog"
         windowtype="Browser:About"
+#ifdef XP_MACOSX
+		inwindowmenu="false"
+#endif

1. Don't use hard tabs.
2. Move it to the #ifdef block below:
         onload="init(event);" onunload="uninit(event);"
 #ifdef XP_MACOSX
         buttons="extra2"
         align="end"
 #else

         title="&aboutDialog.title;"
         buttons="accept,extra2"
 #ifdef XP_UNIX


+GK_ATOM(inwindowmenu, "inwindowmenu")


This isn't necessary at all.

-function checkFocusedWindow()
+var aboutItemIsHidden = false;
+
+function macWindowMenuDidShow()
 {
+  var checkedWindow = false;
   var windowManagerDS =
     Components.classes['@mozilla.org/rdf/datasource;1?name=window-mediator']
               .getService(Components.interfaces.nsIWindowDataSource);
-
   var sep = document.getElementById("sep-window-list");
   // Using double parens to avoid warning
-  while ((sep = sep.nextSibling)) {
+  while ((sep = sep.nextSibling) && (!checkedWindow && !aboutItemIsHidden)) {

Multiple items could be hidden, you shouldn't stop at the "about" menu item.

-      break;
+      checkedWindow = true;
+    }
+    // Hide the About window's item in the Window menu, according to the Apple HIG:
+    // http://developer.apple.com/mac/library/documentation/UserExperience/
+    //  Conceptual/AppleHIGuidelines/XHIGWindows/XHIGWindows.html#//apple_ref/
+    //  doc/uid/20000961-TPXREF18

Any other menuitem could be hidden.  Thus there's no need to add this comment
+    if (win.document.documentElement.getAttribute("inwindowmenu") == "false") {
+      sep.setAttribute("hidden", "true");

Use the hidden property instead.

+      aboutItemIsHidden = true;

See my previous comment on that.
Attachment #417947 - Flags: review?(mano) → review-
Here's a new patch, addressing all review comments in comment 12.
Attachment #417947 - Attachment is obsolete: true
Attachment #418138 - Flags: review?(mano)
Comment on attachment 418138 [details] [diff] [review]
Updated patching according to review comments.


>     if (win == window) {
>       sep.setAttribute("checked", "true");
>-      break;
>+    }
>+    if (win.document.documentElement.getAttribute("inwindowmenu") == "false") {
>+      sep.hidden = true;

The if's order should be flipped, and the second one should be an |else if| (i.e. if (win.document.documentElement.getAttribute("inwindowmenu") == "false") ... else if (win == window) ).

r=mano otherwise.
Attachment #418138 - Flags: review?(mano) → review+
Attached patch Final patch (obsolete) — Splinter Review
This is the final patch, with the change mentioned by Asaf in comment 14. Who would I ask for superreview on this one?
Attachment #418138 - Attachment is obsolete: true
Eh. I forgot to ask you to remove braces around single line blocks...
Here's the same final patch but without braces on the single line blocks.
Attachment #418700 - Attachment is obsolete: true
Asaf, did you see that last patch with the fixed braces?
Attachment #418722 - Flags: review?(mano)
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/20a1b693824f.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
You need to log in before you can comment on or make changes to this bug.