Closed Bug 456214 Opened 12 years ago Closed 11 years ago

Polish toolbar buttons in the Add-ons Manager, Page Info dialog and Error Console

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b3

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: polish, verified1.9.1)

Attachments

(2 files, 8 obsolete files)

No description provided.
Attached patch patch, rev1 (obsolete) — Splinter Review
Depends on: 456216
Comment on attachment 339595 [details] [diff] [review]
patch, rev1

This patch only works together with the patch in bug 456216.
Attached patch rev2 (obsolete) — Splinter Review
Attachment #339595 - Attachment is obsolete: true
Attached patch v3: with text-shadow (obsolete) — Splinter Review
I'll request review when I update the patch for bug 398928.
Attachment #339635 - Attachment is obsolete: true
Blocks: 458197
Attached patch v4 (obsolete) — Splinter Review
Attachment #341254 - Attachment is obsolete: true
Attachment #344109 - Flags: review?(rflint)
Comment on attachment 344109 [details] [diff] [review]
v4

New patch coming soon.
Attachment #344109 - Attachment is obsolete: true
Attachment #344109 - Flags: review?(rflint)
Attached patch v5 (obsolete) — Splinter Review
Attachment #350576 - Flags: review?(dao)
Attached patch v5.1 (obsolete) — Splinter Review
I removed two unnecessary !importants. The only ones left are "color" rules on toolbarbuttons; these are necessary because toolbarbutton.css specifies color: buttontext !important. I don't know why it does that but I don't want to change it in this bug.
Attachment #350576 - Attachment is obsolete: true
Attachment #350577 - Flags: review?(dao)
Attachment #350576 - Flags: review?(dao)
Attachment #350577 - Flags: review?(dao) → review-
Comment on attachment 350577 [details] [diff] [review]
v5.1

It's not obvious what "segm" means... please rename.

> <!DOCTYPE window [
>   <!ENTITY % pageInfoDTD SYSTEM "chrome://browser/locale/pageInfo.dtd">
>   %pageInfoDTD;
>+  <!ENTITY % global SYSTEM "chrome://global/locale/global.dtd"> %global;

You should follow the prevailing style and put %global; on the next line.

>+#console-toolbox[chromedir="ltr"] #Console\:modeAll,
>+#console-toolbox[chromedir="rtl"] #Console\:modeMessages {
>+  border-width: 0 4px 0 6px;
>+  -moz-border-image: url("chrome://global/skin/toolbar/segm-left.png") 0 4 0 6 repeat;
> }

The chromedir attributes should be on the buttons.

>+#ToolbarMode > #Console\:clear {

Why "#ToolbarMode >"?

>+++ b/toolkit/themes/pinstripe/global/viewbuttons.css
>+window:not([active]) #viewGroup > radio {

