258665 - Document stylesheet switching UI

Status: VERIFIED FIXED

(Firefox Graveyard :: Help Documentation, defect)

Description

[Jeff Walden (gone starting June 8)]

Now that the stylesheet switcher is back in, we need to document what's been
added since it was removed.  This *really* has to be done ASAP for 1.0PR.

Comment 1

[Sean Umphlet (nosebleed)]

Attachment: This adds the View -> Page Style to the menu.

Jeff, do we need to document the statusbar icon as well?

Comment 2

[Sean Umphlet (nosebleed)]

Attachment: Updated patch... added my name to the patch file (for credit purpose)

Comment 3

[Jeff Walden (gone starting June 8)]

Comment on attachment 158480 [details] [diff] [review]
Updated patch... added my name to the patch file (for credit purpose)

(In reply to comment #1)
> Jeff, do we need to document the statusbar icon as well?

Roughly what you've done here (in terms of documenting the icon) is what I had
in mind.

This isn't quite sufficient, though.  For starters, the menu is Page Style. 
Therefore, the Help text should be based around the word "style".  The term
"stylesheet" is just that: a technical term that the user shouldn't have to
know in order to understand the Help text.

Second, you've only documented the menu item at the top level.	From what I
understand, the Page Style menu has a few default items, and you haven't
mentioned them or what actions they perform.  I haven't downloaded an enabled
build to see exactly what displays by default, but I know there's at least one
or two in browser-menubar.inc that should at least be mentioned in passing. 
The format should pretty much mirror that of the Text Size documentation.  You
also have to be careful how much you emphasize that the switcher can change the
page style, because some sites use CSS only for changing text properties, and
selecting "No Style" won't do a whole lot on those sites.  (Unless No Style
also disables built-in styles, but I don't think it does.)

Third, it's my personal preference to emphasize what you can do with Firefox as
you browse.  Consequently, I like sentences like, "&brandShortName; also will
display a statusbar  [this spacing is correct, right?  I'm too lazy to check
now] icon when the current web page offers multiple styles."  This certainly
isn't a full requirement per se, but as long as it isn't done overmuch (and it
isn't right now) it might be worth considering here.

Fourth, the code spacing here is different from the code spacing around it. 
Every other menu or menu item is set off by a line of whitespace for
readability and to make the code look more similar to the actual doc itself. 
This needs to be done first.

Fifth, this needs to be added to the relevant RDF files for the ToC, perhaps
the glossary (I'm leaning towards "no" here, but I don't have an opinion), and
definitely the index (as "Page Style", I think).

Sixth, and in the overall scheme least important (but still no less valid),
you're missing a period at the end of the last sentence. ;-)

Comment 4

[Sean Umphlet (nosebleed)]

> This isn't quite sufficient, though.  For starters, the menu is Page Style. 
> Therefore, the Help text should be based around the word "style".  The term
> "stylesheet" is just that: a technical term that the user shouldn't have to
> know in order to understand the Help text.

Will change this.

> 
> Second, you've only documented the menu item at the top level.	From what I
> understand, the Page Style menu has a few default items, and you haven't
> mentioned them or what actions they perform.  I haven't downloaded an enabled
> build to see exactly what displays by default, but I know there's at least one
> or two in browser-menubar.inc that should at least be mentioned in passing. 
> The format should pretty much mirror that of the Text Size documentation.  You
> also have to be careful how much you emphasize that the switcher can change the
> page style, because some sites use CSS only for changing text properties, and
> selecting "No Style" won't do a whole lot on those sites.  (Unless No Style
> also disables built-in styles, but I don't think it does.)

All I see in browser-menubar.inc is No Style, Basic Page Style, and the lines
that parse any additional styles.(In reply to comment #3). When browsing to
webpages with additional styles, they don't seem to be selectable (perhaps
another bug?) Should I document the alternate selections above or below the
default choices?

Comment 5

[Sean Umphlet (nosebleed)]

Attachment: Added RDF changes, menu item descriptions

OK, I took in most of your suggestions in this new patch. The only problem I
have is with where to document the alternate page styles. Here they don't show
up on any page I test so I don't know exactly what order they go in. According
to browser-menubar.inc they appear above the "No Style" option.

Comment 6

[Steffen Wilberg]

(In reply to comment #5)
> The only problem I have is with where to document the alternate page styles.
> Here they don't show up on any page I test so I don't know exactly what order
they go in.
Try this page: http://www.w3.org/Style/CSS/

> According to browser-menubar.inc they appear above the "No Style" option.
No, they appear below it.

Comment 7

[Jeff Walden (gone starting June 8)]

Comment on attachment 158488 [details] [diff] [review]
Added RDF changes, menu item descriptions

On the nitpicky side, Steffen's made me start running my diffs from the top of
the tree (the dir that contains mozilla/).  It's now become habitual to run
them from there, so I'd appreciate if you can do so from there as well in the
future.  A little more context would be nice, too -- three lines isn't quite
enough for firebird-index1.rdf, for example.  'cvs diff -u8' would do the job,
and it's what I've heard recommended as the default for mozilla.org patches.


>+    <rdf:Description about="#menu-view-page-style">
>+    <nc:subheadings>
>+      <rdf:Seq>i

What's with the trailing 'i'?  I don't think it would affect Help, but it's
definitely something to nix.

>+        <rdf:li> <rdf:Description ID="menu-view-page-style-no-style" nc:name="Increase" nc:link="menu_reference.xhtml#no_style"/> </rdf:li>
>+        <rdf:li> <rdf:Description ID="menu-view-page-style-basic-page-style" nc:name="Decrease" nc:link="menu_reference.xhtml#basic_page_style"/> </rdf:li>
>+        <rdf:li> <rdf:Description ID="menu-view-page-style-alternate-style" nc:name="Normal" nc:link="menu_reference.xhtml#alternate_style"/> </rdf:li>

Increase, Decrease, Normal still?  I didn't mean to copy and paste quite *that*
literally. ;-)

>+  <p>Allows you to select a style for the current web page. By default, 
>+    "Basic Page Style" is selected. &brandShortName; also will display a statusbar
>+	icon when the current web page offers multiple styles.</p>
>+	
  ^^^^^^
These are tab.	Tabs are evil.	They disrupt bonsai/printing.  They're
forbidden in the code style docs somewhere on m.o that I penned for Help. 
Don't use 'em.

Regarding the default of "Basic Page Style", that seems only partly accurate. 
It seems to be true when the default stylesheets have no title attribute (cf.
mozilla.org, which has no "Basic Page Style").	Perhaps saying, "The default is
usually Basic Page Style, though the web page's author can make another style
default if he wishes." would be a better sentence here.  Could you give me your
opinion?

>+    <h4 id="no_style">No Style</h4>
>+    <p>Removes the style formatting of the page.</p>

What do you think of "Removes style formatting from the page."?  I'm also not
super-comfortable with the use of "style" instead of "page style", but I think
it's probably good enough with the connection between "style" and "page" at the
end of the sentence.

>+    <h4 id="basic_page_style">Basic Page Style</p>
>+    <p>This is the default page style specified by the author.</p>   

The style for this sort of text in other menu entries is, [When you click this,
Firefox] "<verb> <thing that's being verbed>.  [Full sentences to explain...]" 
Something more appropriate would be along the lines of, "Displays the current
page with only a basic set of styles."	(Try something else if you can come up
with a better string.)	Also, if you can squeeze something in that says, "if
this menu item is present" or "if the page author has chosen it" or something
along those lines, that would be bonus points.

>+    <h4 id="alternate_style"></h4>
>+    <p>If the author has specified additional styles, &brandShortName will list them here for the user to select</p>
>+   

If that last line has any spaces in it (as it seems when I view it with Edit as
Comment), please remove them.  Also, you need to insert a period at the end of
the sentence, a semicolon after "&brandShortName" (you'd have caused the yellow
XHTML Screen of Death(TM) otherwise -- test the pages by opening them in
Firefox), and the word "page" before "styles" to complete this bit of text.

Comment 8

[Steffen Wilberg]

> On the nitpicky side, Steffen's made me start running my diffs from the top of
> the tree (the dir that contains mozilla/).
/me grins :)

