Closed Bug 459546 Opened 12 years ago Closed 11 years ago

Make about:sessionrestore look good on all platforms

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: zeniko, Assigned: mstange)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

* The checkmarks aren't ideally placed yet. Since real checkboxes can't be displayed in a tree, we might rather want something like in Thunderbird's Subscribe to Newsgroup dialog (i.e. empty square/square with checkmark).

* Icons don't display correctly on Gnome (cf. bug 448976 comment #32).

* Maybe the order of the buttons should change for Gnome and OS X?
Attached image screenshot
- I feel that this style is necessary. 

treechildren::-moz-tree-row(alternate, selected) {
  background-color: -moz-cellhighlight;
}

- In the third party theme that displays treechildren::-moz-tree-line, the terminal of treechildren::-moz-tree-line in the last tree needs improving. 

/* display the tree line */
treechildren::-moz-tree-line {
  visibility: visible;
}
Attached patch Patch for Linux (obsolete) — Splinter Review
I found the time to make a patch for GNOME. This will at least fix the icons and any big HIG discrepancies.
Attachment #342769 - Flags: review?(zeniko)
Comment on attachment 342769 [details] [diff] [review]
Patch for Linux

> #buttons > button {
>-  margin-top: 2em;
>+  margin-top: 1em;
>+  -moz-margin-start: 5px;
> }

The margin-top is mirrored from <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/netError.css#85>, so for consistency's sake we might want not to change that (or change it in netError.css as well).

Otherwise r+=me and thanks!
Attachment #342769 - Flags: review?(zeniko) → review+
Just got rid of the rule change.
Attachment #342769 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 342774 [details] [diff] [review]
Patch for Linux 2 (checked in)

