Closed Bug 282188 Opened 17 years ago Closed 16 years ago

sync xpfe toolbar.xml with toolkit toolbar.xml

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mconnor, Assigned: mnyromyr)

References

Details

Attachments

(4 files, 5 obsolete files)

 
Blocks: 282177
URL:
Summary: sync xpfe toolbar.xmll with toolkit toolbar.xml → sync xpfe toolbar.xml with toolkit toolbar.xml
Attached patch 2005.03.08 X2T diff report (obsolete) — Splinter Review
Diff from Xpfe to Toolkit version.
This makes our toolbar.xml identical to the toolkit file, and fixes the few
other things that need to be fixed to make it work (note: I didn't fix the
Modern theme).	We lose grippies with this patch - I'm only attaching it so I
don't lose it.
Okay, here we go. The diff isn't as scary as its size may suggest. ;-)
It's really mostly moving stuff around:
- make the XPFE toolbar.xml look like toolkit's
- create a new bindings directory for communicator, similar to those used for
global
- create a new toolbar.xml there that implements the XPFE toolbars as
derivations of the toolkit one's (almost all of this is a nearly verbatim copy
from the old XPFE toolbar.xml)
- make xul.css use the toolkit bindings, but:
- add CSS rules to content/communicator.css to shadow those in xul.css
  (the bare toolkit toolbars will be accessible in SeaMonkey only when
   explicitly asked for via xpfe="false" on the respective element)
- tweak both themes to use the correct styles
- for consistency, move another communicator binding file into the same
  directory; we will very likely have more stuff to put there in future
- tweak jar.mn etc.
That's all. ;-)
Many thanks to CTho for his groundwork!
Assignee: nobody → mnyromyr
Attachment #176673 - Attachment is obsolete: true
Attachment #188115 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #189223 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #189223 - Flags: review?(cbiesinger)
Comment on attachment 189223 [details] [diff] [review]
grippy-sustaining toolbar transition to toolkit, v1

for my reference: this patch also changes printPreviewBindings in addition to
moving it:
-	   onblur="navigate(0, this.value, 0);"
-	   onkeypress="if (event.keyCode==13) navigate(0, this.value, 0);"/>
+	   onchange="navigate(0, this.value, 0);"/>
on the xul:textbox for the current page.
sorry for taking so long, the patch intimidated me :-/


why can you remove the .toolbar-primary rule in classic's communicator.css?
themes/classic/communicator/communicator.css
-.toolbar-primary {
-  -moz-binding:
url("chrome://global/content/bindings/toolbar.xml#toolbar-primary");
-}

Why not add these elements to xul.css (as toolkit has them):
toolbarspacer toolbarspring toolbarpaletteitem toolbarpaletteitem[place="palette"]

Why the whitespace changes in xpfe/global/resources/content/bindings/toolbar.xml
compared to toolkit?

xul.css:
+/******* toolkit legacy layer *******/

toolkit is a legacy?


What about toolbarseparator, doesn't that also need a reference to the old
toolbar.xml? (Or does it not matter for it?)
And if it doesn't, then the [xpfe="false"] CSS rule can be removed, because it
makes no difference.

do these printPreviewBindings references need no changes?
/calendar/resources/skin/classic/calendar.css, line 1550 -- -moz-binding:
url("chrome://global/content/printPreviewBindings.xml#printpreviewtoolbar");
/calendar/resources/skin/modern/calendar.css, line 1571 -- -moz-binding:
url("chrome://global/content/printPreviewBindings.xml#printpreviewtoolbar");

printPreviewBindings extends the toolkit binding? Is that intentional?


Still need to review the toolbar.xml changes

nevermind the toolbar-primary change, I need to look at it some more
ok, toolkit-primary looks right.

since it turns out that the toolbarseparator using the toolkit version does
indeed not matter, looks like this should indeed be done:
>And if it doesn't, then the [xpfe="false"] CSS rule can be removed, because it
>makes no difference.
we should probably set some toolbarname attributes after that patch lands.

toolbar.xml does a lot of localname checks without any namespace checks :-/

how could you remove the deferAttached property?

we should also set some statusbar attributes after this patch lands, as well as
statustext ones (that's an awesome part of the patch)

The menubar no longer requires its stylesheet because it now inherits from
toolbar-base, correct?

Attachment #189223 - Flags: review?(cbiesinger) → review-
Attachment #189223 - Flags: superreview?(neil.parkwaycc.co.uk)
Attached patch /suite based, v2 (obsolete) — Splinter Review
Changes to v1:

* The new derived bindings now reside physically in /suite/common/bindings/toolbar.xml, while logically they still belong to chrome://communicator/ - thus the respective references in xpfe and calendar needed to change also. calendar was missing communicator.css.

* printPreviewBindings.xml is only moved logically in order to to match with toolbar.xml; code changes in v1 were just introduced between patch creation and review. (Should sunbird files be changed also?)

* xul.css got the missing new toolkit -moz-bindings; removed unnecessary rules and offensive ;-) comments