> Perhaps saying, "The default is
> usually Basic Page Style, though the web page's author can make another style
> default if he wishes." would be a better sentence here.
As I understand it, "Basic Page Style" refers to the main/normal page style, if
there is no alternate page style offered by the author, and if he hasn't given
his normal page style a (different) name.
 
> >+    <h4 id="no_style">No Style</h4>
> >+    <p>Removes the style formatting of the page.</p>
I doesn't actually "remove" the style. It's still there on the page, but it's
not displayed. :)
 
> >+    <h4 id="basic_page_style">Basic Page Style</p>
> >+    <p>This is the default page style specified by the author.</p>   
> 
> Something more appropriate would be along the lines of, "Displays the current
> page with only a basic set of styles."
"Only a basic set" is slightly misleading. It's the default, normal style, as
intended by the author. Nothing less.

> >+    <h4 id="alternate_style"></h4>
> >+    <p>If the author has specified additional styles, &brandShortName
> will list them here for the user to select</p>
I will also display the page style switching icon in the statusbar in this case.

Comment 9

[Steffen Wilberg]

Let's see what happens to bug 258815.

Comment 10

[Steffen Wilberg]

There will be no statusbar icon in 1.0, see bug 262065.

Comment 11

[Sean Umphlet (nosebleed)]

