Closed Bug 404229 Opened 13 years ago Closed 12 years ago

remove inclusion of global/skin/toolbar.css from browser.xul

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: rflint, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #289209 - Flags: review?(gavin.sharp)
Version: unspecified → Trunk
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #289209 - Attachment is obsolete: true
Attachment #342012 - Flags: review?(gavin.sharp)
Attachment #289209 - Flags: review?(gavin.sharp)
Attachment #342012 - Flags: review?(gavin.sharp) → review?(mano)
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.
Attachment #342012 - Flags: review?(mano) → review+
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.
Attachment #342012 - Flags: approval1.9.1b2?
Comment on attachment 342012 [details] [diff] [review]
Updated patch

a191b2=beltzner; yeah, good to get this for b2
Attachment #342012 - Flags: approval1.9.1b2? → approval1.9.1b2+
http://hg.mozilla.org/mozilla-central/rev/2686d82a880c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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...
(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.
(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.
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
--> future; I'm not aware of a reason why we'd want to force this into 3.1
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Target Milestone: mozilla1.9.1b2 → Future
Blocks: 465731
Attached patch fixed patchSplinter Review
Attachment #349324 - Flags: review?
Attachment #349324 - Flags: review? → review?(rflint)
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...
(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.)
Yes, I agree, it's probably better to treat that issue independently in any case, so I filed bug 466302 for it.
Depends on: 466088
Attachment #349324 - Flags: review?(rflint) → review?(gavin.sharp)
Summary: Make toolbarbuttons aware of toolbar mode → remove inclusion of global/skin/toolbar.css from browser.xul
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.
Attachment #349324 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/3778e80a2a4e
Assignee: rflint → dao
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9.2a1
Blocks: 479674
Attachment #342012 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.