* whitespace changes to new toolkit toolbar.xml were mainly just LF

* deferAttached is and was (AFAICT) never used
Attachment #189223 - Attachment is obsolete: true
Attachment #203318 - Flags: review?
Attachment #203318 - Flags: review? → review?(cbiesinger)
Another comment on printPreviewBindings.xml: since this toolbar didn't show any grippies in the past, there's no need to derive it from grippytoolbar now...
Blocks: 15144
Comment on attachment 203318 [details] [diff] [review]
/suite based, v2

use a lowercase m in mozilla.org (multiple times)

suite/common/content/bindings/toolbar.xml
the license seems wrong... it seems impossible that this code is mozilla.org code from 2001, if the original code is SeaMonkey code :-)

xul.css is still missing:
toolbarbutton[orient] {
  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#menu-orient");
}
Comment on attachment 203318 [details] [diff] [review]
/suite based, v2

ok, the rest looks right.
Attachment #203318 - Flags: review?(cbiesinger) → review+
(In reply to comment #5)
> sorry for taking so long, the patch intimidated me :-/

And I have to apologize again for that, with the same reason :-(
Comment on attachment 203318 [details] [diff] [review]
/suite based, v2

I will change the licence text on check-in, given that no other changes are due to sr before...
Attachment #203318 - Flags: superreview?(jag)
Jag, I'll take the /calendar portion out of this patch, because calendar's printing preview is broken for SM anyway :-( and this patch's changes might break Sunbird. The rest of the patch won't be affected by this.
(In reply to comment #16)
> Jag, I'll take the /calendar portion out of this patch, because calendar's
> printing preview is broken for SM anyway :-( and this patch's changes might
> break Sunbird. The rest of the patch won't be affected by this.

Your patch won't affect Sunbird. Both calendar/resources/skin/classic/calendar.css and calendar/resources/skin/modern/calendar.css aren't used in Sunbird. Sunbird used its own calendar.css in calendar/sunbird/themes/[w/p]instripe/sunbird/calendar.css
Comment on attachment 203318 [details] [diff] [review]
/suite based, v2

>Index: xpfe/communicator/jar.mn
>===================================================================
>-    content/communicator/printPreviewBindings.xml        (resources/content/printPreviewBindings.xml)
>+    content/communicator/bindings/printPreviewBindings.xml (resources/content/printPreviewBindings.xml)

Be aware that bug 334936 will move all those files to suite/ - you might need to update your patch before checking this in. It may be a good idea though to move this file to the bindings/ dir when cvs copying in the other bug ;-)


>Index: xpfe/global/resources/content/bindings/toolbar.xml

I take it that this replaces the xpfe toolbar.xml with a copy of the toolkit one. Why not make jar.mn just directly pack the toolkit one and remove the xpfe one instead?


>Index: suite/Makefile.in
>Index: suite/common/Makefile.in
>Index: suite/common/jar.mn

In the mean time, common is already built ;-)

>+    content/communicator/bindings/toolbar.xml            (content/bindings/toolbar.xml)
>Index: suite/common/content/bindings/toolbar.xml

No "content/" here, we stick this directly in common/bindings/
umm, regarding printPreviewBindings.xml:
1) Why do you touch it in this bug at all?
2) Why don't we just switch over to the toolkit version per jar.mn? The diff of the two versions looks like they just did some bugfixes and removed some debug code... But anyways, I think that should go into a different bug.
Blocks: 334478
So the difference between toolbar and toolbar-primary is that the primary one will listen for pref changes and change the buttons (et al) inside to be text, image, or text + image. If I read this patch correctly we'll now force all toolbars in classic to observe this pref. Is this ok?
> So the difference between toolbar and toolbar-primary is that the primary one
> will listen for pref changes and change the buttons (et al) inside to be text,
> image, or text + image. If I read this patch correctly we'll now force all
> toolbars in classic to observe this pref. Is this ok?

Hm, this was/is neither intended nor useful (AFAIR after all this time ;-) ). Furthermore, the class "toolbar-primary" would lose most of its meaning.
I think I'll split up the grippytoolbar binding into a grippy part and derive the pref observing binding from it, tying it to the toolbar-primary class again.

Also, I think that for consistency's sake, print preview should have a grippy, too.

I'll attach an updated patch tomorrow (CEST).
Structural changes to v2:
- left printPreviewToolbar untouched, since there's an almost identical version in toolkit and we never had grippies there anyway
- made xpfe toolkit.jar to include toolkit's toolbar.xml directly (that's the 25k size reduction)
- now using correct paths under /suite
- split grippytoolbar into grippytoolbar (with the grippy xul) and grippytoolbar-primary (with the pref observer; and tied to XUL via the toolbar-primary class)

