Closed Bug 1289851 Opened 4 years ago Closed 3 years ago

Integrate helpoverlay files now that the Help Viewer is under Suite

Categories

(SeaMonkey :: Help Viewer, enhancement)

enhancement
Not set

Tracking

(seamonkey2.48 fixed)

RESOLVED FIXED
Tracking Status
seamonkey2.48 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(1 file, 4 obsolete files)

Bug 686168 moved the help viewer to suite.

The files:
suite\common\helpOverlay.js
suite\common\helpOverlay.xul
suite\themes\classic\communicator\helpOverlay.css
suite\themes\classic\mac\communicator\helpOverlay.css
suite\themes\modern\communicator\helpOverlay.css

could be integrated now into the code and then removed.
Severity: normal → enhancement
Version: unspecified → Trunk
Summary: Remove helpoverlay from Seamonkey → Integrate helpoverlay files now that the Help Viewer is under Suite
Attached patch 1289851-helpoverlay.patch (obsolete) — Splinter Review
Tested on Windows only. Someone should test it on a Mac if possible.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8778590 - Flags: review?(iann_bugzilla)
Hmm, it's a bit weird having 2 helpFileLayout.css.
Comment on attachment 8778590 [details] [diff] [review]
1289851-helpoverlay.patch

Just a drive-by comment:

+  list-style-image: url("chrome://communicator/skin/icons/communicatoricons.png") !important;

You shouldn't need the !important, that was only for overridning the toolkit image.
Ah, and 3 more things now when I tested this (and thought about this a bit more):
1) There’s no styling of the help window from help.css, but that's bug 1296668 (we talked about this recently).
2) We don't need the helpFileLayout.css that you moved from toolkit since that was their in-content styling, but we’re using the other helpFileLayout.css.

3)
/suite/themes/classic/mac/communicator/helpOverlay.css:

-#help-sidebar-splitter {
-  border-inline-end: 1px solid #BEBEBE;
-}

/suite/themes/classic/mac/communicator/helpviewer/help.css
 #helpsidebar-splitter {
   border-right: 1px solid #bebebe;
+  border-inline-end: 1px solid #BEBEBE;

The correct id is help-sidebar-splitter, so this also means that you don’t want the border-right rule.
Attached patch 1289851-helpoverlay-V2.patch (obsolete) — Splinter Review
Incorporated the suggestions and fixes from Stefan and also removed all now unsused files. I hope I found them all. Took the change from Bug 1296668 because it was not possible to get it working from the dist\bin without changing the location in the xul. Worked only in modern.
Attachment #8778590 - Attachment is obsolete: true
Attachment #8778590 - Flags: review?(iann_bugzilla)
Attachment #8783311 - Flags: review?(iann_bugzilla)
Comment on attachment 8783311 [details] [diff] [review]
1289851-helpoverlay-V2.patch

>+++ b/suite/common/helpviewer/help.xul

>-      <splitter id="help-sidebar-splitter" collapse="before"/>
>+      <splitter id="help-sidebar-splitter"
>+                collapse="before">
This can be on a single line as less than 80 characters.

>+        <findbar id="FindToolbar"
>+                 browserid="help-content"/>
This can be on a single line as less than 80 characters.

>+        <browser type="content"
>+                 id="help-external"
>+                 collapsed="true"/>
This can be on a single line as less than 80 characters.

>+++ b/suite/themes/classic/communicator/helpviewer/help.css

> /* Set the minimum sidebar width so the help contents aren't squeezed together.*/
>-#help-sidebar { min-width: 15em; width: 20em; max-width: 25em; }
>+#help-sidebar { min-width: 30px; width: 20em; max-width: 25em; }
Should this min-width be in em?

>+++ b/suite/themes/classic/mac/communicator/helpviewer/help.css

