Last Comment Bug 561668 - Port |Bug 518989 - Themes cannot give about:support an original design| to Modern
: Port |Bug 518989 - Themes cannot give about:support an original design| to Mo...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks: 545110
  Show dependency treegraph
 
Reported: 2010-04-25 14:33 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2010-05-04 03:07 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
needed


Attachments
patch (2.68 KB, patch)
2010-04-30 15:25 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (2.64 KB, patch)
2010-05-03 13:12 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
patch v3 [Checkin: comment 11] (2.55 KB, patch)
2010-05-03 15:15 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2010-04-25 14:33:46 PDT
about:support and its styles are in Toolkit now. Our Default theme gets those automatically, Modern needs a copy.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2010-04-26 14:52:24 PDT
Looking at Modern's jar.mn I found that it's missing aboutSupport.css as well so I started with bug 547458. Afterwards I'll revert to this one.
Comment 2 Robert Kaiser 2010-04-27 06:10:09 PDT
We'll need this at some point in time of the 2.1 series, but I don't want to block a specific version for it.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2010-04-29 14:35:48 PDT
(In reply to comment #1)
> Looking at Modern's jar.mn I found that it's missing aboutSupport.css as well

s/aboutSupport/aboutMemory/ of course.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2010-04-30 15:25:50 PDT
Created attachment 442828 [details] [diff] [review]
patch

about:support is quite similar to about:plugins with respect to the tables so I followed the same approach as in about.css and used the plugins.css import to act as a base. The remaining color definitions are borrowed from about.css and plugins.css.

The first-child rules are needed since plugins.css defines border-top for all tds and aboutSupport.css for th.column, column-wise tables are not used in about:plugins but in about:support, and column-wise ths can be detected but not column-wise tds (I blame the core implementation for that shortcoming) so that rule is the simplest way to achieve the goal, even though its scope is maybe a bit too broad.

@Neil: If there are other possible Modern reviewers please feel free to hand the requests for this bug and bug 547458 off to them. I guess your queue is already long enough.
Comment 5 neil@parkwaycc.co.uk 2010-05-03 11:23:36 PDT
Comment on attachment 442828 [details] [diff] [review]
patch

>+html {
>+  background: #FFF;
>+}
>+
>+body {
>+  color: #22262F;
>+  font: message-box;
Unnecessary, plugins.css includes this.

>+.page-subtitle {
>+  margin-bottom: 3em;
Too large for my taste. Maybe half as much?

>+th.column {
>+  border-top: 1px dotted #2D3B49;
>+  white-space: nowrap;
>+  width: 0px;
Needs some text-align too, no?

>+.prefs-table {
>+  width: 100%;
Also unnecessary.

>+  table-layout: fixed;
>+}
>+
>+.pref-name {
>+  width: 70%;
>+  white-space: nowrap;
>+  overflow: hidden;
>+}
>+
>+.pref-value {
>+  width: 30%;
>+  white-space: nowrap;
>+  overflow: hidden;
This doesn't actually work, you get a 50/50 split...
Comment 6 Jens Hatlak (:InvisibleSmiley) 2010-05-03 12:49:24 PDT
(In reply to comment #5)
> >+  table-layout: fixed;
> >+}
> >+
> >+.pref-name {
> >+  width: 70%;
> >+  white-space: nowrap;
> >+  overflow: hidden;
> >+}
> >+
> >+.pref-value {
> >+  width: 30%;
> >+  white-space: nowrap;
> >+  overflow: hidden;
> This doesn't actually work, you get a 50/50 split...

Bah, filed bug 563419. Will fix it for Modern right away, though.
Comment 7 Jens Hatlak (:InvisibleSmiley) 2010-05-03 13:12:24 PDT
Created attachment 443161 [details] [diff] [review]
patch v2

(In reply to comment #5)
> >+.page-subtitle {
> >+  margin-bottom: 3em;
> Too large for my taste. Maybe half as much?

If you like. It's only that with the 3em the margins above and below the Copy All to Clipboard button are equal for me (WinXP).

> >+th.column {
> >+  border-top: 1px dotted #2D3B49;
> >+  white-space: nowrap;
> >+  width: 0px;
> Needs some text-align too, no?

Yep. I only saw the th rule in plugins.css but it sets the alignment to center.
Comment 8 neil@parkwaycc.co.uk 2010-05-03 14:16:33 PDT
(In reply to comment #7)
> It's only that with the 3em the margins above and below the Copy
> All to Clipboard button are equal for me (WinXP).
It's the margins above and below the blurb that I was trying to get equal.

I notice that this page uses HTML buttons, which sucks :-(
Comment 9 neil@parkwaycc.co.uk 2010-05-03 14:30:15 PDT
Comment on attachment 443161 [details] [diff] [review]
patch v2

>+.major-section {
>+  margin-top: 2em;
>+  margin-bottom: 1em;
>+  font-size: large;
>+  text-align: start;
>+  font-weight: bold;
[As discussed on IRC we can probably lose the font-* styles]

>+tr:first-child th,
>+tr:first-child td {
Use >s please.

>+.prefs-table th.name {
>+  width: 70%;
There is only one element in the document with a class of name, isn't there?

>+  white-space: nowrap;
>+  overflow: hidden;
These styles need to apply to the data cells, not the header.
Comment 10 Jens Hatlak (:InvisibleSmiley) 2010-05-03 15:15:51 PDT
Created attachment 443210 [details] [diff] [review]
patch v3 [Checkin: comment 11]

cf. bug 563419 comment 1.
Comment 11 Jens Hatlak (:InvisibleSmiley) 2010-05-03 16:18:33 PDT
Comment on attachment 443210 [details] [diff] [review]
patch v3 [Checkin: comment 11]

http://hg.mozilla.org/comm-central/rev/61a20d4f19f5

Note You need to log in before you can comment on or make changes to this bug.