Last Comment Bug 586026 - New places ui lacks mac css and certain icons doesn't blend in with existing
: New places ui lacks mac css and certain icons doesn't blend in with existing
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Stefan [:stefanh] (away until May 28)
:
Mentors:
Depends on: 595378
Blocks: SMPlacesBMarks 593840
  Show dependency treegraph
 
Reported: 2010-08-10 11:42 PDT by Stefan [:stefanh] (away until May 28)
Modified: 2010-09-10 15:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot, normal and hover:active state (18.65 KB, image/png)
2010-08-10 14:12 PDT, Stefan [:stefanh] (away until May 28)
no flags Details
Fix the most obvious thing for a3 (pushed for a3) (986 bytes, patch)
2010-08-11 11:22 PDT, Stefan [:stefanh] (away until May 28)
kairo: review+
kairo: approval‑seamonkey2.1a3+
Details | Diff | Review
WIP (27.31 KB, patch)
2010-08-17 15:49 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Another WIP (29.43 KB, patch)
2010-08-20 17:21 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Next WIP (32.52 KB, patch)
2010-08-22 07:14 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
New WIP (31.53 KB, patch)
2010-08-22 15:54 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Another WIP (35.46 KB, patch)
2010-08-28 16:41 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
Final patch (35.54 KB, patch)
2010-08-29 08:03 PDT, Stefan [:stefanh] (away until May 28)
no flags Details | Diff | Review
New version with namespace declarations (36.26 KB, patch)
2010-09-06 10:21 PDT, Stefan [:stefanh] (away until May 28)
mnyromyr: review+
iann_bugzilla: feedback+
Details | Diff | Review