Attachment: Patch

Hopefully the last one :)

Comment 12

[Jeff Walden (gone starting June 8)]

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

>Index: mozilla/browser/components/help/locale/en-US/firebird-toc.rdf
...
>+  <rdf:Description about="#menu-view-page-style">
>+    <nc:subheadings>
>+      <rdf:Seq>
>+        <rdf:li> <rdf:Description ID="menu-view-page-style-no-style" nc:name="No Style" nc:link="menu_reference.xhtml#no_style"/> </rdf:li>
>+        <rdf:li> <rdf:Description ID="menu-view-page-style-basic-page-style" nc:name="Basic Page Style" nc:link="menu_reference.xhtml#basic_page_style"/> </rdf:li>      
>+      </rdf:Seq>
>+    </nc:subheadings>
>+  </rdf:Description>

Add in the appropriate <rdf:li/> within the P entry for this; you've forgotten
it.

>+  <h3 id="page_style">Page Style</h3>
>+  <p>Allows you to select a style for the current web page. By default, 
>+    &quot;Basic Page Style&quot; is selected unless the author specifies his own
>+    default.</p>

How about "a different" instead of "his own"?  "his own" seems to concentrate
more on the author than on the page style itself.

>+    <h4 id="no_style">No Style</h4>
>+    <p>Removes the style formatting of the page.</p>

Remove "the" and change "of the" to "from the", please.

>+    <h4 id="basic_page_style">Basic Page Style</h4>
>+    <p>Displays the default style formatting of the page.</p>   

Let's go with, "Displays the page with a general style specified by the
author."?  It isn't necessarily default, and "formatting" doesn't fit well with
the use of "style" in other areas.

>+    <h4 id="alternate_style"></h4>
>+    <p>If the author has specified additional page styles, &brandShortName;
>+     will list them here for the user to select.</p>

Change "the user" to "you", because it's more personal.

Fix those, and you're good to go.  It shouldn't take more than five or ten
minutes.

Comment 13

[Sean Umphlet (nosebleed)]