Note to self: /suite/common/bindings/toolbar.xml needs to be cvs moved over from XPFE before being altered to retain history
Attachment #203318 - Attachment is obsolete: true
Attachment #223394 - Flags: superreview?
Attachment #223394 - Flags: review+
Attachment #203318 - Flags: superreview?(jag)
Attachment #223394 - Flags: superreview? → superreview?(jag)
Comment on attachment 223394 [details] [diff] [review]
updated to trunk and comments, v3

Looks good. Though the "grippy" prefix in "grippytoolbox", "grippytoolbar", and "grippymenubar" is technically unnecessary (after all, they live in a different file) I guess it is a good reminder of what makes them different (and necessary to exist) so let's keep that.

The ../../ to reach toolkit/ looks a bit ugly, but I guess we can't do $(srcdir) or @SRCDIR@ or some such. And I suppose since the goal is to use FF's toolkit.jar at some point, this will go away, huh.

+       bindings in in the chrome://communicator/ domain that are based upon the

s/in in/in/
Attachment #223394 - Flags: superreview?(jag) → superreview+
Comment on attachment 223394 [details] [diff] [review]
updated to trunk and comments, v3

>Index: xpfe/global/jar.mn
>===================================================================
>-        content/global/bindings/toolbar.xml         (resources/content/bindings/toolbar.xml)
>+*       content/global/bindings/toolbar.xml         (../../toolkit/content/widgets/toolbar.xml)

As jag stated, ../../toolkit looks a bit ugly, just /toolkit should work and looks much better.
> just /toolkit should work

Will do on check-in.
Depends on: 341149
Checked in on trunk, with two slight modifications:
- used 
    (/toolkit/content/widgets/toolbar.xml) 
  instead of
    (../../toolkit/content/widgets/toolbar.xml)
  in xpfe/global/jar.mn
- added a comment to the now obsolete and unused file
    xpfe/global/resources/content/bindings/toolbar.xml
  that it will be removed eventually
I just don't hope every toolkit transition bug will need that long until fixed. ;-)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: helpwanted
Resolution: --- → FIXED
This regressed Txul on luna and planetoid ~1%.  nye suiterunner appears to have taken a huge Txul hit around the same time (at least 15%, maybe 35%).
(In reply to comment #27)
> This regressed Txul on luna and planetoid ~1%.  nye suiterunner appears to have
> taken a huge Txul hit around the same time (at least 15%, maybe 35%).

Understandable, as normal SeaMonkey goes through an additional level of redirection for this XBL compared to previously - and suiterunner now does load primary toolbar (including urlbar) now, which it couldn't load before that patch landed...
Removing toolbar.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/toolbar.xml,v  <--  toolbar.xml
new revision: delete; previous revision: 1.32
done
Comment on attachment 277386 [details] [diff] [review]
Make grippies work in the Error Console again (-w)

>-                xbl:inherits="collapsed,last-toolbar,orient=tborient,align=tbalign,pack=tbpack">
>+                  xbl:inherits="collapsed,last-toolbar,orient,align,pack,orient=tborient,align=tbalign,pack=tbpack">

Is there any special reason for changing indentation here?
Oops, forget that last comment, that's the -w patch of course, and the indentation change is clear from the real one.
Comment on attachment 277386 [details] [diff] [review]
Make grippies work in the Error Console again (-w)

r=me if you remove the spurious "orient,align,pack," here:

>+                  xbl:inherits="collapsed,last-toolbar,orient,align,pack,orient=tborient,align=tbalign,pack=tbpack">
                                                         ^^^^^^^^^^^^^^^^^^
Attachment #277386 - Flags: review?(mnyromyr) → review+
I copied the PersonalToolbar styles to editor and mail because they look nice.

I was toying with the idea of creating a .toolbar-secondary class to use in modern/communicator/toolbar.css instead of duplicating id styles.
Attachment #279585 - Flags: review?(mnyromyr)
It's possible that the primary/other toolbar differentiation might be made differently with toolbar customization (bug 394288), so I guess putting too much effort into those classes right now might not be ideal.

The fixes themselves are needed anyways, let's see if we can clean up the theme more when that customization stuff lands.
Beauty lies in the eye of the beholder, even more with the Modern theme. ;-)

In particular, I don't like the new grippies' look - they look very 	inhomogeneous, almost as if they'd have different Z-coordinates.
Comment on attachment 280498 [details]
editor window after and before attachment 279585 [details] [diff] [review]

Sorry, got my screenies mixed up... :-(
Attachment #280498 - Attachment description: editor window before and after attachment 279585 → editor window after and before attachment 279585
Attachment #280498 - Attachment is obsolete: true
Attachment #279585 - Flags: review?(mnyromyr) → review+
Depends on: 396565
You need to log in before you can comment on or make changes to this bug.