282188 - sync xpfe toolbar.xml with toolkit toolbar.xml

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mike Connor [:mconnor]]

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: 2005.03.08 X2T diff report

Diff from Xpfe to Toolkit version.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch to switch to toolkit toolbar

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.

Comment 3

[Karsten Düsterloh]

Attachment: grippy-sustaining toolbar transition to toolkit, v1

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!

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

nevermind the toolbar-primary change, I need to look at it some more

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

http://developer.mozilla.org/en/docs/XUL:Toolbars might be helpful to reviewers
of this patch

Comment 10

[Karsten Düsterloh]

Attachment: /suite based, v2

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

Comment 11

[Karsten Düsterloh]

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...

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

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 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 203318 [details] [diff] [review]
/suite based, v2

ok, the rest looks right.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

(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 15

[Karsten Düsterloh]

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...

Comment 16

[Karsten Düsterloh]

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.

Comment 17

[Simon Paquet [:sipaq]]

(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 18

[Robert Kaiser]

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/

Comment 19

[Robert Kaiser]

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.

Comment 20

[jag (Peter Annema)]

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?

Comment 21

[Karsten Düsterloh]

> 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).

Comment 22

[Karsten Düsterloh]

Attachment: updated to trunk and comments, v3

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

Comment 23

[jag (Peter Annema)]

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/

Comment 24

[Robert Kaiser]

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.

Comment 25

[Karsten Düsterloh]

> just /toolkit should work

Will do on check-in.

Comment 26

[Karsten Düsterloh]

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. ;-)

Comment 27

[Andrew Schultz]

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%).

Comment 28

[Robert Kaiser]

(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...

Comment 29

[Karsten Düsterloh]

Removing toolbar.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/toolbar.xml,v  <--  toolbar.xml
new revision: delete; previous revision: 1.32
done

Comment 30

[[e-mail]]

Attachment: Make grippies work in the Error Console again

Comment 31

[[e-mail]]

Attachment: Make grippies work in the Error Console again (-w)

Comment 32

[Robert Kaiser]

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?

Comment 33

[Robert Kaiser]

Oops, forget that last comment, that's the -w patch of course, and the indentation change is clear from the real one.

Comment 34

[Karsten Düsterloh]

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">
                                                         ^^^^^^^^^^^^^^^^^^

Comment 35

[[e-mail]]

Attachment: Modern theme fixes

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.

Comment 36

[Robert Kaiser]

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.

Comment 37

[Karsten Düsterloh]

Attachment: editor window after and before attachment 279585

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 38

[Karsten Düsterloh]

Comment on attachment 280498 [details]
editor window after and before attachment 279585 [details] [diff] [review]

Sorry, got my screenies mixed up... :-(