Closed Bug 521927 Opened 15 years ago Closed 14 years ago

Make Search, Folder Location and Views widgets for MailNews customizable toolbars

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a2

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

Details

Attachments

(2 files, 11 obsolete files)

77.03 KB, patch
mnyromyr
: review+
philip.chee
: feedback+
Details | Diff | Splinter Review
255.59 KB, image/png
ehsan.akhgari
: feedback+
Details
At the moment Search, Folder Location and Views are not available as widgets for the MailNews Customizable toolbars.
Target Milestone: --- → seamonkey2.1a1
This patch:
* Removes Location Bar and Search Box Holder from xul.
* Removes View Menu items for Location Bar and Search Box.
* Removes view picker overlay xul.
* Adds toolbar items for Mail Views, Search and Folder Location.
* Hides mailview container when the search box is hidden.
* Some css tidy up.
Attachment #406060 - Flags: superreview?(neil)
Attachment #406060 - Flags: review?(mnyromyr)
(In reply to comment #1)
> Created an attachment (id=406060) [details]
> Add widgets for Mail Views, Search and Folder Location to MailNews patch v0.1
> 
> This patch:
> * Removes Location Bar and Search Box Holder from xul.
> * Removes View Menu items for Location Bar and Search Box.
> * Removes view picker overlay xul.
> * Adds toolbar items for Mail Views, Search and Folder Location.
> * Hides mailview container when the search box is hidden.
> * Some css tidy up.

Ignore the following addition:
+<!ENTITY viewToolbar.tooltip "View Toolbar">

fixed locally.
Along with the removal of the references to toolbar-gradient34.png in mac/messenger/mailWindow1.css the image itself can be removed.
(In reply to comment #3)
> Along with the removal of the references to toolbar-gradient34.png in
> mac/messenger/mailWindow1.css the image itself can be removed.
Fixed locally.
> +++ b/suite/mailnews/jar.mn
> +% style chrome://global/content/customizeToolbar.xul chrome://messenger/content/messenger.css
> +% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/

You copied these from Thunderbird (he said accusingly). Why do we need these lines in suite?
Nothing in chrome://messenger/content/messenger.css is needed as far as I can tell.
In any case chrome://messenger/skin/messenger.css imports chrome://messenger/content/messenger.css and nothing in skin/messenger.css is needed either.

> +
> +  // make sure the mail views search box is initialized
> +  if (document.getElementById("mailviews-container"))
> +    ViewPickerOnLoad();
> +
> +  // make sure the folder location picker is initialized
> +  if (document.getElementById("folder-location-container")) {
> +    OnLoadLocationTree();
> +    FolderPaneSelectionChange();
> +  }
Unlike Firefox/Thunderbird, our toolbar items are not removed from the DOM so I don't think that they don't need to be reinitialized. And since they are never removed from the DOM, the null check is redundant.

+<script type="application/javascript"
+        src="chrome://messenger/content/msgViewPickerOverlay.js"/>

I know that this is the new style guideline but it looks ugly in the middle of the other lines. Ideally Serge should have reformatted these in his x-javascript->javascript bug.

> -           defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-goback,button-goforward,button-next,button-junk,button-delete,button-mark,spring,throbber-box"
> +           defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-next,button-delete,button-mark,spring,searchBox,throbber-box"

Some buttons were removed from the defaultset. I presume this is to make room for the search box?

> +    <toolbaritem id="folder-location-container" insert-after="button-stop"

You copied this from Thunderbird (again!). First of all the attribute is "insertafter". Secondly this doesn't do anything here even without the typo.

>  function OnLoadLocationTree()
>  {
> -    var locationTree = document.getElementById('locationPopup').tree;
> +  var locationTree = document.getElementById('locationPopup').tree;
> +  if (locationTree) {

Unnecessary null check. Also if you are touching these lines change the single quotes to double quotes.

> -window.addEventListener("load", ViewPickerOnLoad, false);
Why?

> -/* XXX bug 313731 buttons are taller than menulists */
> -#msgLocationToolbar > #searchBox[collapsed="true"] {
> -  visibility: hidden;

Hmm. Don't we still need something like this for the searchBox?
(In reply to comment #5)
> > +++ b/suite/mailnews/jar.mn
> > +% style chrome://global/content/customizeToolbar.xul chrome://messenger/content/messenger.css
> > +% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/
> 
> You copied these from Thunderbird (he said accusingly). Why do we need these
> lines in suite?
Fixed.
> > +  // make sure the folder location picker is initialized
> > +  if (document.getElementById("folder-location-container")) {
> > +    OnLoadLocationTree();
> > +    FolderPaneSelectionChange();
> > +  }
> Unlike Firefox/Thunderbird, our toolbar items are not removed from the DOM so I
> don't think that they don't need to be reinitialized. And since they are never
> removed from the DOM, the null check is redundant.
Seems to need the above to work.
> > -           defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-goback,button-goforward,button-next,button-junk,button-delete,button-mark,spring,throbber-box"
> > +           defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-next,button-delete,button-mark,spring,searchBox,throbber-box"
> 
> Some buttons were removed from the defaultset. I presume this is to make room
> for the search box?
Yes.
> 
> > +    <toolbaritem id="folder-location-container" insert-after="button-stop"
> 
> You copied this from Thunderbird (again!). First of all the attribute is
> "insertafter". Secondly this doesn't do anything here even without the typo.
Fixed.
> 
> >  function OnLoadLocationTree()
> >  {
> > -    var locationTree = document.getElementById('locationPopup').tree;
> > +  var locationTree = document.getElementById('locationPopup').tree;
> > +  if (locationTree) {
> 
> Unnecessary null check. Also if you are touching these lines change the single
> quotes to double quotes.
Again, needed due to the above.
> 
> > -window.addEventListener("load", ViewPickerOnLoad, false);
> Why?
Put back and removed the ViewPickerOnLoad from MailToolboxCustomizeDone, I suppose it could be put in the InitPanes() function though.
> 
> > -/* XXX bug 313731 buttons are taller than menulists */
> > -#msgLocationToolbar > #searchBox[collapsed="true"] {
> > -  visibility: hidden;
> 
> Hmm. Don't we still need something like this for the searchBox?
Don't appear to, but still checking.
>> Unlike Firefox/Thunderbird, our toolbar items are not removed from the DOM so I
>> don't think that they don't need to be reinitialized. And since they are never
>> removed from the DOM, the null check is redundant.
> Seems to need the above to work.

Interesting. I suppose composite XBL widgets are affected negatively by being moved around the DOM.
Comment on attachment 406060 [details] [diff] [review]
Add widgets for Mail Views, Search and Folder Location to MailNews patch v0.1

>+% style chrome://global/content/customizeToolbar.xul chrome://messenger/content/messenger.css
>+% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/
We've already got these in the themes.

>+  // make sure the mail views search box is initialized
>+  if (document.getElementById("mailviews-container"))
>+    ViewPickerOnLoad();
>+
>+  // make sure the folder location picker is initialized
>+  if (document.getElementById("folder-location-container")) {
>+    OnLoadLocationTree();
>+    FolderPaneSelectionChange();
>+  }
What goes wrong if we don't do these?

>+           defaultset="button-getmsg,button-newmsg,separator,button-reply,button-replyall,button-forward,separator,button-next,button-delete,button-mark,spring,searchBox,throbber-box"
The search box looks out of place on the primary toolbar.

>+      <button id="advancedButton"
>+              label="&advancedButton.label;"
>+              tooltiptext="&advancedButton.tooltip;"
>+              accesskey="&advancedButton.accesskey;"
>+              oncommand="onAdvancedSearch();"/>
Might it be worth separating the advanced button too?

>+  if (locationTree) {
Don't need to check this.

>   gSearchBox.collapsed = true;
>+  gViewsContainer.collapsed = true;
Customising the toolbar is hard when account central is showing...

>+#mailviews-container > menulist > menupopup > menu > .menu-text {
>+  -moz-margin-start: 0px !important;
>+}
>+
>+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menu {
>+  -moz-padding-end: 0px !important;
>+}
Part of another bug?
Attachment #406060 - Flags: superreview?(neil) → superreview-
Comment on attachment 406060 [details] [diff] [review]
Add widgets for Mail Views, Search and Folder Location to MailNews patch v0.1

This is pretty broken...
Both the Search Widget and the Mail View Widget are collapsed in a new profile or when you had the location/search bar hidden before.
I suspect there's some initialization code not/misfiring.

Even if the profile had a visible Mail View widget before your patch, dragging it into the toolbar and using custom filters doesn't work:
JavaScript strict warning: chrome://messenger/content/searchBar.js, line 194: reference to undefined property gSearchInput.value
JavaScript strict warning: chrome://messenger/content/searchBar.js, line 379: reference to undefined property gSearchInput.value
JavaScript error: chrome://messenger/content/searchBar.js, line 379: gSearchInput.value is undefined
Attachment #406060 - Flags: review?(mnyromyr) → review-
(In reply to comment #9)
> (From update of attachment 406060 [details] [diff] [review])
> This is pretty broken...
> Both the Search Widget and the Mail View Widget are collapsed in a new profile
> or when you had the location/search bar hidden before.
> I suspect there's some initialization code not/misfiring.
> 
> Even if the profile had a visible Mail View widget before your patch, dragging
> it into the toolbar and using custom filters doesn't work:
> JavaScript strict warning: chrome://messenger/content/searchBar.js, line 194:
> reference to undefined property gSearchInput.value
> JavaScript strict warning: chrome://messenger/content/searchBar.js, line 379:
> reference to undefined property gSearchInput.value
> JavaScript error: chrome://messenger/content/searchBar.js, line 379:
> gSearchInput.value is undefined

Can you give exact steps for reproducing as I have tried most ways and cannot repeat the problem.
Changes since v0.1:
* Made Advanced button separate widget from search one.
* Added separate toolbar to put mailviews, search and advanced widgets on.
* Set new widgets to disable rather than collapse/hide when not relevant.
* Fixed toolbar binding so that custom toolbars have added toolbar-primary class.
* Fixed various theme issues for when customize toolbar dialog is showing.
Attachment #406060 - Attachment is obsolete: true
Attachment #407656 - Flags: superreview?(neil)
Attachment #407656 - Flags: review?(mnyromyr)
> +      <method name="appendCustomToolbar">
You missed "labelalign".

> +            toolbar.setAttribute("class", "toolbar-primary chromeclass-toolbar");

> * Fixed toolbar binding so that custom toolbars have added toolbar-primary
> class.
What problem are you trying to solve here?

> +<!ENTITY showMessengerToolbarCmd.label "Mail Toolbar">
> +<!ENTITY showMessengerToolbarCmd.accesskey "M">
I notice that Thunderbird uses "o".

> +  if (document.getElementById("folder-location-container")) {
> +    OnLoadLocationTree();
What happens if you remove the if check?
And you already do a null check in OnLoadLocationTree() so some redundancy here.

> diff --git a/suite/mailnews/mailWindowOverlay.xul b/suite/mailnews/mailWindowOverlay.xul
> +  <toolbar class="chromeclass-toolbar"

I disagree with this toolbar appearing in the standalone message window.

> +    <toolbaritem id="advanced-container"
> +                 title="&advancedToolbarItem.title;"
> +                 align="center">
> +      <button id="advancedButton"
> +              label="&advancedButton.label;"
> +              tooltiptext="&advancedButton.tooltip;"
> +              accesskey="&advancedButton.accesskey;"
> +              oncommand="onAdvancedSearch();"/>
> +    </toolbaritem>
Ideally this should be a normal toolbar button complete with large/small icons in both classic and modern.

> +  var locationTree = document.getElementById('locationPopup').tree;
> +  if (locationTree) {
What happens if you remove the if check?

>    gSearchBox = document.getElementById("searchBox");
> -  gSearchBox.collapsed = true;
> +  gSearchBox.setAttribute("disabled", true);
> +  gViewsContainer = document.getElementById("mailviews-container");
> +  gViewsContainer.setAttribute("disabled", true);
You don't null check these.

>  function ShowingThreadPane()
>  {
> -  gSearchBox.collapsed = false;
> +  gSearchBox.removeAttribute("disabled");
> +  gViewsContainer.removeAttribute("disabled");
Or here.

> -  gSearchBox.collapsed = true;
> +  gSearchBox.setAttribute("disabled", true);
> +  gViewsContainer.setAttribute("disabled", true);
Or here. A wee bit of inconsistency don't you think?

>  function OnLoadLocationTree()
>  {
> -    var locationTree = document.getElementById('locationPopup').tree;
> +  var locationTree = document.getElementById('locationPopup').tree;
> +  if (locationTree) {
What happens if you remove the if check?

> +toolbarpaletteitem > #button-delete > deck > toolbarbutton {
> +  -moz-image-region: rect(0 29px 29px 0) !important;
The -moz-image-region should be on the #button-delete. If something is applying a -moz-image-region to the toolbarbutton then that's a bug and should be fixed there.

> +  color: black !important;
Is the !important needed? The priority rules for CSS selectors are a mystery to me. (several instances)

> +toolbarpaletteitem > #button-junk > deck > toolbarbutton {
ditto.

What about classic/mac/messenger/primaryToolbar.css?

> diff --git a/suite/themes/modern/messenger/primaryToolbar.css b/suite/themes/modern/messenger/primaryToolbar.css
#button-junk? #button-delete? #mailviews-container?
Comment on attachment 407656 [details] [diff] [review]
Add widgets for Mail Views, Search, Advanced and Folder Location to MailNews patch v0.1a

Purely from a visual point of view, as a stopgap, it would be helpful if the background that Modern uses for the bookmarks toolbar could be used for the search bar, but ideally, what we would do is to create a complete gradient for the entire toolbox rather than per-toolbar, which would then automatically work with custom toolbars.

>+            toolbar.setAttribute("class", "toolbar-primary chromeclass-toolbar");
But this is very wrong. Did you not try it in Modern? ;-)

>+  // make sure the folder location picker is initialized
>+  if (document.getElementById("folder-location-container")) {
>+    OnLoadLocationTree();
>+    FolderPaneSelectionChange();
Still not sure why this is here.

>+      <menulist id="locationFolders"
>+                width="170"
170 pixels is always going to be wrong; you need to move the width to somewhere where you can override it to about half as wide when the menulist is in the customisation palette, so maybe primaryToolbar.css or a new file.


>+    <toolbaritem id="advanced-container"
>+                 title="&advancedToolbarItem.title;"
>+                 align="center">
>+      <button id="advancedButton"
>+              label="&advancedButton.label;"
>+              tooltiptext="&advancedButton.tooltip;"
>+              accesskey="&advancedButton.accesskey;"
>+              oncommand="onAdvancedSearch();"/>
>+    </toolbaritem>
Could make this a toolbarbutton perhaps, but you would need to either force the text to appear or provide icons for it.

>-    <toolbarset id="customToolbars" context="toolbar-context-menu"/>
Why are you changing the standalone message window?

> toolbarpaletteitem > #button-delete {
>   display: -moz-box;
> }
> 
>+toolbarpaletteitem > #button-delete > deck > toolbarbutton {
>+  -moz-image-region: rect(0 29px 29px 0) !important;
>+  color: black !important;
Won't these styles inherit, in which case they can be merged with the above? (But see following comment.)

>+toolbarpaletteitem > #mailviews-container > label,
>+toolbarpaletteitem > #mailviews-container > menulist {
It would be helpful if you could give these items ids.
It's unfortunate that the customise toolbar code uses the disabled property; I'm not sure which would be better, switching it to the attribute or creating a toolbaritem binding with the property, but either way would help.

>+#mailviews-container > menulist > menupopup > menu > .menu-text {
>+  -moz-margin-start: 0px !important;
>+}
>+
>+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menu {
>+  -moz-padding-end: 0px !important;
Still don't know what this is for.
(In reply to comment #13)
> (From update of attachment 407656 [details] [diff] [review])
> Purely from a visual point of view, as a stopgap, it would be helpful if the
> background that Modern uses for the bookmarks toolbar could be used for the
> search bar, but ideally, what we would do is to create a complete gradient for
> the entire toolbox rather than per-toolbar, which would then automatically work
> with custom toolbars.
> 
> >+            toolbar.setAttribute("class", "toolbar-primary chromeclass-toolbar");
> But this is very wrong. Did you not try it in Modern? ;-)
Yes, and toolbar-primary has the only background that the buttons (File, Junk, Stop) look okay against. The background used by the bookmarks toolbar looks good for search, mailviews and folder widgets.
> 
> >+  // make sure the folder location picker is initialized
> >+  if (document.getElementById("folder-location-container")) {
> >+    OnLoadLocationTree();
> >+    FolderPaneSelectionChange();
> Still not sure why this is here.
The folder location widget doesn't have a drop down without it.
> 
> >+    <toolbaritem id="advanced-container"
> >+                 title="&advancedToolbarItem.title;"
> >+                 align="center">
> >+      <button id="advancedButton"
> >+              label="&advancedButton.label;"
> >+              tooltiptext="&advancedButton.tooltip;"
> >+              accesskey="&advancedButton.accesskey;"
> >+              oncommand="onAdvancedSearch();"/>
> >+    </toolbaritem>
> Could make this a toolbarbutton perhaps, but you would need to either force the
> text to appear or provide icons for it.
Tried, just end up with "Advanced..." and no border/raised button look.
> 
> >-    <toolbarset id="customToolbars" context="toolbar-context-menu"/>
> Why are you changing the standalone message window?
Moved into mailWindowOverlay.xul, without that custom toobars appear between the menu bar and the main mail toolbar.

> >+#mailviews-container > menulist > menupopup > menu > .menu-text {
> >+  -moz-margin-start: 0px !important;
> >+}
> >+
> >+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
> >+#mailviews-container > menulist > menupopup > menuitem,
> >+#mailviews-container > menulist > menupopup > menu {
> >+  -moz-padding-end: 0px !important;
> Still don't know what this is for.
In classic the mailviews have inconsistent leading spaces in the menus (so Tags/Custom have space, the rest don't) and they have trailing spaces where they don't in the View > Messages menu.
> > >+            toolbar.setAttribute("class", "toolbar-primary chromeclass-toolbar");
> > But this is very wrong. Did you not try it in Modern? ;-)
> Yes, and toolbar-primary has the only background that the buttons (File, Junk,
> Stop) look okay against. The background used by the bookmarks toolbar looks
> good for search, mailviews and folder widgets.

"toolbar-primary" is not just styling but also behavioural which isn't wanted for custom toolbars:

http://mxr.mozilla.org/comm-central/search?string=toolbar-primary&find=utilityOverlay.js&findi=%5C.js&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central

Also I don't think the primary toolbar icon should appear for custom toolbars:
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/communicator/toolbar/toolbarBindings.xml#57
Depends on: 525373
Attachment #407656 - Flags: superreview?(neil)
Attachment #407656 - Flags: review?(mnyromyr)
Attached patch toolbar-secondary patch v0.1b (obsolete) — Splinter Review
Changes since v0.1a:
* Added labelalign to appendCustomToolbar method.
* Advanced is now a toolbarbutton.
* More null checks.
* Toolkit changes spun off to bug 525373.
* Added changes for classic/mac/messenger/primaryToolbar.css
* Moved locationFolders width setting to css.
* Created a toolbaritem binding for the disabled property.
Attachment #407656 - Attachment is obsolete: true
Attachment #412510 - Flags: superreview?(neil)
Attachment #412510 - Flags: review?(mnyromyr)
Attachment #412510 - Flags: superreview?(neil)
Attachment #412510 - Flags: review?(mnyromyr)
Attached patch advanced toolbaritem patch v0.1c (obsolete) — Splinter Review
Changes since v0.1b:
* Make Advanced button a button within toolbaritem otherwise it gets vertically stretched on wide toolbars.
Attachment #412510 - Attachment is obsolete: true
Attachment #413447 - Flags: superreview?(neil)
Attachment #413447 - Flags: review?(mnyromyr)
Comment on attachment 413447 [details] [diff] [review]
advanced toolbaritem patch v0.1c

+#mailviews-container > menulist > menupopup > menu > .menu-text {
+  -moz-margin-start: 0px !important;
+}

Please don't do this in mac classic. This will look completely wrong, because there is like 20px space at the left in mac menuitems (and it should be the same in menus)

+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
+#mailviews-container > menulist > menupopup > menuitem,
+#mailviews-container > menulist > menupopup > menu {
+  -moz-padding-end: 0px !important;
+}

I can only see a reason for the menu to not have any left padding. And I'm a bit sceptical to that as well since this will make the look of this menulist unique.
(In reply to comment #18)

> I can only see a reason for the menu to not have any left padding.

Erm, right padding.
Note that on mac classic this will probably require:

1) Restyling of the mac classic mail/browser tabs since you make the search toolbar much darker (they don't fit since they're too light and we might also consider making them north-faced) than it is now.
2) That the PT/link toolbar in the browser should probably look the same as your new search toolbar.
Re comment #20 - it might be better to keep the current look of the search toolbar.
Attachment #413447 - Flags: superreview?(neil)
Attachment #413447 - Flags: review?(mnyromyr)
Changes since v0.1c:
* Fixes issue with restoring defaults and the advanced button.
* Addresses stefanh's concerns with OSX and classic theme.
Attachment #413447 - Attachment is obsolete: true
Attachment #413956 - Flags: superreview?(neil)
Attachment #413956 - Flags: review?(mnyromyr)
Should the search bar be treated the same as the PT when it comes to dropping large toolbar icons on it? I mean, it is a search bar and one could argue that it differs from the main toolbar.
Comment on attachment 413956 [details] [diff] [review]
Better looking on OSX patch v0.1d

>diff --git a/suite/common/bindings/toolbar.xml b/suite/common/bindings/toolbar.xml

I'm no expert for toolbar.xml code, it just "looks okay" to my inexperienced eye. ;-)

>diff --git a/suite/common/bindings/toolbaritem.xml b/suite/common/bindings/toolbaritem.xml
>+   - The Original Code is SeaMonkey prefwindow code.

I somehow doubt this. ;-)

>+        <setter>
>+        <![CDATA[
>+          if (val)

I'd prefer one additional level of indentation for CDATA and content here, just like almost everywhere else.

>diff --git a/suite/mailnews/commandglue.js b/suite/mailnews/commandglue.js
>     if (gAccountCentralLoaded)
>       UpdateMailToolbar("gAccountCentralLoaded");
>     else
>-      document.getElementById('advancedButton').setAttribute("disabled" , !(IsCanSearchMessagesEnabled()));
>+      document.getElementById("button-advanced").setAttribute("disabled", !(IsCanSearchMessagesEnabled()));

No braces needed for this ! expression.

>diff --git a/suite/mailnews/mailWindowOverlay.xul b/suite/mailnews/mailWindowOverlay.xul
> <script type="application/javascript" src="chrome://messenger/content/mailWindowOverlay.js"/>
>+<script type="application/javascript"
>+        src="chrome://messenger/content/msgViewPickerOverlay.js"/>
> <script type="application/javascript" src="chrome://messenger-newsblog/content/newsblogOverlay.js"/>

Might as well keep the pattern of the surrounding lines.

>         <toolbarbutton id="button-isJunk"
...
>-                       observes="button_junk"/>
>+                       observes="button-junk"/>
>         <toolbarbutton id="button-notJunk"
...
>-                       observes="button_junk"/>
>+                       observes="button-junk"/>
...
>         <toolbarbutton id="button-mark-deleted"
...
>-                       observes="button_delete"
>+                       observes="button-delete"
>                        oncommand="goDoCommand(event.shiftKey ? 'button_shiftDelete' : 'button_delete')"/>
>         <toolbarbutton id="button-mark-undelete"
...
>-                       observes="button_delete"
>+                       observes="button-delete"

This is wrong. The toolbarbuttons should observe the command broadcaster (button_junk, button_delete), not the buttons (button-junk, button-delete). I do concede that the naming is extraordinary inelegant here. :-(

>diff --git a/suite/mailnews/msgMail3PaneWindow.js b/suite/mailnews/msgMail3PaneWindow.js

> function OnLoadLocationTree()
> {
>+  var locationTree = document.getElementById('locationPopup').tree;
>+  if (locationTree) {

Brace on its own line, please.

>diff --git a/suite/mailnews/msgViewPickerOverlay.xul b/suite/mailnews/msgViewPickerOverlay.xul
>deleted file mode 100644

Yeehaa! :-D

>diff --git a/suite/themes/classic/mac/messenger/primaryToolbar.css b/suite/themes/classic/mac/messenger/primaryToolbar.css
>+#locationFolders {
>+  width: 170px;
>+}

This is way too small, most of my account/folder/group names are cropped. I'd propose 220px or maybe even 250px?
Attachment #413956 - Flags: review?(mnyromyr) → review-
Comment on attachment 413956 [details] [diff] [review]
Better looking on OSX patch v0.1d

Re mac, don't forget to add back 'nowindowdrag="true"' ;-)
Attachment #413956 - Flags: superreview?(neil)
Changes since v0.1d:
* Revised "Original Code" comment in toolbaritem.xml and fixed indentation.
* Removed unneeded braces in commandglue.js change.
* Kept with surrounding pattern for change at start of mailWindowOverlay.xul
* Defaults searchToolbar to small icons with text to the side so it is more likely to keep the same height when customised.
* Put nowindowdrag="true" onto searchToolbar for OS X.
* Put brace on its own line in msgMail3PaneWindow.js change.
* Used em instead of px for locationFolders' widths so should be better across all platforms.

I've left changes to junk and delete buttons as they are. The changes are there to make those buttons correctly become enabled when being customised (when on the toolbar or in the customizeToolbar dialog). This is because toolkit only removes the observes attribute from the first layer and does not dig deeper (see bug 525373 for some of the gory details). The way round this is to make the elements at a deeper layer observe the element which will get "cleaned up" by the toolkit code.
Attachment #413956 - Attachment is obsolete: true
Attachment #419740 - Flags: superreview?(neil)
Attachment #419740 - Flags: review?(mnyromyr)
Comment on attachment 419740 [details] [diff] [review]
Adjusted location folder width patch v0.1e

(In reply to comment #26)
> I've left changes to junk and delete buttons as they are. The changes are there
> to make those buttons correctly become enabled when being customised (when on
> the toolbar or in the customizeToolbar dialog). This is because toolkit only
> removes the observes attribute from the first layer and does not dig deeper
> (see bug 525373 for some of the gory details). The way round this is to make
> the elements at a deeper layer observe the element which will get "cleaned up"
> by the toolkit code.

Bah. :-(
Not much to do agains it, then, for the time being...

>+++ b/suite/themes/classic/messenger/mailWindow1.css
> #locationFolders {
>   width: 20em;
> }
...
>+++ b/suite/themes/classic/messenger/primaryToolbar.css
>+#locationFolders {
>+  width: 22em;
>+}

Using DOMI to look at the CSS for the Location Bar, it seems that the mailWindow1.css rule is always overruled by the toolbar rule (even on Mac and in Modern), so you may just drop the former?

r=me with that. :)
Attachment #419740 - Flags: review?(mnyromyr) → review+
Attachment #419740 - Flags: superreview?(neil)
Changes since v0.1e:
* Removed unneeded css rules for locationFolders in the mailWindow1.css files.

Carrying forward r= and requesting sr=
Attachment #419740 - Attachment is obsolete: true
Attachment #420834 - Flags: superreview?(neil)
Attachment #420834 - Flags: review+
Comment on attachment 420834 [details] [diff] [review]
Less locationFolders css patch v0.1f

>+            toolbar.setAttribute("class", "toolbar-secondary chromeclass-toolbar");
So, fiddling around with "secondary" toolbars because the Messenger icons don't have the correct transparency is not a good idea, and it breaks Navigator.

>+++ b/suite/common/bindings/toolbaritem.xml
Do we still need this, given the changes to disabled attribute handling?
Or was this actually to fix a different bug?

> function MailToolboxCustomizeDone(aToolboxChanged)
> {
>   toolboxCustomizeDone("mail-menubar", getMailToolbox(), aToolboxChanged);
>   SetupMoveCopyMenus('button-file', accountManagerDataSource, folderDataSource);
>+
>+  // make sure the folder location picker is initialized
>+  OnLoadLocationTree();
I just realised that this won't work in the standalone message window, since it doesn't have a location tree function, so moving the test there is no use.

>+  FolderPaneSelectionChange();
I don't think this has any effect, does it?

> function Create3PaneGlobals()
> {
>   // Update <mailWindow.js> global variables.
>   accountCentralBox = document.getElementById("accountCentralBox");
>-  gSearchBox = document.getElementById("searchBox");
>-  gSearchBox.collapsed = true;
>+  gSearchContainer = document.getElementById("search-container");
>+  if (gSearchContainer)
>+    gSearchContainer.setAttribute("disabled", true);
>+  gViewsContainer = document.getElementById("mailviews-container");
>+  if (gViewsContainer)
>+    gViewsContainer.setAttribute("disabled", true);
Won't these elements always exist? Also, why not set them to disabled in the XUL? Maybe even use a broadcaster.

>-#locationFolders {
>-  width: 20em;
>-}

>+#locationFolders {
>+  width: 22em;
>+}
10% extra free :-)

>+#mailviews-container > menulist > menupopup > menu > .menu-text {
>+  -moz-margin-start: 0px !important;
>+}
>+
>+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menu {
>+  -moz-padding-end: 0px !important;
>+}
[I still need to look into this.]
Comment on attachment 420834 [details] [diff] [review]
Less locationFolders css patch v0.1f

>+.paletteRow > toolbarpaletteitem > #folder-location-container > #locationFolders {
Use toolbarpaletteitem[place="palette"] instead of .paletteRow >
Comment on attachment 420834 [details] [diff] [review]
Less locationFolders css patch v0.1f

>+#mailviews-container > menulist > menupopup > menu > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menuitem,
>+#mailviews-container > menulist > menupopup > menu {
>+  -moz-padding-end: 0px !important;
>+}
This is definitely wrong; menulists menuitem already have the correct padding, while the right thing to do for menulist menus is to remove the padding override in menu.css in the first place. (In fact the padding override for both menus and menulists in .menulist-menupopup submenus is wrong too, but unfortunately we accidentally stopped using them so it's not so obvious...)
Attachment #420834 - Flags: superreview?(neil) → superreview-
Changes since v0.1f:
* Check for existence of OnLoadLocationTree before trying to call it;
* Removed unneeded call to FolderPaneSelectionChange;
* Switched to using broadcaster to disable Views and Search containers;
* Switched to using toolbarpaletteitem[place="palette"] instead of .paletteRow >;
* Moved 3-pane specific customisable widgets into folderPane.xul and related xul into messenger.xul and messageWindow.xul;
* Removed -moz-padding-end: 0px !important css changes;

Note:
* toolbaritem.xml is still needed for button decks which are disabled - it is the way toolkit does disabling (via property instead of attribute and I think we can only observe on attributes not properties);
* As discussed on IRC, I think, "secondary" toolbars do not seem to break navigator in modern.
Attachment #420834 - Attachment is obsolete: true
Attachment #431633 - Flags: superreview?(neil)
+<!ENTITY searchSubjectOrAddress.emptytext "Search Subject or Address">
+        <textbox id="searchInput"
+                 flex="1"
+                 type="search"
+                 emptytext="&searchSubjectOrAddress.emptytext;"
I think we should start using "placeholder" in place of "emptytext"

+  <toolbox id="mail-toolbox">
+    <toolbarpalette id="MailToolbarPalette">
I don't think that the <toolbox> is necessary. Could you try without it?
Attachment #431633 - Flags: superreview?(neil)
Changes since v0.1g:
* Used placeholder instead of emptytext;
* Removed unneed toolbox elements from folderPane.xul.
Attachment #431633 - Attachment is obsolete: true
Attachment #431724 - Flags: superreview?(neil)
Attachment #431724 - Flags: feedback?(stefanh)
Comment on attachment 431724 [details] [diff] [review]
with placeholder and no toolbox patch v0.1h

(I'll look at this in detail later on this week)

+               placeholder="&searchSubjectOrAddress.emptytext;"
 Instead of .emptytext, you might want to use .placeholder and be consistent with what I did in bug 550186 (see the comments in the mentioned bug about the possible impact for localizers)
Attachment #431724 - Flags: superreview?(neil)
Attachment #431724 - Flags: feedback?(stefanh)
Changes since v0.1h:
* Entity has correct name.
Attachment #431724 - Attachment is obsolete: true
Attachment #443324 - Flags: feedback?(stefanh)
Attachment #443324 - Flags: superreview?(bugzilla)
Attachment #443324 - Flags: superreview?(bugzilla)
Attachment #443324 - Flags: superreview-
Attachment #443324 - Flags: feedback?(philip.chee)
Attachment #443324 - Flags: superreview- → feedback?(kairo)
Comment on attachment 443324 [details] [diff] [review]
Revised patch with placeholder v0.1i

+  <binding id="toolbaritem" extends="xul:box">

+/* ::::: toolbaritem ::::: */
+toolbaritem {
+  -moz-binding: url("chrome://communicator/content/bindings/toolbaritem.xml#toolbaritem");
+}

If we don't need extends="xul:box" could we just do:
+  -moz-binding: url("chrome://global/content/bindings/general.xml#basecontrol");

diff --git a/suite/mailnews/folderPane.xul b/suite/mailnews/folderPane.xul

+    <toolbaritem id="folder-location-container"

Possibly file a followup bug to port the relevant tests from:
/mail/test/mozmill/folder-display/test-folder-toolbar.js etc.
(is mozmill working for SeaMonkey?)

+        <menupopup id="locationPopup"
+                   height="400"
+                   oncommand="OnLocationTreeSelect(this);"/>

Thunderbird uses "folderLocationPopup"

+    <toolbaritem id="mailviews-container"

Do we need something similar to TB in MailToolboxCustomizeDone?

  // make sure the mail views search box is initialized
  if (document.getElementById("mailviews-container"))
    ViewPickerOnLoad();

Hmm. I guess not.

+      <menulist id="viewPicker"
+                oncommand="ViewChangeByMenuitem(event.target);">
+        <menupopup id="viewPickerPopup"
+                   onpopupshowing="RefreshViewPopup(this, true);">
+          <menuitem id="viewPickerAll"
+                    label="&viewAll.label;"
+                    value="0"/>

Thunderbird adds |type="radio"| should we also use this?

+      <textbox id="searchInput"

No placeholder text?

diff --git a/suite/mailnews/messageWindow.xul b/suite/mailnews/messageWindow.xul
+    <toolbar id="searchToolbar"
+             class="chromeclass-toolbar"
diff --git a/suite/mailnews/messenger.xul b/suite/mailnews/messenger.xul
+    <toolbar id="searchToolbar"
+             class="chromeclass-toolbar"

Could we move <toolbar id="searchToolbar"> to mailWindowOverlay.xul and just leave stub overlay points in messenger.xul and messageWindow.xul?

+             observes="mailHideMenus"

On the other hand in the standalone message window, the searchToolbar is unconditionally hidden and can't be unhidden so I fail to see why you have it in this window.

+.toolbar-secondary > .toolbar-box > .toolbar-holder,
 .toolbar-primary-holder {
   background: url("chrome://communicator/skin/toolbar/prtb-bg-line.gif") repeat-x top;

If there isn't already theme a bug, we should file one to replace background urls with -moz-linear-gradient[s].

The [Advanced...] button if dragged to the main toolbar looks decidedly out of place.
1. No image.
2. Totally different button look.
3. Just says Advanced... Advanced what?

I tried:
1. Changing the button back to a toolbarbutton.
2. Getting rid of the toolbaritem parent.
3. Adding class="toolbarbutton-1" to the button to fix the stretch issues.
4. Adding a suitable image.

Not sure if this makes it better or worse.

On start up I get this (even in safe mode):

Error: redeclaration of const kViewItemAll
Source file: chrome://messenger/content/msgViewPickerOverlay.js
Line: 43

Moving the location picker in/out of the palette causes this:

Error: locationTree is undefined
Source file: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1077
Attachment #443324 - Flags: feedback?(philip.chee) → feedback-
> No placeholder text?
Ah, clearing the search text doesn't restore the placeholder text for some reason.
(In reply to comment #38)
> > No placeholder text?
> Ah, clearing the search text doesn't restore the placeholder text for some
> reason.

It should if you switch focus from the field.
Attachment #443324 - Flags: feedback?(stefanh)
Comment on attachment 443324 [details] [diff] [review]
Revised patch with placeholder v0.1i

It's a bit weird on mac, because the search bar is a separate bar that has nothing to do with the main toolbar. And it's also not customizable on mac.

Anyway, here are my comments on the mac css part:

+toolbarpaletteitem[place="palette"] > #folder-location-container > #locationFolders {
+  width: 9em;
+}


I think this is rather narrow and I also think you should give the viewPicker menulist a width as well. Perhaps having a max-width of like 11em on both?

Then I think that these style rules should be soemwhere else in the file, possibly at the bottom - but I'm sure the "real" reviewer will comment on that ;-)

I'm removing the request since here will obviously be a new patch again.
(In reply to comment #37)
> Possibly file a followup bug to port the relevant tests from:
> /mail/test/mozmill/folder-display/test-folder-toolbar.js etc.
> (is mozmill working for SeaMonkey?)

Erm, that file is in /mail/ - that should already explain that SeaMonkey isn't using it. That said, mozmill itself works in SeaMonkey, but we don't have any automated tests running on it right now.
Comment on attachment 443324 [details] [diff] [review]
Revised patch with placeholder v0.1i

UI-wise, this looks good from a first test, I'm really longing for having this available!
So, f+ for the patch working and me liking where this is going.

That said, I saw the const redeclaration as well, and as discussed on IRC, we need to find a solution to dynamically switch the label of the button when it's not the next sibling of the search item in the DOM tree (i.e. if it's positioned somewhere else).
Attachment #443324 - Flags: feedback?(kairo) → feedback+
Changes since v0.1i:
* Switched toolbaritem binding to general.xml#basecontrol as all children of toolbaritem have their own xul:box;
* Switched id to folderLocationPopup as per TB;
* Use type="radio" in mail view toolbar widget;
* Moved more of the overlay into mailWindowOverlay, only the defaultsets, hidden status and the toolbar orders are left in the messageWindow/messenger files;
* As default search toolbar is shown in 3-pane and hidden in 1-pane windows - it no longer observes mailHideMenus;
* Advanced Search Messages button now only shows "Advanced" when it is next to the search text box, when on its own it shows "Search Messages";
* Existence of locationTree is now checked for;
* When in customization dialog, locationFolders and viewPicker now have a width of 11em.

I no longer get the "redeclaration of const kViewItemAll" error but I've not changed anything to fix that, so perhaps got fixed elsewhere.
Attachment #443324 - Attachment is obsolete: true
Attachment #445744 - Flags: feedback?(philip.chee)
Attachment #445744 - Flags: feedback?(stefanh)
Attachment #445744 - Flags: feedback?(kairo)
Comment on attachment 445744 [details] [diff] [review]
Name changing buttons patch v0.1j

The switch to radio menulist caused 2 checkmarks to appear on the checked menuitems... we might need either an override or a fix in pinstripe's menu.css (the checkmark should be the same as for non-radio menulists). I also saw the "redeclaration of const kViewItemAll".
(In reply to comment #44)
> (From update of attachment 445744 [details] [diff] [review])
> I also saw the
> "redeclaration of const kViewItemAll".

Strangely if you change the order of the const in that file, you get a redeclaration error for whichever const is first. Maybe Neil or Mnyromyr can offer an explanation?
Sounds as if the script is getting included twice somehow.
(In reply to comment #46)
> Sounds as if the script is getting included twice somehow.

Does it only error on the first redeclaration it finds in a particular file?
An error in a top-level const would abort the rest of the script.
Comment on attachment 445744 [details] [diff] [review]
Name changing buttons patch v0.1j

f+ for it working and L10n stuff looking OK to me. I've also seen this redeclaration thing again, but I guess you know that anyhow ;-)
Attachment #445744 - Flags: feedback?(kairo) → feedback+
> (From update of attachment 445744 [details] [diff] [review] [details])
> I also saw the
> "redeclaration of const kViewItemAll".

Got it! msgViewPickerOverlay.xul still exists in dist/bin/chrome/messenger.jar and there is a reference to it in messenger.manifest. I guess you'll need to nuke the obj-dir.
Comment on attachment 445744 [details] [diff] [review]
Name changing buttons patch v0.1j

f+ because it all works as designed (I think).

> * Advanced Search Messages button now only shows "Advanced" when it is next to
> the search text box, when on its own it shows "Search Messages";

This only works if the button is after the search box but not before. Is this deliberate?

> +++ b/suite/locales/en-US/chrome/mailnews/messenger.dtd
> -<!ENTITY advancedButton.label "Advanced…">
> -<!ENTITY advancedButton.accesskey "A">
> 
> +++ b/suite/locales/en-US/chrome/mailnews/messenger.properties
> +# Used for Advanced Search Messages button in the toolbar.
> +advancedButtonLabel=Advanced…
> +advancedButtonAccessKey=A
> +searchButtonLabel=Search Messages…
> +searchButtonAccessKey=S
> 
> +      <button id="button-search"
> +              label=""
> +              accesskey=""
> +              indexkey=""

Optional Suggestions:

Put all the strings in the DTD. Then do something like:

      <button id="button-search"
              label=""
              accesskey=""
              indexkey=""
              labelShort="advancedButton.short.label"
              accesskeyShort="advancedButton.short.accesskey"
              labelLong="advancedButton.long.label"
              accesskeyLong="advancedButton.long.accesskey"

(You can probably think of better names for these entities)

> +function UpdateSearchToolbarButton(aDefault)
> +{

I can think of a way to avoid javascript by using clever CSS and the sibling selector but I guess that's another bug. Let's get this patch in the tree first.
Attachment #445744 - Flags: feedback?(philip.chee) → feedback+
(In reply to comment #50)
> Got it! msgViewPickerOverlay.xul still exists in dist/bin/chrome/messenger.jar
> and there is a reference to it in messenger.manifest.
That must date back to when the view picker was an optional extension...
(In reply to comment #51)
> (From update of attachment 445744 [details] [diff] [review])
> f+ because it all works as designed (I think).
> 
> > * Advanced Search Messages button now only shows "Advanced" when it is next to
> > the search text box, when on its own it shows "Search Messages";
> 
> This only works if the button is after the search box but not before. Is this
> deliberate?
Yes. Did not seem to look right when to the left of the search box (but perhaps this is due to the left-to-right mindset?)

> Optional Suggestions:
> 
> Put all the strings in the DTD. Then do something like:
> 
As there is already a string bundle defined, it is easier to use that than use strings in the DTD.

> > +function UpdateSearchToolbarButton(aDefault)
> > +{
> 
> I can think of a way to avoid javascript by using clever CSS and the sibling
> selector but I guess that's another bug. Let's get this patch in the tree
> first.
Would you then have two buttons and just use CSS to hide one of them?
(In reply to comment #53)
> > This only works if the button is after the search box but not before. Is this
> > deliberate?
> Yes. Did not seem to look right when to the left of the search box (but perhaps
> this is due to the left-to-right mindset?)

When thinking RTL, the next sibling is on the left, but it's still the next sibling, and the previous one that now would be on the right would feel just as unnatural. Good that the code works with "previous"/"next" instead of "left"/"right" terminology, as this way RTL feels correct as well. ;-)

(Oh, and I could note that it clearly can't feel "right" when it's on the _left_ but I guess that's a bad joke anyhow.)
Comment on attachment 445744 [details] [diff] [review]
Name changing buttons patch v0.1j

I'm ok with this provided that you address the double checkmarks (see comment #44).
Attachment #445744 - Flags: feedback?(stefanh) → feedback+
Depends on: 567782
Attachment #445744 - Flags: superreview?(neil)
Attachment #445744 - Flags: review?(mnyromyr)
Comment on attachment 445744 [details] [diff] [review]
Name changing buttons patch v0.1j

Sorry if anything here is covered by earlier comments.

>+            toolbar.setAttribute("class", "toolbar-secondary chromeclass-toolbar");
The modern theme really wasn't designed to accept custom toolbars... this isn't helping.

>+      <menulist id="viewPicker"
This doesn't feel right in folderPane.js; perhaps put them in msgViewPickerOverlay.xul instead?

>+function UpdateSearchToolbarButton(aDefault)
I don't like the way it changes into "Advanced..." when you're customising. Can you make it "Advanced Search" instead?

>+    // Only set attributes if things have changed.
Don't bother, just set the attributes (it gets optimised internally anyway).

>+  <broadcaster id="mailDisableViewsSearch" disabled="true"/>
messenger.xul perhaps?

>+  <toolbar id="searchToolbar"
Why do we need the search bar by default in the standalone window? Since it's only got the one button, anyone who wants it can create it in a trice.

>-                       observes="button_junk"/>
>+                       observes="button-junk"/>
I take it this is to make customising the disabled junk button work?

>+      <button id="button-search"
>+              label=""
>+              accesskey=""
Probably don't need these, since you're setting them in script.

>+                  <menuitem id="menu_showSearchToolbar" checked="false"/>
[And this is really ugly...]

>-#locationPopup {
>+#folderLocationPopup {
Why was this renamed? [quoting the XUL was harder, since it was moved too]

>+  var locationTree = document.getElementById("folderLocationPopup").tree;
>+  if (locationTree)
Don't need this test, since we only call this in the 3pane, no?

>+    menuitem.setAttribute("type", "radio");
Hmm... shouldn't there be a group name too? I guess you always override checked so it doesn't matter.

>+toolbarpaletteitem[place="palette"] > #folder-location-container > #locationFolders {
>+  max-width: 11em;
>+}
>+
>+toolbarpaletteitem[place="palette"] > #mailviews-container > #viewPicker {
>+  max-width: 11em;
>+}
Can we limit the quick search width too?
(In reply to comment #56)
> >+function UpdateSearchToolbarButton(aDefault)
> I don't like the way it changes into "Advanced..." when you're customising. Can
> you make it "Advanced Search" instead?

It looks strange to have "Advanced Search" as a standalone button with no context of the search bar and OTOH this is quite long and space-wasting when next to the search bar in the default setup. Why exactly is it bad to have the title change?
"Advanced" is an adjective. Advanced ... what? "Search" would be better.
(In reply to comment #57)
> It looks strange to have "Advanced Search" as a standalone button with no
> context of the search bar and OTOH this is quite long and space-wasting when
> next to the search bar in the default setup. Why exactly is it bad to have the
> title change?
You misunderstood me completely. I was only complaining about the choice of title that you get during customisation.
(In reply to comment #58)
> "Advanced" is an adjective. Advanced ... what? "Search" would be better.

Right, but that's what's there right now as well.

(In reply to comment #59)
> You misunderstood me completely. I was only complaining about the choice of
> title that you get during customisation.

Ah, ok.
Attachment #445744 - Flags: superreview?(neil)
Attachment #445744 - Flags: review?(mnyromyr)
Changes since v0.1j:
* Switched from using JS to using CSS sibling selectors to change button appearance - always shows as "Search Messages..." when on its own and as "Advanced..." when next to quick search box;
* Removed toolbar-secondary class code;
* Moved broadcaster to messenger.xul;
* Moved searchBar to only be in 3-Pane window, so removing the ugliness - Search button is still available from customization dialog;
* Set a name for the type="radio" menuitems;
* Set a limit to the quick search in the customization dialog;
* Fixed issue with hiding viewPicker, and associated label, when switching to a message window tab.

After discussions on IRC, viewPicker is staying in folderPane.xul (better than other options).

Yes, the change to observes="button_junk"/"button_delete" is to make the button not disabled when in customization dialog.

#locationPopup was renamed to match TB's id for the same popup.

"if (locationTree)" was used to fix the issue in comment 37 (moving the location picker into the customization dialog), if it is in the customization dialog box then it has no tree.
Attachment #445744 - Attachment is obsolete: true
Attachment #448641 - Flags: superreview?(neil)
Attachment #448641 - Flags: review?(mnyromyr)
Comment on attachment 448641 [details] [diff] [review]
Using sibling selector goodness patch v0.2 [Checkin: Comment 73]

Also requesting feedback - if it can be tested on rtl as well as ltr.
Attachment #448641 - Flags: feedback?(philip.chee)
Attachment #448641 - Flags: feedback?(kairo)
Comment on attachment 448641 [details] [diff] [review]
Using sibling selector goodness patch v0.2 [Checkin: Comment 73]

I have no clue on testing RTL and not many means to provide much more input. I'll test the patch in my own builds but only will report back in case I see a problem. I personally think doing two buttons in the DOM is too much overhead for just changing a string but I'll be happy with anything that gets review, given that I'm happy to see the search bar but not the button in my own customized UI.
Attachment #448641 - Flags: feedback?(kairo)
Ian, could you please attach LTR and RTL screenshots with this patch applied?
(In reply to comment #64)
> Ian, could you please attach LTR and RTL screenshots with this patch applied?

I'm hoping that Ratty will be able to help with those unless there are simple instructions on how to do the RTL one somewhere.
(In reply to comment #65)
> (In reply to comment #64)
> > Ian, could you please attach LTR and RTL screenshots with this patch applied?
> 
> I'm hoping that Ratty will be able to help with those unless there are simple
> instructions on how to do the RTL one somewhere.

It's not that hard.  You could try to see if the Force RTL extension works on SM (I haven't tested it recently, but it might work).  If it doesn't, then you would probably be fine with just setting the value of the intl.uidirection.en-US pref to "rtl".
> (From update of attachment 448641 [details] [diff] [review])

Grrr. bugzilla interdiff just gave up and refused to show me anything.

> Also requesting feedback - if it can be tested on rtl as well as ltr.

Sorry but I don't have a RTL locale here. Even people who read arabic here prefer their UI to be in a LTR locale.

As Ehsan says try using the Force RTL extension.
Comment on attachment 448641 [details] [diff] [review]
Using sibling selector goodness patch v0.2 [Checkin: Comment 73]

>+    <toolbar id="searchToolbar"
>+             class="chromeclass-toolbar"
>+             persist="collapsed"
>+             grippytooltiptext="&searchToolbar.tooltip;"
>+             toolbarname="&showSearchToolbarCmd.label;"
>+             accesskey="&showSearchToolbarCmd.accesskey;"
>+             customizable="true"
>+             nowindowdrag="true"
>+             mode="full"
>+             iconsize="small"
>+             labelalign="end"
I'm not convinced that these are the best defaults for this toolbar, nor in fact am I keen on our default settings for custom toolbars, but then again I don't know what Firefox defaults to.

>+  if (!threadPaneVisible)
>+    gDisableViewsSearch.setAttribute("disabled", true);
>+  else
>+    gDisableViewsSearch.removeAttribute("disabled");
[Dunno if it's worth switching these to avoid the !]
Attachment #448641 - Flags: superreview?(neil) → superreview+
Shows the "Advanced..." button picking up the right CSS, otherwise it would show as "Search Messages..."
Attachment #448726 - Flags: feedback?(ehsan)
(In reply to comment #68)
> (From update of attachment 448641 [details] [diff] [review])
> >+    <toolbar id="searchToolbar"
> >+             class="chromeclass-toolbar"
> >+             persist="collapsed"
> >+             grippytooltiptext="&searchToolbar.tooltip;"
> >+             toolbarname="&showSearchToolbarCmd.label;"
> >+             accesskey="&showSearchToolbarCmd.accesskey;"
> >+             customizable="true"
> >+             nowindowdrag="true"
> >+             mode="full"
> >+             iconsize="small"
> >+             labelalign="end"
> I'm not convinced that these are the best defaults for this toolbar, nor in
> fact am I keen on our default settings for custom toolbars, but then again I
> don't know what Firefox defaults to.
Due to the height of the defaultset small icons look best on the search bar (not sure if it was Stefan or KaiRo that highlighted this but I agree). Again, due to the height of the defaultset, having text only works when it is after the icon, not below it. I prefer to have text, but another option would be to have no text.
> 
> >+  if (!threadPaneVisible)
> >+    gDisableViewsSearch.setAttribute("disabled", true);
> >+  else
> >+    gDisableViewsSearch.removeAttribute("disabled");
> [Dunno if it's worth switching these to avoid the !]

I would have to changed the comment to match:
// disable mailviews and search if threadpane is invisble
to
// enable mailviews and search if threadpane is visible

I suppose it corrects the spelling too ;)
Either way I'll correct the spelling on checkin.
Comment on attachment 448726 [details]
Screenshots of LTR vs RTL with patch applied

Looks good RTL-wise.
Attachment #448726 - Flags: feedback?(ehsan) → feedback+
Comment on attachment 448641 [details] [diff] [review]
Using sibling selector goodness patch v0.2 [Checkin: Comment 73]

feedback=me++.

I couldn't do much testing because todays mailnews trunk is severely broken (gDBView is null) but I could move the toolbaritems around and the advanced/search messages button changed text as expected based on positioning. Ultra minor nit when in the palette window the button label says "search messages" but there is also a title for the toolbaritem "advanced search".
Attachment #448641 - Flags: feedback?(philip.chee) → feedback+
Attachment #448641 - Flags: review?(mnyromyr) → review+
Comment on attachment 448641 [details] [diff] [review]
Using sibling selector goodness patch v0.2 [Checkin: Comment 73]

http://hg.mozilla.org/comm-central/rev/8c9473e4b9f3
Attachment #448641 - Attachment description: Using sibling selector goodness patch v0.2 → Using sibling selector goodness patch v0.2 [Checkin: Comment 73]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: seamonkey2.1a1 → seamonkey2.1a2
Phew. Congrats IanN.
You need to log in before you can comment on or make changes to this bug.