Closed Bug 336997 Opened 14 years ago Closed 13 years ago

Correct Toolkit theme mistakes on OS/2

Categories

(SeaMonkey :: Themes, defect, major)

x86
OS/2
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

Details

(Keywords: fixed1.8.1)

Attachments

(5 files, 1 obsolete file)

Ever since the checkin to winstripe's menu.css on 2005-10-06 17:31 from Bug 243078 arrows on submenus and checkmarks on menuitems of Toolkit apps (at least of Firefox) were lost on OS/2 and I forgot to rectify that in Bug 314843.

As there are other problems with the appearance on OS/2 (e.g. Bug 247161 that goes nowhere), let me try to address these issues here.
Attached image Screenshot with changes
On this screenshot, note the following things:
- The check marks in the menus are back and now look like they
  do on OS/2 (and are the same as the radio marks in the menus)
- The same for the submenu arrows
- The scrollbar arrows are centered and look OS/2-like
- The scrollbar is as narrow as it is on OS/2 natively
- The scrollbar background has the correct OS/2 color
- The scrollbar thumb (is that the word for the sliding box in
  the middle of the scrollbar?) looks pressed down when clicked
  (not visible on screenshot)
- The scrollbar has the hightlights/shadows on its borders (to
  better separate it from content, this is also how it looks like
  in native OS/2 apps)
Attached patch CSS patch (obsolete) — Splinter Review
These are the necessary CSS changes. The file xulscrollbars.css is new and adapted from the copied winstripe version. I will upload the necessary icons in a minute.

Disadvantages and questions:
- While the scrollbar "thumb" looks pressed when clicked it jumps up again
  when dragged. Not sure where to change this behavior.
- I don't know how to check the @media print section? Where does it take
  effect? I don't remember ever printing something with a visible scrollbar...
- The scrollbars in print preview are currently all white. Where do they
  get their properties from?
- It is also still missing the characteristic handle in its middle. I tried
  the following CSS lines but they didn't have any effect (I also tried to
  add the background images to thumb>gripper but that didn't work, either):

thumb {
  -moz-appearance: scrollbarthumb-vertical;
  background-image: url("skin/classic/global/arrow/thumb-vert.png");
  min-height: 17px;
}
thumb[orient="horizontal"] {
  -moz-appearance: scrollbarthumb-horizontal;
  background-image: url("skin/classic/global/arrow/thumb-horiz.png");
  min-width: 17px;
}
Comment on attachment 221206 [details] [diff] [review]
CSS patch

Gavin, as you CC'd yourself, would you mind to having a first look at the patch?
Attachment #221206 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Comment on attachment 221206 [details] [diff] [review]
CSS patch

I don't have an OS/2 build to test this with, and since that's pretty much all there is to do when reviewing theme changes, I can't review this :).
Attachment #221206 - Flags: review?(gavin.sharp)
Comment on attachment 221206 [details] [diff] [review]
CSS patch

Well, so it will be Mike then.

Gavin, even if you don't want to make a real review can you shed any light on the issues I listed in comment 2? I don't think they are OS/2 specific...
Attachment #221206 - Flags: review?(mozilla)
(In reply to comment #2)
Could point 2 be related to point 3? I would guess that print preview uses the @media print rules. I don't know about the other issues.
Better use ThreeDShadow instead of ThreeDLightShadow for the upper edge of the scrollbar, so that the horizontal one is clearly seperated.
Attachment #221206 - Attachment is obsolete: true
Attachment #221276 - Flags: review?(mozilla)
Attachment #221206 - Flags: review?(mozilla)
Comment on attachment 221276 [details] [diff] [review]
Complete fix including a typo correction [checked into trunk on 2006-05-09]

r=mkaply
Attachment #221276 - Flags: review?(mozilla) → review+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Mike, thanks for the quick checkin. Is it possible, though, that the checkin of the PNGs was garbled somehow? As Walter Meinl just noted on the newsgroup checking them out again on OS/2 gives you unusable files. Did you not mark them as binaries (although the CVS server should be smart enough)?

I reopen, also because I think I should try to address the questions from comment 2 before forgetting about this stuff.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'll check the PNGs. Also, I noticed that I get CSS errors on my console on the 50% values in scrollbars.css.
PNGs are fixed now. You are correct - they weren't binary.
(In reply to comment #12)
> I'll check the PNGs. Also, I noticed that I get CSS errors on my console on the
> 50% values in scrollbars.css.

Oh yes. That looks like a cut-and-paste error.
Seems like the previous patch(es) contained rather many mistakes, most of which I correct in this one:

- the 50% cut-and-paste error
- missing plusses in the jar.mn file
- all graphics -- I think -- are supposed to have the same name
  as in winstripe so that we don't end up with two versions in the
  JAR (I therefore converted the PNG files to GIF and changed the
  entries in the file accordinly)
- to have the scrollbar thumb appear pressed down :active is the
  correct selector (:hover:active isn't)
- "-moz-appearance" is not effective for most things on OS/2
- I also added graphics for the middle of the scrollbar thumb
- Finally, I also finetuned padding and margins in the menubar
  and between menuitems and around the menuseparators so that it
  really is 1:1 as OS/2 native menus. (As remarked on the newsgroup
  the density of menuitems was too high previously)

The only thing that I could not solve is the issue with the scrollbars in print preview. I suggest to postpone that to another bug.
Attachment #225780 - Flags: review?(mozilla)
Attachment #221276 - Attachment description: Correct a typo → Complete fix including a typo correction [checked into trunk on 2006-05-09]
Comment on attachment 225780 [details] [diff] [review]
Updates [checked into trunk]

r=mkaply
Attachment #225780 - Flags: review?(mozilla) → review+
Comment on attachment 225780 [details] [diff] [review]
Updates [checked into trunk]

OK, patch checked in. I added the new GIF files and removed the now obsolete PNGs.
Attachment #225780 - Attachment description: Updates → Updates [checked into trunk]
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
OK, checked the current trunk stage into branch. I hope I didn't miss any files...
Keywords: fixed1.8.1
Similar to SeaMonkey in bug 342695 a minor bug in these fixes was discovered in the newsgroup. Instead of filing a new bug I reopen this one for it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, same fix as for SeaMonkey. (The third fix for the autocomplete dropdown is not needed for toolkit.) I hope this fixes it without introducing other problems (I haven't found any but I use Firefox much less than SeaMonkey).
Attachment #232306 - Flags: review?(mozilla)
Comment on attachment 232306 [details] [diff] [review]
Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk and 1.8 branch]

> I hope this fixes it without introducing other problems

That's why we have testers :)

r=mkaply
Attachment #232306 - Flags: review?(mozilla) → review+
Attachment #232306 - Attachment description: Fix for colored menubar without grey gaps and printer panel dropdown → Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk]
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Comment on attachment 232306 [details] [diff] [review]
Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk and 1.8 branch]

Minor OS/2 only correction, tested on trunk.
Attachment #232306 - Flags: approval1.8.1?
Comment on attachment 232306 [details] [diff] [review]
Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk and 1.8 branch]

a=drivers for the 181 branch
Attachment #232306 - Flags: approval1.8.1? → approval1.8.1+
Attachment #232306 - Attachment description: Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk] → Fix for colored menubar without grey gaps and printer panel dropdown [checked into trunk and 1.8 branch]
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.