135607 - Help toolbar needs work

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Brian King [:kinger]]

Build ID: 2002040314

At the very least, I think the following needs to be done:
- New icon for Home. That was designed for the personal toolbar in the browser
and is smaller that the others, making the toolbar look out of proportion.
- Print button moved away from right to beside the Home button
- Mozilla logo button (throbber) should go on the right side to make it
consistent with other apps in the suite.

Comment 1

[Ian Oeschger (gone)]

Yes, kinger. It does need work. The CSS has lagged behind from inattention, and
now that masthead no longer shows up, and the navigation button in the classic
skin are having problems, I think because of the image sheet slicing that should
not be used.

Comment 2

[Brian King [:kinger]]

any thoughts appreciated on this Matthew

Comment 3

[Matthew T (active 1999-2002)]

[<][>][A] [                ] (( Search ))

Where < is Back, > is Forward, and A is Home. Print isn't nearly common enough
to deserve its own toolbar button; `File' > `Print...' is quite sufficient for that.

Comment 4

[Ian Oeschger (gone)]

Attachment: screenshot of bad sidebar. Bad, sidebar! Bad!

In addition to the lack of color differentiation between the selected tab and
the other tabs, this screenshot shows some weird blocking above and below the
sloped right side of the tab itself. Above the tab, the background color is too
dark to blend with the shadow of the tab, and in the panel, there is some
object that is picking up this same light color. Need to import the sidebar.css
styles in some better way.

Comment 5

[Ian Oeschger (gone)]

cc'ing hewitt, father of sidebar.css

Comment 6

[Brian King [:kinger]]