> /* Set the minimum sidebar width so the help contents aren't squeezed together.*/
>-#help-sidebar { min-width: 15em; width: 20em; max-width: 25em; }
>+#help-sidebar { min-width: 30px; width: 20em; max-width: 25em; }
Should this min-width be in em?

> #help-back-button > .toolbarbutton-menubutton-dropmarker {
>   margin-top: 3px;
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
>+  -moz-image-region: auto;
> }
> 
> #help-back-button:hover > .toolbarbutton-menubutton-dropmarker {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
>+  -moz-image-region: auto;
>+
> }

> #help-back-button:hover:active > .toolbarbutton-menubutton-dropmarker {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
>+  -moz-image-region: auto;
> }
These three could be combined plus the equivalent 3 for help-forward-button

> #help-back-button[disabled="true"] > .toolbarbutton-menubutton-dropmarker {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.png") !important;
>+  -moz-image-region: auto !important;
> }
> 
> 
> /* Style the forward dropmark */
> 
> #help-forward-button > .toolbarbutton-menubutton-dropmarker {
>   margin-top: 3px;
>-  list-style-image: url("chrome://help/skin/dropmark-nav.png");
>-  -moz-image-region: rect(0px, 14px, 24px, 0px);
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.png");
>+  -moz-image-region: auto;
>+
> }
The margin-top can be a separate additional rule, so the rest can be combined.

> #help-forward-button[disabled="true"] > .toolbarbutton-menubutton-dropmarker {
>-  list-style-image: url("chrome://help/skin/dropmark-nav.png") !important;
>-  -moz-image-region: rect(48px, 14px, 72px, 0px) !important;
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.png") !important;
>+  -moz-image-region: auto !important;
>+
> }
This could be combined with the equivalent help-back-button rule.

r/a=me with those addressed.
Attachment #8783311 - Flags: review?(iann_bugzilla) → review+
Attached patch 1289851-helpoverlay-V3.patch (obsolete) — Splinter Review
Patch with nits fixed. Review+ from IanN carried forward.

Stefan could you test this on OSX and see that it breaks nothing there?
Attachment #8783311 - Attachment is obsolete: true
Attachment #8787736 - Flags: review+
Attachment #8787736 - Flags: feedback?(stefanh)
Comment on attachment 8787736 [details] [diff] [review]
1289851-helpoverlay-V3.patch

The patch doesn't seem to apply anymore, mind updating it?
Attachment #8787736 - Flags: feedback?(stefanh)
Attached patch 1289851-helpoverlay-V3a.patch (obsolete) — Splinter Review
Patch adjusted for bit rot after consoleoverlay removal.

Review+ from IanN carried forward.
Attachment #8787736 - Attachment is obsolete: true
Attachment #8787977 - Flags: review+
Attachment #8787977 - Flags: feedback?(stefanh)
Comment on attachment 8787977 [details] [diff] [review]
1289851-helpoverlay-V3a.patch

diff --git a/suite/themes/classic/mac/communicator/helpviewer/help.css b/suite/themes/classic/mac/communicator/helpviewer/help.css
--- a/suite/themes/classic/mac/communicator/helpviewer/help.css
+++ b/suite/themes/classic/mac/communicator/helpviewer/help.css
@@ -1,102 +1,136 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-@import url("chrome://global/skin/");
+@import url("chrome://communicator/skin/");
 @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
 
 #HelpToolbar {
   padding-inline-start: 5px;
 }
 
 #HelpToolbar toolbarbutton {
   min-width: 0px;
-  list-style-image: url("chrome://help/skin/Toolbar.png");
+  list-style-image: url("chrome://communicator/skin/icons/communicatoricons.png";

It works fine if you add the missing right ”)” above ;-)
Attachment #8787977 - Flags: feedback?(stefanh) → feedback+
Missing bracket added.

Review+ from IanN carried forward.
Attachment #8787977 - Attachment is obsolete: true
Attachment #8787993 - Flags: review+
https://hg.mozilla.org/comm-central/rev/f823ce6ce813
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.