Use :root:not([active]) so that this works for arbitrary root elements?
Attached patch v6 (obsolete) — Splinter Review
(In reply to comment #9)
> (From update of attachment 350577 [details] [diff] [review])
> It's not obvious what "segm" means... please rename.
> You should follow the prevailing style and put %global; on the next line.
> The chromedir attributes should be on the buttons.
> Use :root:not([active]) so that this works for arbitrary root elements?

Fixed.

> >+#ToolbarMode > #Console\:clear {
> 
> Why "#ToolbarMode >"?

To make the rule more specific than "#ToolbarMode toolbarbutton", but I guess !important makes the intention clearer.
Attachment #350577 - Attachment is obsolete: true
Attachment #350652 - Flags: review?(dao)
Thanks Dão! It's really great to have a responsive theme reviewer.
Oh, viewbutton-middle-selected.png and viewbutton-middle-inactive.png are missing in this patch...
Would it be possible to reuse viewbuttons.css in console.css? I.e. wrap the toolbarbuttons in <hbox id="viewGroup" chromedir="&locale.dir"></hbox>, use #viewGroup > * instead of #viewGroup > radio and add [checked] similar to [selected]?
I'll try that. By the way, do I need a license header in viewbuttons.css?
I guess we could do without the license header there. Then again, it can't hurt to add it.
Attached patch v7 (obsolete) — Splinter Review
(In reply to comment #13)
> use #viewGroup > * instead of #viewGroup > radio

This would result in a styled toolbarseparator (and probably in a slower selector, but I don't know if that matters much). In this patch I'm just duplicating all the selectors for toolbarbuttons, but now I realize that I could just set display:none on the toolbarseparator - do you want me to do that?

(In reply to comment #15)
> I guess we could do without the license header there. Then again, it can't hurt
> to add it.

I added it.
Attachment #350652 - Attachment is obsolete: true
Attachment #350775 - Flags: review?(dao)
Attachment #350652 - Flags: review?(dao)
Is it necessary that #viewGroup contains #Console:clear and the toolbarseparator? I had this structure in mind:

#viewGroup
  #Console:modeAll
  #Console:modeErrors
  #Console:modeWarnings
  #Console:modeMessages
toolbarseparator
#Console:clear
Ah, that makes a lot more sense.
Attached patch v8Splinter Review
Attachment #350775 - Attachment is obsolete: true
Attachment #350782 - Flags: review?(dao)
Attachment #350775 - Flags: review?(dao)
Attachment #350782 - Flags: review?(dao) → review+
Attachment #350782 - Flags: approval1.9.1?
pushed: http://hg.mozilla.org/mozilla-central/rev/5090a2e23f6b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #350782 - Flags: approval1.9.1? → approval1.9.1+
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081213 Minefield/3.2a1pre ID:20081213020516

Markus, is this patch ready for 1.9.1 check-in?
Status: RESOLVED → VERIFIED
I'm not sure. Sometimes I can see missing button backgrounds, so maybe we should wait for bug 468473 before landing this on 1.9.1.

STR:
1. Start / restart Firefox.
2. Load this page in the active tab.
3. Click on the favicon in the location bar.
4. Click "More information…".

Now the "Media" button's background is missing in the page info dialog.
Depends on: 468473
Blocks: 470181
The patch in bug 468473 fixes the problem in comment 23.
Whiteboard: [1.9.1 landing waits for bug 468473 first]
Comment on attachment 350782 [details] [diff] [review]
v8

>+++ b/toolkit/themes/pinstripe/global/viewbuttons.css

>+ * The Original Code is the nsSessionStore component.

Can you please fix this on trunk and before landing on branch?
Markus, will this get into branch?
Bug 468473 has been fixed. Means we can go for 1.9.1. Markus please take care of comment 25. It also has to be corrected on mozilla-central.

I've started a tryserver build and will check if everything works well.
Somehow the patch wasn't applied to the tryserver build. So stopped for now.

In any way the patch doesn't apply cleanly on 1.9.1:

patching file toolkit/themes/pinstripe/global/console/console.css
Hunk #2 succeeded at 183 (offset 1 line).
patching file toolkit/themes/pinstripe/global/jar.mn
Hunk #1 succeeded at 46 (offset 1 line).
Hunk #2 succeeded at 205 (offset 4 lines).
(In reply to comment #25)
> (From update of attachment 350782 [details] [diff] [review])
> >+++ b/toolkit/themes/pinstripe/global/viewbuttons.css
> 
> >+ * The Original Code is the nsSessionStore component.
> 
> Can you please fix this on trunk and before landing on branch?

Oops, thanks.
http://hg.mozilla.org/mozilla-central/rev/793f8f182fb2

I'll land the patch on branch later today.
Or not. I'll wait for bug 470181 because I don't want to break SeaMonkey.
No longer blocks: 470181
Depends on: 470181
Whiteboard: [1.9.1 landing waits for bug 468473 first] → [1.9.1 landing waits for bug 470181 first]
(In reply to comment #30)
> Or not. I'll wait for bug 470181 because I don't want to break SeaMonkey.

Thanks for waiting :-) There's now a fix in bug 470181 - I'll land it after you've landed your patch on 191.
Whiteboard: [1.9.1 landing waits for bug 470181 first] → [needs 1.9.1 landing]
Landed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e3f8301acef5
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: Firefox 3.2a1 → Firefox 3.1b3
verified fixed on the 1.9.1 branch using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090108 Shiretoko/3.1b3pre. Adding keyword.
You need to log in before you can comment on or make changes to this bug.