> ... Print isn't nearly common enough to deserve its own toolbar button; `File'
> `Print...' is quite sufficient for that.

Are you saying a menubar should be added to the Help window. I don't think there
is enough functionality to warrant that.

Comment 7

[R.J. Keller]

Attachment: Fixes problem with Home button in classic theme.

This makes the Home button look right in the classic theme. I think we need to
get something going on this bug.

Comment 8

[R.J. Keller]

Attachment: Images added

The home images added.

Comment 9

[Ian Oeschger (gone)]

moving stuff over to an outside-the-firewall email for the time being, looking
for people to pick these Help and doc bugs up for me.

Comment 10

[R.J. Keller]

--> me

Comment 11

[R.J. Keller]

I'm incorporating these ideas into the new help menu for
1.6a/Firebird/Thunderbird. I currently have the following:

[b][f][n][i][d] <-- space --> [p][t]

b = Back Button
f = Forward Button
n = Find Button
i = Increase Text Size Button
d = Decrease Text Size Button
p = Print
t = Throbber

What do you all think of this layout?

Comment 12

[R.J. Keller]

Comment on attachment 129620 [details] [diff] [review]
Fixes problem with Home button in classic theme.

jag, can you r/sr this? This patch also includes the attached image files
zipped up.

Comment 13

[[e-mail]]

Comment on attachment 129621 [details]
Images added

home-act.gif is the wrong size, also the hover effect isn't quite right.

Comment 14

[Daniel Wang]

LET ME DO THE HOME ICON!!!
will attach one shortly, depends on how fast I work :-)

Comment 15

[Daniel Wang]

Attachment: home image

heh, I thought home icons need a lot of work, didn't know it only needs some
mnor tweaking...

Comment 16

[R.J. Keller]

Comment on attachment 129620 [details] [diff] [review]
Fixes problem with Home button in classic theme.

Looks good to me. What are the dimensions of each image?

Comment 17

[R.J. Keller]

Attachment: Patch

Patch makes Daniel's images work. Thanks daniel!

Comment 18

[R.J. Keller]

Comment on attachment 133043 [details] [diff] [review]
Patch

Neil, this review should include Daniel's images.

Comment 19

[[e-mail]]

Comment on attachment 132611 [details]
home image

This is the correct image, right? So why was it marked obsolete?

Comment 20

[R.J. Keller]

OK, next up is the throbber add-on and maybe a show/hide sidebar button. I'd
like it to look somewhat like Firebird Help
(http://firebirdhelp.mozdev.org/screenshots.html), but not exactly.

Why have a throbber, you say? Because someone could use the Help Viewer to get
Help documents online. With the throbber, you could see the status.

Comment 21

[Daniel Wang]

the last check-in isn't working. The Home button now has no picture

Comment 22

[R.J. Keller]

Daniel: I made a checkin yesterday that fixed the home button. It should be in
today's nightlies.

Comment 23

[[e-mail]]

Actually you still got it wrong and I had to fix it.
FYI, you need to use cvs add -kb when adding images.

Comment 24

[R.J. Keller]

Attachment: Adds a throbber

Adds the throbber so that Help can be used to access documents on a website.
This will be more useful once content packs is in (bug 220561).

Comment 25

[Daniel Wang]

question: what happens when the user click the trobber?

Comment 26

[R.J. Keller]

Goes to the help home page. Brant told me he thought it'd be better than loading
the Navigator home page in Navigator.

Comment 27

[[e-mail]]

Comment on attachment 133534 [details] [diff] [review]
Adds a throbber

>+    if (aStateFlags &
>+        aWebProgress.STATE_START &&
Won't work, STATE_START is defined on nsIWebProgressListener, not
nsIWebProgress

>   onProgressChange : function(aWebProgress, aRequest, aCurSelfProgress,
>                               aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {},
>   onSecurityChange : function(aWebProgress, aRequest, state) {},
onStateChange appears to be missing, could you add it please?

>+  init : function()
>+  {
>+    this.throbberElement = document.getElementById("navigator-throbber");
>+  },
>+
>+  destroy : function()
>+  {
>+    //this is needed to avoid memory leaks.
>+    this.throbberElement = null;
>+  },
I don't see where you call these.

>+          <button id="navigator-throbber" oncommand="goHome();" tooltiptext="&throbber.tooltip;"/>
This tooltip is wrong.

Comment 28

[R.J. Keller]

Attachment: Adds Throbber v2

> onStateChange appears to be missing, could you add it please?

Missing? It's the function the throbber code is in!

> I don't see where you call these.

this.throbberElement is called a lot in the functions.

All other comments are in this patch.

Comment 29

[[e-mail]]

Comment on attachment 134086 [details] [diff] [review]
Adds Throbber v2

>+    const nsIWebProgressHandler = Components.interfaces.nsIWebProgressListener;
Use nsIWebProgressListener for consistency (it's confusing to have a const with
a different name).

>+
>+  init : function()
>+  {
>+    this.throbberElement = document.getElementById("navigator-throbber");
>+  },
>+
>+  destroy : function()
>+  {
>+    //this is needed to avoid memory leaks.
>+    this.throbberElement = null;
>+  },
>+
This won't work as you don't actually call these functions anywhere.

Comment 30

[R.J. Keller]

> This won't work as you don't actually call these functions anywhere.

How does the navigator throbber do this? This is a tough patch for me because I
can't debug it (Help is to fast to see if the throbber moves!)

I've been trying to copy the navigator throbber as much as possible.

Comment 31

[[e-mail]]

I use those easily identified external links :-)

OTOH I think that init() is called before addProgressListener() and destroy() is
called after removeProgressListener().

Comment 32

[R.J. Keller]

Attachment: Adds throbber v2.1

Patch with Neil's comments. The help.dtd file looks a little messed up because
it's in the wrong encoding. I think that Neil ran into this problem.

Comment 33

[[e-mail]]

Comment on attachment 134115 [details] [diff] [review]
Adds throbber v2.1

I thought I had made it clear, but you need to call
window.XULBrowserWindow.init(); too. Also can you please reference the memory
leak bug number in the comment, thanks.

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 134115 [details] [diff] [review]
Adds throbber v2.1

>--- resources/content/help.js	16 Oct 2003 08:17:06 -0000	1.48
>+++ resources/content/help.js	25 Oct 2003 16:09:29 -0000
>@@ -321,7 +321,22 @@
> 
> nsHelpStatusHandler.prototype =
> {
>-  onStateChange : function(aWebProgress, aRequest, aStateFlags, aStatus) {},
>+  onStateChange : function(aWebProgress, aRequest, aStateFlags, aStatus)
>+  {
>+    const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
>+
>+    // Turn on the throbber.
>+    if (aStateFlags &
>+        nsIWebProgressListener.STATE_START)
>+    {
>+      this.throbberElement.setAttribute("busy", "true");
>+    }
>+    else if (aStateFlags &
>+             nsIWebProgressListener.STATE_STOP)
>+    {
>+      this.throbberElement.renoveAttribute("busy");
>+    }


Style nits: please follow the file's bracing style, which happens to be:

  function foo()
  { // brace on own line at start of function
    if (cond) // try and fit cond on a single line
      bar; // subsequently, no braces here.
    else if (cond2)
      baz; // and still no braces
  }

You have plenty of space to put the contents of the if and else if conditions
all on one line following even the most stringent column requirements. 
Fortunately, our requirements are not that stringent (<80 columns) and the 58
and 61 columns required here respectively for the conditions to fit on a single
line dictate that they should be.

Comment 35

[R.J. Keller]

Attachment: Adds throbber v2.2

Comment 36

[[e-mail]]

Comment on attachment 134129 [details] [diff] [review]
Adds throbber v2.2

>+  //Start the status handler.
>+  window.XULBrowserWindow.init();
Although it seems to work I would prefer to see this just after it's created.
Actually I think that displayTopic(); should be last, but I'm not too worried
about that.

>+      this.throbberElement.renoveAttribute("busy");
And fix this typo too.

Comment 37

[R.J. Keller]

Attachment: Adds throbber v2.2.1

Comment 38

[R.J. Keller]

Attachment: Adds throbber v2.3

phew! If this isn't the final patch, then I don't think one will ever come ;)

Comment 39

[[e-mail]]

BTW, you don't appear to need destroy(), it doesn't actually leak.

Comment 40

[[e-mail]]

Although who's to say that it won't start leaking again in the future ;-)

Comment 41

[R.J. Keller]

Attachment: Adds throbber - moves init() before adding progress listener

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 134351 [details] [diff] [review]
Adds throbber - moves init() before adding progress listener

We've wrapped up 1.6a. This will have to land when the tree opens for 1.6b.

Comment 43

[Alec Flett]

Comment on attachment 134351 [details] [diff] [review]
Adds throbber - moves init() before adding progress listener

sr=alecf

Comment 44

[[e-mail]]

I hope you don't mind, but I checked in attachment 134351 [details] [diff] [review].

Comment 45

[R.J. Keller]

fine with me. Thanks Neil!

Comment 46

[Daniel Wang]

v