(In reply to comment #12)
> (From update of attachment 161197 [details] [diff] [review])
> >Index: mozilla/browser/components/help/locale/en-US/firebird-toc.rdf
> ...
> >+  <rdf:Description about="#menu-view-page-style">
> >+    <nc:subheadings>
> >+      <rdf:Seq>
> >+        <rdf:li> <rdf:Description ID="menu-view-page-style-no-style"
nc:name="No Style" nc:link="menu_reference.xhtml#no_style"/> </rdf:li>
> >+        <rdf:li> <rdf:Description ID="menu-view-page-style-basic-page-style"
nc:name="Basic Page Style" nc:link="menu_reference.xhtml#basic_page_style"/>
</rdf:li>      
> >+      </rdf:Seq>
> >+    </nc:subheadings>
> >+  </rdf:Description>
> 
> Add in the appropriate <rdf:li/> within the P entry for this; you've forgotten
> it.

I don't quite understand what you're asking for here. What "P entry"??

Comment 14

[Jeff Walden (gone starting June 8)]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 161197 [details] [diff] [review])
> > >Index: mozilla/browser/components/help/locale/en-US/firebird-toc.rdf
> > ...
> > >+  <rdf:Description about="#menu-view-page-style">
> > >+    <nc:subheadings>
> > >+      <rdf:Seq>
> > >+        <rdf:li> <rdf:Description ID="menu-view-page-style-no-style"
> nc:name="No Style" nc:link="menu_reference.xhtml#no_style"/> </rdf:li>
> > >+        <rdf:li> <rdf:Description ID="menu-view-page-style-basic-page-style"
> nc:name="Basic Page Style" nc:link="menu_reference.xhtml#basic_page_style"/>
> </rdf:li>      
> > >+      </rdf:Seq>
> > >+    </nc:subheadings>
> > >+  </rdf:Description>
> > 
> > Add in the appropriate <rdf:li/> within the P entry for this; you've forgotten
> > it.
> 
> I don't quite understand what you're asking for here. What "P entry"??

D'oh!  There is no "P entry"; I was thinking of the index (where you do have the
entry in the P location).  However, the ToC still needs something like this.  In
order for this to actually show up in the ToC, you need to link it from the View
menu RDF element, which should look like this:

 <rdf:Description about="#menu-view">
    <nc:subheadings>
      <rdf:Seq>
        <rdf:li> <rdf:Description ID="menu-view-toolbars" nc:name="Toolbars"
nc:link="menu_reference.xhtml#toolbars"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-status-bar" nc:name="Status Bar"
nc:link="menu_reference.xhtml#status_bar"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-sidebar" nc:name="Sidebar"
nc:link="menu_reference.xhtml#sidebar"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-stop" nc:name="Stop"
nc:link="menu_reference.xhtml#stop"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-reload" nc:name="Reload"
nc:link="menu_reference.xhtml#reload"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-text-size" nc:name="Text Size"
nc:link="menu_reference.xhtml#text_size"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-character-encoding"
nc:name="Character Encoding" nc:link="menu_reference.xhtml#character_encod$    
   <rdf:li> <rdf:Description ID="menu-view-page-source" nc:name="Page Source"
nc:link="menu_reference.xhtml#page_source"/> </rdf:li>
        <rdf:li> <rdf:Description ID="menu-view-full-screen" nc:name="Full
Screen" nc:link="menu_reference.xhtml#full_screen"/> </rdf:li>
      </rdf:Seq>
    </nc:subheadings>
  </rdf:Description>

Add in an <rdf:li/> in the appropriate place with the appropriate info for
menu-view-page-style, and then this entry will show up in the ToC in the
appropriate place.  Otherwise, it's just floating in the middle of nowhere with
no connection to the rest of the RDF file; I don't even know if it's a valid RDF
file then.

Comment 15

[Jeff Walden (gone starting June 8)]

Any chance of making a new patch that addresses the nits in comment 14, Sean? 
I'd really, really appreciate it if you can make the corrected patch.

(Note that this patch, for reasons that escape me, doesn't mention the icon in
the status bar, so it's *not* partially obsoleted by that decision.)

Comment 16

[Jeff Walden (gone starting June 8)]

(In reply to comment #15)
> make the corrected patch

By tomorrow, please.  (sorry for bugspam)

Comment 17

[Sean Umphlet (nosebleed)]

OK, here's the scoop. My net connection died, and I won't be able to submit the
patch on my temporary connection... So, unfortunately, I don't think I can do
the patch. If someone else wants to take the bug, feel free to do so. But I
won't be able to. I'm sorry, I'll try to be back ASAP.

Comment 18

[Jeff Walden (gone starting June 8)]

I went ahead and made the updated patch (which had to be unbitrotted, probably
because of the ToC workaround fix).  Fixed branch and trunk.