406552 - Sidebar in Help Viewer is not collapsible

Status: RESOLVED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Ed]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007113002 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007113002 SeaMonkey/2.0a1pre

In the latest trunk, the sidebar in the help viewer has no collapse button.

Since this was present in previous releases, this is a regression.

Reproducible: Always

Comment 1

[Tony Mechelynck [:tonymec]]

It was present in 1.1.x, is that it?

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> It was present in 1.1.x, is that it?

Yes. Confirming for Windows, setting Platform to All.

Comment 3

[Stefan [:stefanh]]

This is because we use the toolkit help viewer and toolkit doesn't use grippies.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> This is because we use the toolkit help viewer and toolkit doesn't use
> grippies.

Thought so; setting dependency assuming the Toolkit version won't support it.

Comment 5

[Philip Chee]

Now that the toolkit help viewer is NPOTDB I think that we can get a rs+ if we want to add grippies to this. Want to come up with a patch Jens? Should be just a SMOP.

Comment 6

[Stefan [:stefanh]]

Adding grippies means that this bug should probably be moved to toolkit: help viewer. One might also want to consider the current look of the grippies (very outdated).

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #6)
> Adding grippies means that this bug should probably be moved to toolkit: help
> viewer.

Yes. Is there a way to do that in one step? Bugzilla doesn't let me choose Help Viewer when I select Toolkit. :-/

> One might also want to consider the current look of the grippies (very
> outdated).

Feel free to file a bug (if there is none yet). :-)

Patch coming up.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Unconditional patch

The attached patch unconditionally adds a grippy to the Toolkit Help Viewer. Since help.xul is already preprocessed I'm wondering whether we should take the conditional approach. Questions from my side:

1. what would be the correct symbol (MOZ_SUITE?) and is it defined in this context?
2. who's an appropriate reviewer when this bug is moved to Toolkit:Help Viewer?

Not requesting review yet for the above reasons.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Just saw that I forgot to indent. No review request, no problem, right? ;-)

Comment 10

[Stefan [:stefanh]]

Whatever you do, I'm quite convinced that this is not suitable for other apps than seamonkey, so a conditional approach is probably what you want.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Conditional approach

This time even with indentation. :-)

Comment 12

[Stefan [:stefanh]]

Comment on attachment 356412 [details] [diff] [review]
Conditional approach

Actually, now when I think of it, I'm not sure we want the left pane to be hidden completely because we show search results there... The current behavior seems odd to me (after dragging half-way it suddenly becomes hidden).

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #12)
> (From update of attachment 356412 [details] [diff] [review])
> Actually, now when I think of it, I'm not sure we want the left pane to be
> hidden completely because we show search results there...

On the other hand, we're not talking about the default here, only the *option* to collapse the left pane to give more room for the contents on the right. I think that's a case of giving the user choice.

> The current behavior
> seems odd to me (after dragging half-way it suddenly becomes hidden).

Agreed, but that can (and probably should) be changed. For example the browser sidebar (sidebarOverlay.xul) collapses at a much smaller size and I think that's because of min-width defined in navigator.css, #sidebar-splitter (cf. <https://developer.mozilla.org/en/XUL_Tutorial/Splitters>, Splitter example, text below the last image).

There was also the question whether this bug could be fixed through a suite overlay. There's a line in suite/common/jar.mn that points to suite/common/helpOverlay.xul so I think SM already is overlaying help.xul. I don't know much about overlaying, though.

Comment 14

[[e-mail]]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 356412 [details] [diff] [review])
> > Actually, now when I think of it, I'm not sure we want the left pane to be
> > hidden completely because we show search results there...
> On the other hand, we're not talking about the default here, only the *option*
> to collapse the left pane to give more room for the contents on the right. I
> think that's a case of giving the user choice.
You would need to uncollapse the left pane when the user did a search.

> > The current behavior
> > seems odd to me (after dragging half-way it suddenly becomes hidden).
> Agreed, but that can (and probably should) be changed. For example the browser
> sidebar (sidebarOverlay.xul) collapses at a much smaller size and I think
> that's because of min-width defined in navigator.css, #sidebar-splitter
Yes, that would explain it.

> There was also the question whether this bug could be fixed through a suite
> overlay. There's a line in suite/common/jar.mn that points to
> suite/common/helpOverlay.xul so I think SM already is overlaying help.xul. I
> don't know much about overlaying, though.
As I recall we do this to handle external links out of help properly. What this means is that we don't need to modify help.xul to add to it, as long as the element in question has an id. (This is how we add the help-external browser.)

Comment 15

[[e-mail]]

Comment on attachment 356412 [details] [diff] [review]
Conditional approach

As per comment #14.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Suite overlay solution

New approach with review comments addressed:
- uses suite overlay
- shows sidebar when using search
- changes min-width to 30px. Template rule is #sidebar-box in sidebarOverlay.css, not #sidebar-splitter in navigator.css as stated previously: it's always the collapsible frame that needs to be targeted. Modern's helpOverlay.xul is empty, that's why a different file is used there (the matching Classic file is part of *stripe)

Comment 17

[[e-mail]]

Comment on attachment 356788 [details] [diff] [review]
Suite overlay solution

>+function showSidebar() {
>+  document.getElementById("help-sidebar").collapsed = false;
>+}
This doesn't quite work, as the *splitter* is supposed to manage the collapsed state, and it still thinks that the sidebar is collapsed.

>+  <textbox id="findText" oncommand="showSidebar(); doFind();"/>
[Ideally you would add an event listener in the overlay, but then you'd have to fix the <window> too ;-) ]

> /* Set the minimum sidebar width so the help contents aren't squeezed together.*/
This comment no longer makes sense; by the way I worked out why you used 30px.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Suite overlay solution v2 [Checkin: Comment 20]

(In reply to comment #17)
> (From update of attachment 356788 [details] [diff] [review])
> >+function showSidebar() {
> >+  document.getElementById("help-sidebar").collapsed = false;
> >+}
> This doesn't quite work, as the *splitter* is supposed to manage the collapsed
> state, and it still thinks that the sidebar is collapsed.

Right. New approach: do it like goToggleSplitter() in abCommon.js.

> >+  <textbox id="findText" oncommand="showSidebar(); doFind();"/>
> [Ideally you would add an event listener in the overlay, but then you'd have to
> fix the <window> too ;-) ]

Consistency. ;-)

> > /* Set the minimum sidebar width so the help contents aren't squeezed together.*/
> This comment no longer makes sense; by the way I worked out why you used 30px.

Removed the comment.

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Neil clarified via IRC r+ is indeed including sr+ here so please check in and close.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 357033 [details] [diff] [review]
Suite overlay solution v2
[Checkin: Comment 20]


http://hg.mozilla.org/comm-central/rev/54b5b742341c

Comment 21

[Tony Mechelynck [:tonymec]]

Verifying on Linux:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090128 SeaMonkey/2.0a3pre - Build ID: 20090128000434

Help viewer sidebar now has a functioning grippy (clicking it toggles the sidebar's open/closed state).