>-/* XXXzeniko will need an appropriate color here (or none?) */
> treechildren::-moz-tree-row(alternate) {
>-  background-color: #EEEEEE;

This needs to be fixed on Windows as well for a11y. Either remove it or use rgba.
Comment on attachment 342774 [details] [diff] [review]
Patch for Linux 2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/27efd8a373aa
Attachment #342774 - Attachment description: Patch for Linux 2 → Patch for Linux 2 (checked in)
Keywords: checkin-needed
This also fixes the first point of comment #1 (which might affect the other platforms as well).
Attachment #342788 - Flags: review?(dao)
Attachment #342788 - Attachment is patch: true
Attachment #342788 - Attachment mime type: application/octet-stream → text/plain
Attachment #342788 - Flags: review?(dao) → review+
Comment on attachment 342788 [details] [diff] [review]
use rgba for the alternate windows

I think we should just remove this for Windows.
Alright, lets drop this nonnative bit completely.
Attachment #342788 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 342794 [details] [diff] [review]
no alternate color on Windows (checked in)

http://hg.mozilla.org/mozilla-central/rev/f9dbebe66234
Attachment #342794 - Attachment description: no alternate color on Windows → no alternate color on Windows (checked in)
Attachment #342794 - Flags: review+
Keywords: checkin-needed
When I go to about:sessionrestore now every other tab/item I click on the name of the tab disappears.  Will this be fixed in final release for this?

Als, what is the point of having the list like that if I can't uncheck a tab and not have it restored?
(In reply to comment #11)
A screenshot or even your build identifier would quite help in determining your issue (as this is most likely OS dependent). As for unchecking: have you tried clicking on the checkmark in the Restore column (and if the checkmark is missing, that'd be another bug to fix).
Markus: You seem to be quite invested in polishing our OS X theme. Would you mind having a look at the new about:sessionrestore page? I'm quite sure that it must be somehow wrong on OS X (not having been able to test it myself). Thanks.
This is about:sessionrestore with the first item selected.  Notice how it doesn't highlight in blue but instead erases the text of my first tab.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre
This is about:sessionrestore with the second item selected.  Notice it highlights in blue as it should unlike with the first screenshot.  This happens for every other tab.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre)
Gecko/20081013 Minefield/3.1b2pre
Depends on: 459740
Depends on: 459746
Depends on: 459751
(In reply to comment #12)
> (In reply to comment #11)
> A screenshot or even your build identifier would quite help in determining your
> issue (as this is most likely OS dependent). As for unchecking: have you tried
> clicking on the checkmark in the Restore column (and if the checkmark is
> missing, that'd be another bug to fix).

Simon, at least on OS X the checkbox isn't visible. Shall I file a new bug on that? Further I can also see that for each odd row the textcolor is changed to white when selecting the entry.
(In reply to comment #16)
> Simon, at least on OS X the checkbox isn't visible.

That's bug 459746...

> Further I can also see that for each odd row the textcolor is changed

... and this bug 459740. For both bugs, patches are welcome!
Whiteboard: [see dependencies for various OS X issues]
The OS X polish should really happen before Beta 2 if we don't want a whole lot of duplicates of the three bugs this one depends on.
Flags: blocking-firefox3.1?
Target Milestone: --- → Firefox 3.1b2
I'll start working on them tomorrow.
Comment on attachment 342774 [details] [diff] [review]
Patch for Linux 2 (checked in)

>diff -r f6ed4aa2071c browser/themes/gnomestripe/browser/aboutSessionRestore.css

> #buttons {
>   width: 100%;
>+  -moz-box-direction: reverse;

Can we actually hardcode this in the page's source with ifdefs? This way you'd still have the platform order when using a third-party theme.
This is directly related to bug 459751.
As for bug 459740, I'd suggest just dropping the whole "alternate" rules. I'd originally planned to use alternate colors for different windows instead of different lines - but as I've proven myself, that's quite easy to get quite wrong...
Attached patch Patch for OS X, v1 (obsolete) — Splinter Review
This fixes bug 459740 and adds the missing icons (checkmarks, question mark, folder).
I didn't turn off the "alternate" styling because on OS X every tree has that styling. Instead I made the general styles in tree.css apply to trees without a column picker, too, and removed them in aboutSessionRestore.css.

I'm not really happy with the appearance yet but I think this is good enough for now.
Attachment #344868 - Flags: review?(zeniko)
Attachment #344868 - Flags: review?(zeniko) → review+
Comment on attachment 344868 [details] [diff] [review]
Patch for OS X, v1

(In reply to comment #22)
> I didn't turn off the "alternate" styling because on OS X every tree
> has that styling.

Part of the issue is that "alternate" and "odd" aren't the same in this case ("odd" changes per line, "alternate" changes per window).

> Instead I made the general styles in tree.css apply to trees without a
> column picker, too, and removed them in aboutSessionRestore.css.

You'll need review from a Toolkit peer for that change, since I haven't got the slightest idea why trees without column pickers are to be treated differently. r+=me and thanks for the rest.
(In reply to comment #20)
> Can we actually hardcode this in the page's source with ifdefs?

Good idea. Would you mind offering a patch over in bug 459751 (also removing the then unneeded Gnomestripe rule)?
Yeah, I can do that.
Attachment #344868 - Flags: review?(rflint)
Comment on attachment 344868 [details] [diff] [review]
Patch for OS X, v1

>diff --git a/browser/themes/pinstripe/browser/aboutSessionRestore.css b/browser/themes/pinstripe/browser/aboutSessionRestore.css

> treechildren::-moz-tree-image(container, open, noicon) {
>-  -moz-image-region: rect(16px, 32px, 32px, 16px);
> }

Oops, forgot to qrefresh. The other two lines should be removed as well, of course.

(In reply to comment #23)
> (From update of attachment 344868 [details] [diff] [review])
> (In reply to comment #22)
> > I didn't turn off the "alternate" styling because on OS X every tree
> > has that styling.
> 
> Part of the issue is that "alternate" and "odd" aren't the same in this case
> ("odd" changes per line, "alternate" changes per window).

Ah, I didn't notice that.

Ryan, could you review the tree.css change?
Attachment #344868 - Attachment is obsolete: true
Attachment #344868 - Flags: review?(rflint)
I checked this patch in without the tree.css change.
http://hg.mozilla.org/mozilla-central/rev/272cab893e34
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Even though the bug is marked as fixed we should add bug 462618 and bug 462620 to the dependencies list for open OS X issues.
Assignee: nobody → mstange
Depends on: 462618, 462620
Keywords: helpwanted
Flags: blocking-firefox3.1?
Whiteboard: [see dependencies for various OS X issues]
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.