842439 - Improve style and rearrange content of the about:privatebrowsing page

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Philip Chee]

From bug 837493 Comment 10:

> Some suggestions for improvement which could be done in a follow-up bug:
> 
>> +      <vbox id="normal" align="start">
>> +        <description>&privatebrowsingpage.normal;</description>
> 
> I prefer the way Firefox organises their page. With "You are not currently 
> in a private window" or "Nightly won't remember any history for this 
> window." just under the mail title block.
> 
> Needs more CSS. Suggestions:
> # Like our about:certerror and about:blocked we should have a background 
> image in the top left of the container. Firefox uses 
> chrome://global/skin/icons/question-48.png
> # Set a favicon for this page so that it shows up in the tab and 
> locationbar.
> # The contents looked rather cramped vertically. Needs more vertical space 
> around both buttons.
> 
>> +<!ENTITY privatebrowsingpage.info         "To start 
> &privatebrowsingpage.title;, you can also select File ― New ― Private 
> Window.">
> ...
>> Not sure what the character between File, New and Private Window should 
> be,
>> shows as a long "-" for me.
> I think we should use some sort of right arrow or caret. I'm sure there is a 
> UTF-8 code point for something like this.
> 
> In Firefox the title bar shows "Would you like to start Private Browsing? - 
> Nightly" when in normal mode. Ours just says "private:browsing"

Comment 1

[rsx11m]

> <button label="&privatebrowsingpage.learnmore.label;"
>         accesskey="&privatebrowsingpage.learnmore.accesskey;"
>         disabled="true"
>         oncommand="openHelp('private-browsing', 'chrome://communicator/locale/help/suitehelp.rdf');"/>

This should be enabled once the Help content is available (bug 872000).

Comment 2

[rsx11m]

I've taken care of this in my patch for the Help updates already, otherwise I wouldn't have been able to properly test the suite-toc.rdf additions (so, never mind).

Comment 3

[rsx11m]

I'm wondering what the use case for the "Switch to a normal window" button is, which simply switches back to a normal window but leaves the private one open. Firefox just states there "To stop Private Browsing, you can close this window" which sounds like the more useful "get me out of here" information.

Comment 4

[rsx11m]

I'll look into this, assuming that it's mostly about un-experimentalizing the main heading and otherwise following mostly what Firefox has done for that page. I'm not going to include any new artwork, (a) because that's probably what bug 872521 should do, and (b) I'm not really good in drawing icons. ;-)

Comment 5

[rsx11m]

Attachment: WIP Screenshots

This is where I am right now. The top row shows the current layout ("normal" on the left, "private" on the right), the 2nd row its Firefox counterparts. The next two rows show how it looks like with my current WIP patch in both themes.

> From bug 837493 Comment 10:
> > I prefer the way Firefox organises their page. With "You are not currently 
> > in a private window" or "Nightly won't remember any history for this 
> > window." just under the mail title block.

Done with similar spacing and fonts applied. I've removed the last line in return. The old wording for "normal" mode was wrong anyway ("not using Private Browsing" isn't true, you can have it active in another window, just not in the one you are looking at right now).

> > # Like our about:certerror and about:blocked we should have a background 
> > image in the top left of the container.

Due to the lack of a PB-specific icon I'm just using information-48.png for "private" mode here, question-48.png is applied in "normal" mode (same for Firefox). Note that the icon doesn't count towards the max-width: 50em limit, thus the box appears bigger now than before (still fits fine on an 800x600 screen).

I'm using warning-16.png rather than information-16.png for the secondary description on possible tracking of the activity by 3rd parties even in PB mode which seems to be more appropriate. Similar to Firefox, that warning is shown once you've entered PB mode, to avoid confusing users when they run into the PB page in a normal window.

> > # Set a favicon for this page so that it shows up in the tab and 
> > locationbar.

I've skipped that part for now, the reasons being that we don't have a favicon for any other about:xxx page (except for the Add-ons manager and Sync pages, which were inherited from Toolkit) and I don't see how it can be easily added to a XUL implementation. Maybe worth revisiting in bug 872521, if new artwork is introduced there.

> > # The contents looked rather cramped vertically. Needs more vertical space 
> > around both buttons.

Applied some vertical margins to the buttons, also increased font size to match that of the main description.

> >> +<!ENTITY privatebrowsingpage.info         "To start 
> > I think we should use some sort of right arrow or caret. I'm sure there is a 
> > UTF-8 code point for something like this.

Done.

> > In Firefox the title bar shows "Would you like to start Private Browsing? - 
> > Nightly" when in normal mode. Ours just says "private:browsing"