Description Stefan [:stefanh] (away until May 28) 2010-08-10 11:42:58 PDT
When places ui landed, css for win/nix was added. However, the files in classic/mac was not updated.
Comment 1 Stefan [:stefanh] (away until May 28) 2010-08-10 12:25:22 PDT
For the record, this has the bad
Comment 2 Stefan [:stefanh] (away until May 28) 2010-08-10 12:26:20 PDT
(In reply to comment #1)
> For the record, this has the bad

effect of the "Most visited" icon disappearing when you click it in the PT
Comment 3 Stefan [:stefanh] (away until May 28) 2010-08-10 14:12:02 PDT
Created attachment 464569 [details]
screenshot, normal and hover:active state

I was actually wrong in comment #2, this is of course not because we lack css:

suite/themes/classic/mac/communicator/bookmarks/bookmarks.css

/* query-nodes should be styled even if they're not expandable */
.bookmark-item[container][query],
treechildren::-moz-tree-image(title, query) {
  list-style-image: url("chrome://communicator/skin/bookmarks/query.png");
  -moz-image-region: auto;
}

As you can see from the screenshot, the icon doesn't fit, so instead of creating a hover:active version I think the best thing to do is to not use it in the PT for now. All that needs to be done then is to remove the '.bookmark-item[container][query]'. I could probably create a new query icon that blends in with the existing ones.
Comment 4 Stefan [:stefanh] (away until May 28) 2010-08-10 14:16:52 PDT
To explain further what I mean with the missing css - it's the sidebar and certain elements in the main window that lacks some styling.
Comment 5 Robert Kaiser (not working on stability any more) 2010-08-11 09:03:11 PDT
(In reply to comment #0)
> However, the files in
> classic/mac was not updated.

For the record, this is not true, as http://hg.mozilla.org/comm-central/rev/05e7e38f82ea indeed did land the same changes for mac than it did for the normal default theme files.

This also means that anything else is off my hands, as I can't test on a mac and so have no idea what else than on the normal platforms we need on Mac. We once again need someone on a Mac to go the last mile here, apparently.
Comment 6 Stefan [:stefanh] (away until May 28) 2010-08-11 10:14:07 PDT
(In reply to comment #5)
> (In reply to comment #0)
> > However, the files in
> > classic/mac was not updated.
> 
> For the record, this is not true, as
> http://hg.mozilla.org/comm-central/rev/05e7e38f82ea indeed did land the same
> changes for mac than it did for the normal default theme files.

Yes, I was wrong, there was indeed an update of the mac css files - this was what I ment in comment #3.

Some bits are missing, though (see comment #4)- but that's no big deal, no-one's expecting someone to theme an unknown platform.

> This also means that anything else is off my hands, as I can't test on a mac
> and so have no idea what else than on the normal platforms we need on Mac.

Yeah, sure I agree except for the icon problem. See the last bit of comment #3 where I have told what can be done to temporary avoid the issue you see in the screenshot in attachment #464569 [details] (until someone fixes new icons).
Comment 7 Stefan [:stefanh] (away until May 28) 2010-08-11 11:22:16 PDT
Created attachment 464875 [details] [diff] [review]
Fix the most obvious thing for a3 (pushed for a3)
Comment 8 Robert Kaiser (not working on stability any more) 2010-08-11 11:33:29 PDT
Comment on attachment 464875 [details] [diff] [review]
Fix the most obvious thing for a3 (pushed for a3)

As long as you put it back in later with a more comprehensive fix, go for it.
Comment 9 Stefan [:stefanh] (away until May 28) 2010-08-11 11:37:53 PDT
Comment on attachment 464875 [details] [diff] [review]
Fix the most obvious thing for a3 (pushed for a3)

I pushed this as http://hg.mozilla.org/comm-central/rev/3caedf4e73cf
Comment 10 Stefan [:stefanh] (away until May 28) 2010-08-11 13:25:08 PDT
I now understand why the text-shadow rules on hover:active on the PT doesn't work:
The xul structure have changed.
Comment 11 Stefan [:stefanh] (away until May 28) 2010-08-17 15:49:40 PDT
Created attachment 466829 [details] [diff] [review]
WIP

Just putting up a wip patch here, so I don't lose it.
Comment 12 Stefan [:stefanh] (away until May 28) 2010-08-20 17:21:25 PDT
Created attachment 467962 [details] [diff] [review]
Another WIP
Comment 13 Stefan [:stefanh] (away until May 28) 2010-08-22 07:14:34 PDT
Created attachment 468136 [details] [diff] [review]
Next WIP

Almost done, I need to double-check some things first, though. For the record, these are the icon changes compared to the existing ones:
New icons: bookmarksMenu.png, bookmarksToolbar.png
Ported icons: expander-closed.png, expander-closed-active.png, expander-open.png, expander-open-active.png and query.png.
Changed icons: query-active.png (darkened query.png).

The feed icons needs some changes as well and we should probably have a better "All Bookmarks" icon, but that can wait.
Comment 14 Stefan [:stefanh] (away until May 28) 2010-08-22 15:42:34 PDT
KaiRo,

I have a question regarding the changes you landed recently: The detailsDeck-splitter (added in bug 585601), during what circumstances is it enabled?
Comment 15 Stefan [:stefanh] (away until May 28) 2010-08-22 15:54:49 PDT
Created attachment 468183 [details] [diff] [review]
New WIP

I had to do some changes because of rev 64ad57c8da27
Comment 16 Robert Kaiser (not working on stability any more) 2010-08-22 16:11:06 PDT
(In reply to comment #14)
> I have a question regarding the changes you landed recently: The
> detailsDeck-splitter (added in bug 585601), during what circumstances is it
> enabled?

It's always there, but never in an enabled state, because you can't resize the detailsDeck, you can only collapse it - that's why the splitter has a grippy (and that one works). Not sure about the design of the grippies on Mac, though...
Comment 17 Stefan [:stefanh] (away until May 28) 2010-08-22 23:01:41 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > I have a question regarding the changes you landed recently: The
> > detailsDeck-splitter (added in bug 585601), during what circumstances is it
> > enabled?
> 
> It's always there, but never in an enabled state, because you can't resize the
> detailsDeck, you can only collapse it - that's why the splitter has a grippy
> (and that one works). Not sure about the design of the grippies on Mac,
> though...

Ah, ok - got it. Yeah, no grippies in Mac classic. We'll see what we can do about this. I'm a bit surprised that you can have a parent node disabled and it's children enabled, though.
Comment 18 Stefan [:stefanh] (away until May 28) 2010-08-28 16:41:22 PDT
Created attachment 470221 [details] [diff] [review]
Another WIP

Almost there... Added some livemark icons (1 ported, 2 new). I'll file a new bug regarding the grippie, since I have no idea atm on how to solve that. Looks kind of funny, since the default mac splitter is shown and there's no indication on that it's disabled. What I will do is that I will hide the splitter until we know how to handle it.
Comment 19 Stefan [:stefanh] (away until May 28) 2010-08-29 07:57:17 PDT
Filed bug 591779 for the splitter/grippy issue.
Comment 20 Stefan [:stefanh] (away until May 28) 2010-08-29 08:03:40 PDT
Created attachment 470296 [details] [diff] [review]
Final patch

For the scope bar, the switch to buttons created some problems (the buttons should not look like pushbuttons), but I managed to re-style the buttons so they look the same as before (I used "button[group="scopeBar"]" instead of id's since I'm worried that someone will add a new button without knowing that this will regress mac).

As for the new splitter with its grippy, see bug 591779.

I might re-visit the icons and fine-tune the color scheme a bit later on (there is for instance some mis-match in color between the unsorted bookmarks icon and the bookmarksToolbar button), but I think this looks pretty good now.
Comment 21 neil@parkwaycc.co.uk 2010-08-29 10:13:46 PDT
(In reply to comment #20)
> For the scope bar, the switch to buttons created some problems (the buttons
> should not look like pushbuttons)
You mean button[type="radio"] doesn't do the right thing on Mac?

[Although I'm not sure that we wouldn't be better off with a radiogroup.]
Comment 22 Stefan [:stefanh] (away until May 28) 2010-08-29 12:02:07 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > For the scope bar, the switch to buttons created some problems (the buttons
> > should not look like pushbuttons)
> You mean button[type="radio"] doesn't do the right thing on Mac?

The scope bar buttons are sort of unique.
Comment 23 Stefan [:stefanh] (away until May 28) 2010-08-29 15:51:41 PDT
Btw, I forgot to remove the gAMA, cHRM, iCCP and sRGB chunks - I'll do that once the reviews are done.
Comment 24 Stefan [:stefanh] (away until May 28) 2010-08-29 16:17:43 PDT
Comment on attachment 470296 [details] [diff] [review]
Final patch

Neil thought there wasn't any point in him looking at this (he hasn't reviewed much of the places code), so I'm cancelling the request. I'll eventually let IanN take a look.
Comment 25 Stefan [:stefanh] (away until May 28) 2010-08-29 16:20:33 PDT
I also noted that I probably need a html namespace in one of the files
Comment 26 Stefan [:stefanh] (away until May 28) 2010-09-03 16:06:15 PDT
Comment on attachment 470296 [details] [diff] [review]
Final patch

Asking IanN for feedback since Neil passed the sr.
Comment 27 Stefan [:stefanh] (away until May 28) 2010-09-06 10:21:43 PDT
Created attachment 472424 [details] [diff] [review]
New version with namespace declarations

This one includes the missing namespace. I also added the default xul one, not sure if that is needed, though.
Comment 28 Stefan [:stefanh] (away until May 28) 2010-09-06 10:56:32 PDT
(In reply to comment #23)
> Btw, I forgot to remove the gAMA, cHRM, iCCP and sRGB chunks - I'll do that
> once the reviews are done.

Filed bug 593840 for that - better do all at once.
Comment 29 Stefan [:stefanh] (away until May 28) 2010-09-06 14:05:13 PDT
Comment on attachment 472424 [details] [diff] [review]
New version with namespace declarations

+/* We just want this when they're in the PT */
FYI, this comment is misleading since we want this if they're in the main toolbar as well
Comment 30 Karsten Düsterloh 2010-09-08 15:06:06 PDT
Comment on attachment 472424 [details] [diff] [review]
New version with namespace declarations

Looks good, just some questions:

>-  width: 0px;
>-  height: 0px;
>+  width: 0;
>+  height: 0;

Any particular reason for that?

>+  border-right: 1px solid #d7dad7;

I suppose there isn't a predefined native color for that?

>+  border: solid #B3B3B3;

And you maybe want to use a common casing style. :-P

r=me either way.
Comment 31 Stefan [:stefanh] (away until May 28) 2010-09-08 15:40:23 PDT
(In reply to comment #30)
> Comment on attachment 472424 [details] [diff] [review]
> New version with namespace declarations
> 
> Looks good, just some questions:
> 
> >-  width: 0px;
> >-  height: 0px;
> >+  width: 0;
> >+  height: 0;
> 
> Any particular reason for that?

No, not more than that the other files in the patch uses '0'. I can actually revert those 2 changes in bookmarks.css to decrease the blame.

> 
> >+  border-right: 1px solid #d7dad7;
> 
> I suppose there isn't a predefined native color for that?

No, sorry. This is btw the same colour used in mailNews/Addressbook.
> 
> >+  border: solid #B3B3B3;
> 
> And you maybe want to use a common casing style. :-P

Ah, thanks - I must have missed those, I aimed at using uppercase for everything. Will go through the colours again!
> 
> r=me either way.
Comment 32 Stefan [:stefanh] (away until May 28) 2010-09-08 16:15:56 PDT
(In reply to comment #31)

> > >+  border-right: 1px solid #d7dad7;
> > 
> > I suppose there isn't a predefined native color for that?
> 
> No, sorry. This is btw the same colour used in mailNews/Addressbook.

Ignore the last sentense since I didn't looked at the selector and confused it with the splitter.
Comment 33 Ian Neal 2010-09-10 10:37:53 PDT
Comment on attachment 472424 [details] [diff] [review]
New version with namespace declarations

Seems fine apart from the unneeded 0px to 0 changes. (Code inspection only)
Comment 34 Stefan [:stefanh] (away until May 28) 2010-09-10 11:37:35 PDT
I pushed attachment #472424 [details] [diff] [review] with the following adjustments:
- removed comment (comment #29)
- some whitespace fixes
- reverted the 0px->0 changes in bookmarks.css
- fixed casing (all colours should be upper-cased)

http://hg.mozilla.org/comm-central/rev/fd843ab226f5

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