Last Comment Bug 404229 - remove inclusion of global/skin/toolbar.css from browser.xul
: remove inclusion of global/skin/toolbar.css from browser.xul
Status: RESOLVED FIXED
: perf
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Dão Gottwald [:dao]
:
: Neil Deakin
Mentors:
Depends on: 466088
Blocks: 465731 479674
  Show dependency treegraph
 
Reported: 2007-11-18 04:38 PST by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2010-12-17 07:08 PST (History)
13 users (show)
rflint: in‑testsuite-
rflint: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (20.16 KB, patch)
2007-11-18 04:38 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Updated patch (4.26 KB, patch)
2008-10-06 19:17 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
gavin.sharp: review+
Details | Diff | Splinter Review
fixed patch (5.30 KB, patch)
2008-11-20 17:17 PST, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Ryan Flint [:rflint] (ping via IRC for reviews) 2007-11-18 04:38:20 PST
Created attachment 289209 [details] [diff] [review]
Patch

Bug 250867 was on the right track by making the mode styling a feature of toolbar widgets, but a bit off in that it required all consumers to include toolbar.css  to make use of these features. It would be cleaner to just make toolbarbuttons aware of what their parents are up to.
This also includes a fix to get rid of editMenuOverlay.xul's unnecessary use of the global skin.
Comment 1 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-10-06 19:17:00 PDT
Created attachment 342012 [details] [diff] [review]
Updated patch

This updates the patch to trunk and since there are no problems caused by leaving the toolbar.css declarations around, I've simply removed the changes to all other products except Firefox.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-05 17:45:38 PST
Comment on attachment 342012 [details] [diff] [review]
Updated patch

r=me, but you should at least file bugs on the other projects affected by bug 250867.
Comment 3 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-11-17 21:27:14 PST
Comment on attachment 342012 [details] [diff] [review]
Updated patch

Requesting approval for a safe, small Ts/Txul win that should land for beta 2 as it'll require minimal action (appropriately documented on devmo, of course :) from third-party themes.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-17 21:52:04 PST
Comment on attachment 342012 [details] [diff] [review]
Updated patch

a191b2=beltzner; yeah, good to get this for b2
Comment 5 Ryan Flint [:rflint] (ping via IRC for reviews) 2008-11-18 22:19:19 PST
http://hg.mozilla.org/mozilla-central/rev/2686d82a880c
Comment 6 neil@parkwaycc.co.uk 2008-11-19 01:53:17 PST
Interesting, since dao had to put

window:not([active="true"]) menubar > menu {
  color: ThreeDShadow;
}

into global.css and not menu.css so that the menu would update dynamically...
Comment 7 Dão Gottwald [:dao] 2008-11-19 05:07:39 PST
(In reply to comment #3)
> should land for beta 2 as it'll require minimal action

The display:none stuff looks like it belongs in xul.css anyway.

Comment 6 also concerns me.
Comment 8 Dão Gottwald [:dao] 2008-11-20 01:56:49 PST
(In reply to comment #6)
> Interesting, since dao had to put
> 
> window:not([active="true"]) menubar > menu {
>   color: ThreeDShadow;
> }
> 
> into global.css and not menu.css so that the menu would update dynamically...

I just got to a state where I'm stuck with icons+text, i.e., neither mode=icons nor mode=text make a difference.
Comment 9 Dão Gottwald [:dao] 2008-11-20 02:10:15 PST
backed out
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-20 06:48:03 PST
--> future; I'm not aware of a reason why we'd want to force this into 3.1
Comment 11 Dão Gottwald [:dao] 2008-11-20 17:17:14 PST
Created attachment 349324 [details] [diff] [review]
fixed patch
Comment 12 Robert Kaiser 2008-11-22 06:57:37 PST
Ugh, is there any reason why you do not remove the superfluous inclusion of global/skin in editMenuOverlay.xul in the new version? I was just about the file a bug on this when I spotted this in the logs...
Comment 13 Dão Gottwald [:dao] 2008-11-22 07:04:55 PST
(In reply to comment #12)
> Ugh, is there any reason why you do not remove the superfluous inclusion of
> global/skin in editMenuOverlay.xul in the new version?

No, I just didn't read the comments and the patch that carefully, so it got lost. I can update the patch, although filing a separate bug wouldn't be wrong anyway. (E.g. it would have stuck when the toolbar mode stuff got backed out.)
Comment 14 Robert Kaiser 2008-11-22 07:15:17 PST
Yes, I agree, it's probably better to treat that issue independently in any case, so I filed bug 466302 for it.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-21 19:50:11 PST
Comment on attachment 349324 [details] [diff] [review]
fixed patch

r=me. Looks like bug 465731 covers the thunderbird part, but it probably wouldn't hurt to file a bug on calendar to let them know that they might not need the toolbar.css reference anymore.
Comment 16 Dão Gottwald [:dao] 2009-02-22 01:16:15 PST
http://hg.mozilla.org/mozilla-central/rev/3778e80a2a4e

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