Done as well (the window's title bar not shown here).

(In reply to rsx11m from comment #3)
> Firefox just states there "To stop Private Browsing, you can close
> this window" which sounds like the more useful "get me out of here"
> information.

Changed that button to close the private window, e.g., if it was accidentally opened.

Comment 6

[[e-mail]]

Comment on attachment 827514 [details]
WIP Screenshots

I don't like those arrows, which is why I used the dashes in the first place.

Comment 7

[rsx11m]

(In reply to [e-mail] from bug 837493 comment #15)
> Only one of those arrows works in Windows XP for me, it's the same one that
> Bugzilla inline history uses and looks like File → New → Private Window.
> Even in a decent font it doesn't look as good as an em dash IMHO.

Yeah, I've seen your comment there. That's the normal → arrow used in HTML and looks fine to me on Windows 7 with ClearType disabled (which is my default setting as it doesn't work for me). I'm testing on Linux right now where I have font smoothing enabled, and it looks equally unsatisfactory.

If anybody has a better proposal I'll be happy to go with it, but for now I'm not sure. How does '»' look like for you, just to have something different than the '>' frequently seen in forums and used by Firefox?

Comment 8

[rsx11m]

Attachment: Matching patch

Here the patch matching attachment 827514 [details] except for using the French end-quote character instead of an arrow (it's on the Latin-1 Unicode pages, thus chances are that it's supported by most if not all system fonts).

The page is preserved as a XUL page rather than going to XHTML as Firefox did. I also kept basing the style on the global config.css to get the basic layout with the centered box (and it's #including more styles for Mac, thus better to keep it rather than merging everything into a single style sheet). To complement those definitions, an aboutPrivateBrowsing.css page is added for each of the three themes.

I didn't like the hackish style by which the Firefox version sets the window title from entities. Instead, I'm defining "titlenormal" and "titleprivate" attributes properly set at instantiation, which are then read and assigned to both "title" and the "warningTitle" label depending on the context the page was opened (I retained "about:privatebrowsing" as fallback string).

Rather than setting the .hidden property for each individual item, I've followed the Firefox implementation in this case and used top-level classes to toggle visibility for "private" vs. "normal" based on "showPrivate" and "showNormal" specified for the respective elements.

An interesting issue encountered prompted the <vbox id="trackWarnText"> definition, which appears to be a redundant box. Without it though, wrapping of the enclosed <description> doesn't work correctly and the outer box expands beyond the set "max-width" boundaries. Either I'm doing something stupidly wrong here or maybe it's another manifestation of bug 492645 where text wrapping in a XUL box yields wrong calculated heights or widths?

The other weird thing is the "margin: 0;" which I needed to explicitly define for descriptions, otherwise they'd show with an 0.6em left margin for which I couldn't figure out where it's defined.

Anyway, those are minor coding issues which are hopefully acceptable if they can't be resolved. For now, I'm mostly interested in knowing if that's the right design for this page.

Stefan, I'd like to ask you for testing this on Mac OSX again. I've tested on Windows and Linux only, works fine there. It's interesting to see that the icon names are actually quite different among the themes.

Comment 9

[rsx11m]

(In reply to rsx11m from comment #7)
> If anybody has a better proposal I'll be happy to go with it, but for now
> I'm not sure. How does '»' look like for you, just to have something
> different than the '>' frequently seen in forums and used by Firefox?

Another option may be the solid triangular arrow, '▶' (U+25B6) which should be less sensitive to scaling compared to line-width characters (I see neither aliasing nor blurring effects with this symbol).

http://www.fileformat.info/info/unicode/char/25b6/browsertest.htm

The Dingbats block has a less high and more pointed arrowhead '➤' that could be used (didn't try):

http://www.fileformat.info/info/unicode/char/27a4/browsertest.htm

Comment 10

[[e-mail]]

(In reply to rsx11m from comment #9)
> Another option may be the solid triangular arrow, '▶' (U+25B6)
Yes, that seems reasonable.

Comment 11

[rsx11m]

Attachment: Patch with triangular arrow

Does this work for everyone else in the "normal" about:privatebrowsing page?

Comment 12

[Philip Chee]

(In reply to [e-mail] from comment #10)
> (In reply to rsx11m from comment #9)
> > Another option may be the solid triangular arrow, '▶' (U+25B6)
> Yes, that seems reasonable.
I'd suggest using a character that is expected to be available with the fonts shipped with the OSes we support.

Comment 13

[rsx11m]

Agreed, that's why I've posted a quickly updated patch for easier testing. I had a similar discussion with Blake in bug 830177 using the ASCII version of apostrophes rather than their UTF-8 counterparts (for other reasons than font support as well, but that was one point to consider).

The '»' should be supported by most fonts as it's in the Latin-1 page, but looks a bit funny as it's aligned with the bottom rather than the center of the text. Thus, the '▶' would be nicer and is also better visible in those small dialog fonts.

I yet have to check on Linux, but the Windows 7 default fonts should be covered: Looked good with Segoe UI (Windows 7 Aero and Basic), Tahoma (Windows Classic), and Microsoft Sans Serif (High-Contrast Themes).

Comment 14

[[e-mail]]

Doesn't YouTube use that character when you play a video? (If you're using XP then you just see a glyph in the title bar but it works on the tab title.)

Comment 15

[rsx11m]

So, you are saying the '▶' doesn't work on the Windows XP default desktop theme?

Comment 16

[rsx11m]

(In reply to [e-mail] from comment #14)
> but it works on the tab title.

Ok, that should be the same font as the UI (the window title may be a different one).

Comment 17

[[e-mail]]

I have noticed that in general the XP fonts don't contain as many glyphs as the equivalent fonts in later versions, for instance the ⟩ character isn't present in any font shipped with XP. However in the case of the right arrow, Gecko is able to find it in Lucida Sans Unicode. (So if I was to set that as my title bar font then YouTube would display correctly.)

Comment 18

[rsx11m]

The critical factor should probably be availability in the default font of each platform. For Linux, it's available in Lucida as well, and no problems with the KDE4 default dialog font either. I've tested a variety of other fonts, the '▶' was supported in all of them. Stefan will hopefully verify for the Mac OSX default fonts.

BTW: We shouldn't overdo it. ;-) That string will only be visible if you get to about:privatebrowsing in "normal" mode, thus you'd either have to type it in manually or be referred to it by a link.

Comment 19

[rsx11m]

Attachment: Windows XP Screenshots

This is how attachment 828033 [details] [diff] [review] looks like for me on Windows XP SP2, the lowest compatibility level supported for that platform. All characters are shown, thus no problem with the default font of the desktop theme here either.

Since the text is spanning two lines anyway, I'm wondering if I should replace the warning-16.png with the larger warning-24.png? Might look better.

Comment 20

[[e-mail]]

(In reply to rsx11m from comment #19)
> Since the text is spanning two lines anyway, I'm wondering if I should
> replace the warning-16.png with the larger warning-24.png? Might look better.

Modern doesn't have a warning-24.png, and your problem is that your margin-top is set on the inner vbox next to the image rather than on the hbox containing the image and vbox.

Comment 21

[rsx11m]

(In reply to [e-mail] from comment #20)
> Modern doesn't have a warning-24.png,

Eh, correct - no 24px sizes in the modern theme, thus 16px will have to do unless we accept a mix of styles here among themes. :-\

> margin-top is set on the inner vbox next to the image rather than on the
> hbox containing the image and vbox.

I see, thus I'd have to give the hbox an id and move the "margin: 0.6em 0 0 0;" from #trackWarnText to #trackWarnBox (or whatever its name).

Comment 22

[rsx11m]

I've made the following changes locally and they resolved the <hbox> alignment issue.

aboutPrivateBrowsing.css:

> +#trackWarnBox {
> +  margin: 0.6em 0 0 0;
> +}
> +
>  #trackWarnIcon {
>    list-style-image: url("chrome://global/skin/icons/warning-16.png");
>    -moz-margin-end: 0.6em;
>  }
>  
>  #trackWarnText {
> -  margin: 0.6em 0 0 0;
>    max-width: 40em;
>  }

aboutPrivateBrowsing.xul: (I still need the <vbox> below to allow for proper wrapping.)

> -        <hbox class="showPrivate" align="center">                     
> +        <hbox id="trackWarnBox" class="showPrivate" align="center">   
>            <image id="trackWarnIcon"/>
>            <vbox id="trackWarnText">
>              <description>&privatebrowsingpage.track.warn;</description>
>            </vbox>
>          </hbox>

I'm somewhat undecided between align="center" for the <hbox> and align="start" instead. With a 16px icon I'd say that the top-aligned version looks better (similar to the screenshots as shown, but now done correctly code-wise).

Comment 23

[[e-mail]]

Comment on attachment 828033 [details] [diff] [review]
Patch with triangular arrow

Sorry for doing a code review instead of a UI review. I'm not 100% convinced of your styles but although they do appear to bear a slightly better resemblance to about:config I'm not sure whether that was your intent or you were aiming to mimic some other page.

>+      var warningScreen = document.getElementById("warningScreen");
[document.documentElement?]

>+      var titleText = "about:privatebrowsing";
Not used.

>+        warningBox.setAttribute("class", "private");
.className

>+      warningScreen.setAttribute("title", titleText);
document.title =

>+      warningTitle.setAttribute("value", titleText);
.textContent =

But actually I'd prefer it if you had two descriptions, one for normal and one for private, and you retrieved the appropriate string to set as the page title.

>+<!ENTITY privatebrowsingpage.close.info          "Once done, close the window to stop &privatebrowsingpage.title.private;.">
Not sure whether it's actually worth trying to reuse the entity any more.

>+#warningBox.normal .showPrivate {
>+  display: none;
>+}
>+
>+#warningBox.private .showNormal {
>+  display: none;
>+}
The class name "showPrivate" just sounds wrong to me for some reason, although #warningBox[type="normal"] .private looks a little clunky, and then there's always .private .normal, .normal .private { display: none; } but maybe that's too confusing?

[You could have two broadcasters named normal and private and hide the relevant broadcaster and all of the elements observing that broadcaster would automatically hide, but that doesn't fix the icon...]

>+#warningBox.private #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/information-48.png");
>+  -moz-margin-end: 3em;
>+}
>+
>+#warningBox.normal #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/question-48.png");
>+  -moz-margin-end: 3em;
>+}
Please use the child selector here. Also, please define the width and height of images to avoid reflows.

>+#trackWarnIcon {
>+  list-style-image: url("chrome://global/skin/icons/warning-16.png");
>+  -moz-margin-end: 0.6em;
>+}
>+
>+#trackWarnText {
>+  margin: 0.6em 0 0 0;
>+  max-width: 40em;
>+}
You could avoid adding margin to the icon if you didn't remove it from the text?

Comment 24

[rsx11m]

(In reply to [e-mail] from comment #23)
> Sorry for doing a code review instead of a UI review.

I'm flexible, but you are the only ui-reviewer for SeaMonkey that I'm aware of, hence the choice for that role was obvious.

> appear to bear a slightly better resemblance to about:config I'm not sure
> whether that was your intent or you were aiming to mimic some other page.

I've used the Firefox XHTML representation as initial target, as such mimicking the appearance of that page to some extent, within the framework set up for the XUL implementation of the about:config page.
 
> But actually I'd prefer it if you had two descriptions, one for normal and one
> for private, and you retrieved the appropriate string to set as the page title.

I see, thus getting the title from the titles' <description>s rather than setting it.

> The class name "showPrivate" just sounds wrong to me for some reason,
> although #warningBox[type="normal"] .private looks a little clunky, and then
> there's always .private .normal, .normal .private { display: none; } but
> maybe that's too confusing?

The confusing thing here is the negative logic, I'll try if I find a more intuitive solution.

> >+  -moz-margin-end: 0.6em;
> >+  margin: 0.6em 0 0 0;
> You could avoid adding margin to the icon if you didn't remove it from the text?

I thought the -moz-margin-end would apply a margin to the right (LTR) or left (RTL), thus wouldn't affect the top margin? Note that per comment #22, the "margin: 0.6em 0 0 0;" will go to the outer <hbox> now instead.

Comment 25

[[e-mail]]

(In reply to rsx11m from comment #22)
> (I still need the <vbox> below to allow for proper wrapping.)
<description flex="1"> should work.

> I'm somewhat undecided between align="center" for the <hbox> and
> align="start" instead. With a 16px icon I'd say that the top-aligned version
> looks better (similar to the screenshots as shown, but now done correctly
> code-wise).
I think I still prefer center.

(In reply to rsx11m from comment #24)
> I've used the Firefox XHTML representation as initial target
OK, now I know what to look for, thanks.

> > >+  -moz-margin-end: 0.6em;
> > >+  margin: 0.6em 0 0 0;
> > You could avoid adding margin to the icon if you didn't remove it from the text?
> 
> I thought the -moz-margin-end would apply a margin to the right (LTR) or
> left (RTL), thus wouldn't affect the top margin? Note that per comment #22,
> the "margin: 0.6em 0 0 0;" will go to the outer <hbox> now instead.

What I meant was that if you didn't remove the -moz-margin-start on the <description> then you mightn't need to add one to the icon.

Comment 26

[Philip Chee]

Attachment: warning-24.png from Past Modern (MPLv2)

(In reply to [e-mail] from comment #20)
> (In reply to rsx11m from comment #19)
>> Since the text is spanning two lines anyway, I'm wondering if I should
>> replace the warning-16.png with the larger warning-24.png? Might look better.
> Modern doesn't have a warning-24.png,

We have carte blanche from Kuden to reuse the CSS and images in Past Modern.

Comment 27

[rsx11m]

(In reply to Philip Chee from comment #26)
> We have carte blanche from Kuden to reuse the CSS and images in Past Modern.

That's nice. I was just about to post a new patch with Neil's comments addressed but will include that now, thus to see how it looks like with the 24px version of that icon.

Comment 28

[rsx11m]

Attachment: Updated patch (v3)

(In reply to [e-mail] from comment #23)
> >+        warningBox.setAttribute("class", "private");
> .className
> 
> >+      warningScreen.setAttribute("title", titleText);
> document.title =

Both done, simplifying the JavaScript code also by getting rid of the intermediate element variables.

> But actually I'd prefer it if you had two descriptions, one for normal and one
> for private, and you retrieved the appropriate string to set as the page title.

Done, I needed to group those in a <vbox> though given that they can't share the #warningTitle id. For consistency, I've done the same for the status line as well.

> >+<!ENTITY privatebrowsingpage.close.info          "Once done, close the window to stop &privatebrowsingpage.title.private;.">
> Not sure whether it's actually worth trying to reuse the entity any more.

Replaced those with a fixed string "Private Browsing" in all instances.

> there's always .private .normal, .normal .private { display: none; } but
> maybe that's too confusing?

I've removed the "show" part from those class names (that's what's Firefox has introduced in bug 463400 and is still using, by the way) and made them consistently "private" and "normal" both on the top and inner levels.

To solve the issue with the negative logic, the CSS rules now hide them both by default (which may be better anyway, to avoid that both strings are showing briefly at the same time before the outer class name is set) and unhide the ones matching the current status in more specific rules, which have now straight-forward selectors. I left the "#warningBox." prefix there on purpose, to clearly document which element is switched depending on the current status.

> Please use the child selector here. Also, please define the width and height
> of images to avoid reflows.

Both done, now with warning-24.png for all themes.

(In reply to [e-mail] from comment #25)
> > (I still need the <vbox> below to allow for proper wrapping.)
> <description flex="1"> should work.

Magically did the trick, though I don't understand why. ;-)

> I think I still prefer center.

Ok, looks good.

> > > >+  -moz-margin-end: 0.6em;
> > > >+  margin: 0.6em 0 0 0;
> > > You could avoid adding margin to the icon if you didn't remove it from the text?
> What I meant was that if you didn't remove the -moz-margin-start on the
> <description> then you mightn't need to add one to the icon.

I've played with that but I need some rule to get rid of the indent of those <description>s which for some reason appear without setting the margin to zero (changing them to labels didn't help either). I've made that rule a bit more specific though, but it's still equally applied to #warnTrackText as well, thus I still need the -moz-margin-end for #warnTrackIcon to keep some distance.

Making Neil the reviewer now and asking Stefan for ui-r instead, seeing that he has done so in the past.

Comment 29

[[e-mail]]

(In reply to rsx11m from comment #28)
> (In reply to comment #25)
> > > (I still need the <vbox> below to allow for proper wrapping.)
> > <description flex="1"> should work.
> Magically did the trick, though I don't understand why. ;-)
Actually it's the <vbox> that's magic.
In the case of a <description> in an <hbox> by default both its minimum and preferred width are the width of the text on a single line. This minimum width propagates up causing the whole warning to widen. Adding flex changes its minimum width to zero.
In the case of a <description> in a <vbox> the stretching axis is now horizontal, and that does weird things to the minimum and maximum widths. In particular <vbox align="start"> is great for containing a bunch of <checkbox>es of varying widths as it means focus outlines for those <checkbox>es that fit on a single line will shrink to fit. (You can't do that with an <hbox> because without flex the checkbox won't wrap and with flex it will expand to the available width.)

Comment 30

[rsx11m]

(In reply to [e-mail] from comment #29)
> In the case of a <description> in an <hbox> by default both its minimum and
> preferred width are the width of the text on a single line. This minimum
> width propagates up causing the whole warning to widen.

The first part is certainly what I'd expect as the default. The surprise of the second part is that apparently that the implicit min-width overrules an explicit max-width of the enclosing <vbox>. Even more, in the vbox > hbox > description construct here, a max-width on the <description> results in the expected wrapping of the text but nevertheless its box width remains the text width, thus stretching the enclosing <vbox>. If not a bug, it's definitely not an intuitive behavior... anyway, problem solved, thanks!

Comment 31

[[e-mail]]

Comment on attachment 829691 [details] [diff] [review]
Updated patch (v3)

>+        <description id="privateTitle"
>+                     class="private"
>+                     value="&privatebrowsingpage.title.private;"/>
>+        <description id="normalTitle"
>+                     class="normal"
>+                     value="&privatebrowsingpage.title.normal;"/>
...
>+        <description class="private"
>+                     value="&privatebrowsingpage.status.private;"/>
>+        <description class="normal"
>+                     value="&privatebrowsingpage.status.normal;"/>
In case of l10n we should make these text nodes rather than values (and use .textContent to retrieve them).

>+        <vbox class="private" align="start">
>+          <description>&privatebrowsingpage.close.info;</description>
>+          <button label="&privatebrowsingpage.close.label;"
>+                  accesskey="&privatebrowsingpage.close.accesskey;"
>+                  oncommand="window.close();"/>
>+        </vbox>
>+        <vbox class="normal" align="start">
>+          <description>&privatebrowsingpage.start.info;</description>
>+          <button label="&privatebrowsingpage.private.label;"
>+                  accesskey="&privatebrowsingpage.private.accesskey;"
>+                  oncommand="openNewPrivateWith(location.href);"/>
>+        </vbox>
The containing vbox already has align="start", so you don't need to bother with the extra boxes, just added the class to the descriptions and buttons.

>+<!ENTITY privatebrowsingpage.common.descr        "In a Private Browsing window, &brandShortName; won't keep any browser history, search history, download history, web form history, cookies, or temporary internet files.  However, files you download and bookmarks you make will be kept.">
(There's enough room here to spell out .description.)

>+#warningBox .private,
>+#warningBox .normal {
>+  display: none;
>+}
>+
>+#warningBox.private .private {
>+  display: -moz-box;
>+}
>+
>+#warningBox.normal .normal {
>+  display: -moz-box;
>+}
How about #warningBox:not(.private) .normal, #warningBox:not(.normal) .private { display: none; } ?

>+#warningBox.private > #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/information-48.png");
>+  width: 48px;
>+  height: 48px;
>+  -moz-margin-end: 3em;
>+}
>+
>+#warningBox.normal > #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/question-48.png");
>+  width: 48px;
>+  height: 48px;
>+  -moz-margin-end: 3em;
>+}
[There is almost a neater way to do this is as follows:
#warningBox.private {
  list-style-image: url(...);
}

#warningBox.normal {
  list-style-image: url(...);
}

#warningBoxIcon {
  width: 48px;
  height: 48px;
  -moz-margin-end: 3em;
}
But then you have to block the list style image from the buttons.]

>+#trackWarnBox {
>+  margin: 0.6em 0 0 0;
Just use margin-top because the others are already zero anyway.

>+#trackWarnText {
>+  max-width: 40em;
[A similar effect could be achieved by adding an end margin to the box, which would save you an id.]

>+/* Define additional styles to look similar to other about: pages */
In particular, this looks reasonably similar to neterror/certerror...

>+#warningInnerBox button {
>+  margin: 0.8em 0 1em 0;
>+  font-size: 110%;
... except for the font size which looks wrong. Please remove it.

>+#warningOuterBox description {
>+  -moz-margin-start: 0;
It just occurs to me that with the XUL change mentioned earlier, you can use child selectors for these button and description, and then the warning icon's description will keep its left margin, and you won't need the right margin on the icon. Success!

Comment 32

[[e-mail]]

(In reply to rsx11m from comment #30)
> The surprise of
> the second part is that apparently that the implicit min-width overrules an
> explicit max-width of the enclosing <vbox>.
Well, generally in CSS, min-widths override max-widths. But as I said, the description in a vbox is a magic case, and I can't remember why it works.

Comment 33

[rsx11m]

Attachment: Proposed patch (v4)

(In reply to [e-mail] from comment #31)
> In case of l10n we should make these text nodes rather than values (and use
> .textContent to retrieve them).

So done, reverted the title and status lines back to <label>s in the process.

> The containing vbox already has align="start", so you don't need to bother
> with the extra boxes, just added the class to the descriptions and buttons.

Done, and rearranged to be in top-to-bottom alternating-private/normal order.

> (There's enough room here to spell out .description.)

Ok.

> How about #warningBox:not(.private) .normal, #warningBox:not(.normal)
> .private { display: none; } ?

Done, though that has to be #warningBox:not(.private) .private, #warningBox:not(.normal) .normal {...}.

> [There is almost a neater way to do this is as follows:
> #warningBoxIcon {
>   width: 48px;
>   height: 48px;

Not that straight-forward, also given that modern doesn't use 48px icons but some odd sizes.
No change here.

> Just use margin-top because the others are already zero anyway.
> [A similar effect could be achieved by adding an end margin to the box,
> which would save you an id.]

Both done. I'm not sure if 10em is overdoing it, but it looks fine for the en-US case to have a bit of a distance, thus emphasizing the "block" appearance of that warning.

> ... except for the font size which looks wrong. Please remove it.

Ok, gone. I tried to keep it consistent with the main description.

> It just occurs to me that with the XUL change mentioned earlier, you can use
> child selectors for these button and description, and then the warning
> icon's description will keep its left margin, and you won't need the right
> margin on the icon. Success!

Correct, but the annoying left margin also affects the alternative vbox > label in the title and status lines similar to the descriptions, thus I had to add another selector specifically for those.

(In reply to [e-mail] from comment #32)
> Well, generally in CSS, min-widths override max-widths.

My assumption would be that something explicitly defined overrules anything implicitly given for the same parameter as a default, but apparently not in this case.

Phil, I'm cancelling the feedback request for you as Neil has already clarified the CSS issues I've had and everything seems to work fine now.

Comment 34

[[e-mail]]

(In reply to rsx11m from comment #33)
> (In reply to comment #31)
> > How about #warningBox:not(.private) .normal, #warningBox:not(.normal)
> > .private { display: none; } ?
> 
> Done, though that has to be #warningBox:not(.private) .private,
> #warningBox:not(.normal) .normal {...}.

Oops, too much copy & paste ;-)

> Not that straight-forward, also given that modern doesn't use 48px icons but
> some odd sizes.
Actually the size doesn't matter, just that both icons have the same size ;-)

Comment 35

[[e-mail]]

Comment on attachment 829750 [details] [diff] [review]
Proposed patch (v4)

>+        <description class="private">&privatebrowsingpage.close.info;</description>
>+        <description class="normal">&privatebrowsingpage.start.info;</description>
>+        <button class="private"
>+                label="&privatebrowsingpage.close.label;"
>+                accesskey="&privatebrowsingpage.close.accesskey;"
>+                oncommand="window.close();"/>
>+        <button class="normal"
>+                label="&privatebrowsingpage.private.label;"
>                 accesskey="&privatebrowsingpage.private.accesskey;"
>                 oncommand="openNewPrivateWith(location.href);"/>
Actually we should keep the previous order here, as the description references the button below it. r=me with that fixed.

>+<!ENTITY privatebrowsingpage.close.info          "Once done, close the window to stop Private Browsing.">
>+<!ENTITY privatebrowsingpage.close.label         "Close this window now">
>+<!ENTITY privatebrowsingpage.close.accesskey     "C">
>+
> <!ENTITY privatebrowsingpage.private.label       "Open a new private window">
> <!ENTITY privatebrowsingpage.private.accesskey   "O">
>-<!ENTITY privatebrowsingpage.info         "To start &privatebrowsingpage.title;, you can also select File â New â Private Window.">
>+<!ENTITY privatebrowsingpage.start.info          "To start Private Browsing, click the button below or select File ⶠNew ⶠPrivate Window from the menu.">
Nit: belongs above the button, as it now refers to the button below.

Comment 36

[rsx11m]

Attachment: Proposed patch (v5)

(In reply to [e-mail] from comment #34)
> > Not that straight-forward, also given that modern doesn't use 48px icons but
> > some odd sizes.
> Actually the size doesn't matter, just that both icons have the same size ;-)

Which surprisingly they don't in the modern theme; "info" and "question" have different heights. I copy-pasted those CSS lines first and only noticed after it looked funny on the screen.

Orders changed for both files per your comment #35, carrying forward r+ from attachment 829750 [details] [diff] [review].

Stefan, it's all yours now!

Comment 37

[[e-mail]]

(In reply to rsx11m from comment #36)
> (In reply to comment #34)
> > > Not that straight-forward, also given that modern doesn't use 48px icons but
> > > some odd sizes.
> > Actually the size doesn't matter, just that both icons have the same size ;-)
> Which surprisingly they don't in the modern theme; "info" and "question"
> have different heights.
You're right, I totally overlooked that one's 39px and the other 36px.

Comment 38

[Philip Chee]

Attachment: pastmodern-info-and-question-png.zip

From Past Modern:
information.png : in sizes 16, 24, 48, 64
(Modern already has information-16)

question.png in sizes 16, 24, 48, 64
(Modern already has question-16.png question-64.png, and Question.png (32px))

Please note before checking in any png graphics you should run them through pngcrush and/or optipng. This is what I do:

c:\DEV\OptiPNG\pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB contentPluginMissing.png contentPluginMissing-1.png
c:\DEV\OptiPNG\optipng -zc1-9 -zm1-9 -zs0-3 -f0-5  -nx contentPluginMissing.png

Also see Bug 631392 Comment 3

Comment 39

[rsx11m]

Comment on attachment 829857 [details]
pastmodern-info-and-question-png.zip

Thanks, I've opened bug 936898 for those and reattached the ZIP file there as adding all missing global icons is a bit beyond the scope of this specific bug.

I don't have the PNG clean-up tools you mention around at this time but will try to process warning-24.png introduced here before the patch checks in.

If you have further global dialog icons to add, we can do this in bug 936898.

Comment 40

[rsx11m]

(In reply to Philip Chee from comment #38)
> c:\DEV\OptiPNG\pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB

warning-24.png has neither of these items, it's actually quite simple (IHDR, IDAT, IEND, that's it).

Comment 41

[rsx11m]

> $ ./optipng -zc1-9 -zm1-9 -zs0-3 -f0-5  -nx warning-24.png
> ** Processing: warning-24.png
> 24x24 pixels, 4x8 bits/pixel, RGB+alpha
> Input IDAT size = 906 bytes
> Input file size = 963 bytes
> 
> Trying:
>   zc = 9  zm = 9  zs = 1  f = 5         IDAT size = 906
>   zc = 9  zm = 8  zs = 1  f = 5         IDAT size = 906
>   zc = 9  zm = 7  zs = 1  f = 5         IDAT size = 906
>   zc = 9  zm = 6  zs = 1  f = 5         IDAT size = 906
>   zc = 9  zm = 5  zs = 1  f = 5         IDAT size = 906
>   zc = 9  zm = 4  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 9  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 8  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 7  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 6  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 5  zs = 1  f = 5         IDAT size = 906
>   zc = 7  zm = 4  zs = 1  f = 5         IDAT size = 906
> 
> warning-24.png is already optimized.

I'd read this as nothing else to do here for this icon.

Comment 42

[Stefan [:stefanh]]

Attachment: Mac OSX screenshot (v5)

When it comes to the Mac Default theme, I can only see one issue here and that's the warning icon - as you can see it's a bit too close to the text and it shouldn't be centered. You would want it to be positioned about the same as the info icon is (relative to the text).

Comment 43

[Stefan [:stefanh]]

And the vertical alignment problem of the icon is of course this:

+        <hbox id="trackWarnBox" class="private" align="center">

If you change that to align="start", the icon lines up nicely with the text. Then you could add about 10px -moz-margin-end and it would look nice.

Comment 44

[rsx11m]

Neil didn't like going with align="start" thus I kept it "center" as initially proposed.

Two issues evident in attachment 829926 [details]: (1) apparently the OSX fonts are wider, resulting you to need a 3rd line where Windows and Linux only need 2 lines; (2) there doesn't seem to be an 0.6em start margin in front of the <description> box either.

Can you try the following on top of the patch:

--- a/suite/themes/classic/mac/communicator/aboutPrivateBrowsing.css
+++ b/suite/themes/classic/mac/communicator/aboutPrivateBrowsing.css
@@ -29,12 +29,13 @@

 #trackWarnBox {
   margin-top: 0.6em;
-  -moz-margin-end: 10em;
+  -moz-margin-end: 8em;
 }

 #trackWarnIcon {
   list-style-image: url("chrome://global/skin/icons/warning-24.png");
   width: 24px;
   height: 24px;
+  -moz-margin-end: 0.6em;
 }


That may do the trick already, and the Mac theme is separate anyway.

Comment 45

[Stefan [:stefanh]]

The #trackWarnBox doesn't make any difference, thus I suggest keeping the existing rule. The #trackWarnIcon rule could be increased to 1em.

This doesn't change the fact that the vertical alignment is wrong, so I'd suggest skipping the align="center" in the xul file and then use -moz-box-align in the .css files ("start" for Mac Default and "center" for the others)

Comment 46

[Stefan [:stefanh]]

Regarding optimizing, see bug 593840. optipng doesn't seem to allow you to remove sRGB and gAMA etc chunks, but running pngcrush could actually increase the size of an image. iotw, the best way to really optimize an image is to first run pngcrush (like 'pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB -brute') and then run optipng.

Comment 47

[rsx11m]

Attachment: Proposed patch (v6)

Changes made per comment #45, and it still works fine on Windows.

Re optimizing warning-24.png, neither pngcrush (with those options) nor optipng changed anything in that file, thus I assume that it's fully optimized already.

Comment 48

[rsx11m]

Comment on attachment 829926 [details]
Mac OSX screenshot (v5)

BTW: If you find a -moz-margin-end for #trackWarnBox that results in the full text fitting into two lines I'll be happy to change it. Unfortunately, spacing varies quite a bit among platforms as we've seen before.

Comment 49

[[e-mail]]

(In reply to rsx11m from comment #44)
> Neil didn't like going with align="start" thus I kept it "center" as
> initially proposed.
Now that the icon is 24px and the margin-top has moved to the #trackWarnBox it might work with align="start" anyway.

Comment 50

[[e-mail]]

Currently I still prefer center, but should you increase the icon to 32x32 I'd be happy with start.

(In reply to rsx11m from comment #44)
> apparently the OSX fonts are wider
They're bold (this is an option on Windows but nobody uses it.)

Comment 51

[rsx11m]

Attachment: Proposed patch (v6b)

I've tried the 32px version of the warning icon but it looked disproportionate to me (it's not supposed to be /that/ much of a warning), thus I'll stick with the 24px ones which fit nicely with the 2-line text in default fonts.

I'm more inclined though to reduce the 10em right margin a bit, thus giving more room for wider fonts or for l10n needing longer translations, thus allowing the string to hopefully fit into two lines to avoid the issue.

This patch is the same as attachment 829934 [details] [diff] [review] but reduced the right margin by 3em on Windows/Linux and 6em for Mac OSX.

Comment 52

[Stefan [:stefanh]]

Attachment: Mac OSX screenshot (v6b)

Comparison of how it looks with the current patch vs my suggestions below:
I found out that it looks much better when decreasing the margin-end for the warning icon to 1.5em. Then I'm wondering a bit about the second border - is there any point having it?

The titles are a bit tricky when it comes to Mac - we do some stuff with them in tabbrowser to not show the application name. The title shown in the title bar is OK when just loading the about:privatebrowsing page, then the title is "Would you like to start Private Browsing". But loading the page in Private Browsing mode, the title is "Private Browsing - Private Browsing" which is a bit odd :-)

I think what happens here is that we always append "Private Browsing" to the title when in Private Browsing mode. One possible solution might be to skip the window title for mac in this case.

Comment 53

[Stefan [:stefanh]]

I forgot one thing. It's a bit unfortunate that we get 2 "download" like that in the screenshot. It's no big deal, but you could possibly avoid that by changing the sentense to read "However, bookmarks you make and files..." (should it btw be "bookmarks you create"?)

Comment 54

[Stefan [:stefanh]]

Comment on attachment 830209 [details] [diff] [review]
Proposed patch (v6b)

Hmm, I suppose it will be difficult for you to fix the title issue since you don't have a mac - can you file a follow-up, please?

Also, I think you can keep the border (sorry for the confusion), but change the margin-end for the warning icon.

Comment 55

[rsx11m]

Comment on attachment 830209 [details] [diff] [review]
Proposed patch (v6b)

(In reply to Stefan [:stefanh] from comment #54)
> Hmm, I suppose it will be difficult for you to fix the title issue since you
> don't have a mac - can you file a follow-up, please?

Yes, that's better for someone with actual access to a Mac to fix.

The same issue should apply to Firefox. On Windows, I see "Private Browsing - Mozilla Firefox (Private Browsing)" and thus the same replication of the PB string. Adding parenthesis in the SM string should be a different bug we've talked about at some point, I don't know if that was ever filed.
  
(In reply to Stefan [:stefanh] from comment #53)
> I forgot one thing. It's a bit unfortunate that we get 2 "download" like
> that in the screenshot. It's no big deal, but you could possibly avoid that
> by changing the sentense to read "However, bookmarks you make and files..."
> (should it btw be "bookmarks you create"?)

Yeah, that's the Firefox label for this string. There was some discussion on IRC during one of the meetings to use it as a starting point, where the slightly different layout now makes them line up on top of each other.

How about "However, created bookmarks and downloaded files will be kept." which is a bit less on the active ("you") side but would avoid using identical words (and the order actually corresponds better to the order of the first sentence, where browsing history comes first and downloads later).

(In reply to Stefan [:stefanh] from comment #54)
> Also, I think you can keep the border (sorry for the confusion), but change
> the margin-end for the warning icon.

Which margin are you taking about exactly? The "-moz-margin-end" below? Please make sure that it actually works and you don't end up again with the icon having a 0px distance to the description (which was the issue prompting that change).

>+#trackWarnIcon {
>+  list-style-image: url("chrome://global/skin/icons/warning-24.png");
>+  width: 24px;
>+  height: 24px;
>+  -moz-margin-end: 1em;
>+}

Comment 56

[rsx11m]

Comment on attachment 830209 [details] [diff] [review]
Proposed patch (v6b)

>+#warningBox.private > #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/information-large.png");
>+  width: 48px;
>+  height: 48px;
>+  -moz-margin-end: 3em;
>+}
>+
>+#warningBox.normal > #warningBoxIcon {
>+  list-style-image: url("chrome://global/skin/icons/question-large.png");
>+  width: 48px;
>+  height: 48px;
>+  -moz-margin-end: 3em;
>+}

Eh, now I get it - you were talking about the information/question icon moving up tighter to the box, given that 3em is apparently wider on Mac. I'll make it 1.5em then as requested.

Comment 57

[rsx11m]

Attachment: Proposed patch (v7)

This uses now "However, created bookmarks and downloaded files will be kept."

Comment 58

[rsx11m]

Bug 939566 filed for the window-title issue on Mac OSX.

Comment 59

[[e-mail]]

(In reply to Stefan from comment #52)
> One possible solution might be to skip the window title for mac in this case.

Note that this would mean that the tab's title would revert to about:privatebrowsing and the window title would be the same as for any other untitled document.

Maybe we should change the title to be "About Private Browsing" in Private mode?

Comment 60

[rsx11m]

Note that this would also impact the title of the box in the page itself, that's the same string. I'm not sure if "About Private Browsing - Private Browsing" looks much better than without the leasing "About" part. Either way, that string is a bit problematic.

Comment 61

[rsx11m]

Comment on attachment 8333512 [details] [diff] [review]
Proposed patch (v7)

>+<!ENTITY privatebrowsingpage.title.private       "Private Browsing">
>+<!ENTITY privatebrowsingpage.title.normal        "Would you like to start Private Browsing?">

Another suggestion: In analogy to the privatebrowsingpage.title.normal string, how about "You are now in a Private Browsing window" for privatebrowsingpage.title.private? Since it is no longer used elsewhere to just refer to "Private Browsing" we could extend it here to look a bit nicer.

Comment 62

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #59)
> (In reply to Stefan from comment #52)
> > One possible solution might be to skip the window title for mac in this case.
> 
> Note that this would mean that the tab's title would revert to
> about:privatebrowsing and the window title would be the same as for any
> other untitled document.
> 
Hmm, testing (without debugQA) loading about:blank in a private window displays just "Private Browsing" in the title bar (but "Untitled" in the tab)...

Comment 63

[rsx11m]

Comment on attachment 8333512 [details] [diff] [review]
Proposed patch (v7)

>+++ b/suite/common/aboutPrivateBrowsing.xul
>+        document.getElementById("warningBox").className = "private";
>+        document.title = document.getElementById("privateTitle").textContent;

Uncommenting the second line here gives me "SeaMonkey Private Browsing" on Windows rather than "Private Browsing - SeaMonkey Private Browsing" so this should work for the other platforms as well.

So, what's the plan? Make that change here or check in the patch as is and continue discussing in bug 939566 what the final solution is?

Comment 64

[[e-mail]]

(In reply to rsx11m from comment #61)
> Another suggestion: In analogy to the privatebrowsingpage.title.normal
> string, how about "You are now in a Private Browsing window" for
> privatebrowsingpage.title.private? Since it is no longer used elsewhere to
> just refer to "Private Browsing" we could extend it here to look a bit nicer.

That sounds good to me. (Since this is a localised string I'd prefer to get this right first time if possible.)

Comment 65

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #62)
> (In reply to [e-mail] from comment #59)
> > (In reply to Stefan from comment #52)
> > > One possible solution might be to skip the window title for mac in this case.
> > 
> > Note that this would mean that the tab's title would revert to
> > about:privatebrowsing and the window title would be the same as for any
> > other untitled document.
> > 
> Hmm, testing (without debugQA) loading about:blank in a private window
> displays just "Private Browsing" in the title bar (but "Untitled" in the
> tab)...

Just to sort out any confusion here - this was with attachment #8333512 [details] [diff] [review].

Comment 66

[rsx11m]

Attachment: Patch with extended title (v7b)

(In reply to [e-mail] from comment #64)
> (Since this is a localised string I'd prefer to get
> this right first time if possible.)

Agreed, changing the string again a week later would be an annoyance.
Stefan, does this title as proposed in comment #61 work for you?

Comment 67

[rsx11m]

(In reply to Stefan [:stefanh] from comment #65)
> > Hmm, testing (without debugQA) loading about:blank in a private window
> > displays just "Private Browsing" in the title bar (but "Untitled" in the
> > tab)...
> 
> Just to sort out any confusion here - this was with attachment #8333512 [details] [diff] [review]

Stefan: As you can see from my comment #63, I could reproduce this in Windows with an actually modified patch. If the string change in attachment 8333792 [details] [diff] [review] still looks odd to you, we can always decide in the follow-up bug 939566 whether or not to #ifdef that line out for Mac OSX.

Comment 68

[rsx11m]

Attachment: Windows 7 screenshot (v7b)

For reference.

Comment 69

[rsx11m]

Comment on attachment 8333792 [details] [diff] [review]
Patch with extended title (v7b)

>+<!ENTITY privatebrowsingpage.title.private       "You are now in a Private Browsing window">

On a second thought, is the "now" correct here? You could also get to that page by just typing in about:privatebrowsing or following a link, in which case the private window is established already.

Maybe just "You are in a Private Browsing window" instead, as confirmation that you are?

Comment 70

[[e-mail]]

(In reply to rsx11m from comment #69)
> On a second thought, is the "now" correct here? You could also get to that
> page by just typing in about:privatebrowsing or following a link, in which
> case the private window is established already.
> 
> Maybe just "You are in a Private Browsing window" instead, as confirmation
> that you are?

I'd be happy with that change.

Comment 71

[rsx11m]

Attachment: Patch with modified title (v7c)

Ok, let's make sure that Stefan has the complete patch for testing.

Comment 72

[Stefan [:stefanh]]

I'm fine with the string.

Comment 73

[rsx11m]

Thanks - push for comm-central then, please.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/d7853d9e9971