282190 - sync xpfe tree.xml with toolkit tree.xml

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[Mike Connor [:mconnor]]

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This is another one that can just be taken straight from toolkit. Only changes
made in the toolkit version is s/control/accel/ and an ifdef instead of
"!/Mac/.test(navigator.platform)".

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: Current Diff

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Patch in bug 272059

Comment 4

[Stefan [:stefanh]]

Attachment: Current diff (20060325)

The difference is not that big. Mano's patch in bug 263146 was never ported to the xpfe version. Those changes shouldn't be any problem, though - in fact, xpfe needs them ;). Then there are a few other differences.

Comment 5

[Robert Kaiser]

from what I see in a fast look over the file, we should probably just pack toolkit's version in suite and forget about real sync'ing in this case...

Comment 6

[Robert Kaiser]

Attachment: package toolkit's tree.xml into suite's toolkit.jar

I propose to fix this file once and for all (for SeaMonkey) by just using the toolkit file in suite's toolkit.jar

I've taken the conservative approach there though, leaving a non-suite xpfe build alone (wherever they still may exist) and only touching suite builds.

I propose to take this approach for the whole migration of suite widgetry to the new toolkit, as it guarantees us not to break anyone else who might still rely on the old xpfe toolkit.jar - Camino still might do that, and while this tree.xml fix would fit for them, others we switch the same way might not, but we still can add those switches into that new #ifdef block.

Comment 7

[[e-mail]]

Unfortunately there are at least two bugs with toolkit's tree.xml

The first is that the current index is not allowed to be -1
This breaks the mail thread pane

The second is that you can't accel-click rows in BeOS
Fortunately that will be fixed by bug 302174

Comment 8

[Robert Kaiser]

Neil, are those bugs fixed in our current implementation?

If yes, is there a reasonable chance to backport those fixes?

And did you point to the correct bug for the second one? If so, that's a dupe obviously...

Comment 9

[Robert Kaiser]

(In reply to comment #7)
> Unfortunately there are at least two bugs with toolkit's tree.xml
> 
> The first is that the current index is not allowed to be -1
> This breaks the mail thread pane

From what I see, Thunderbird uses the toolkit version and doesn't break...

> The second is that you can't accel-click rows in BeOS
> Fortunately that will be fixed by bug 302174

I can't follow this one, but that shouldn't block reviews for this patch (if we still want to go down that road anyways), esp. if there's a fix for it.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> Created an attachment (id=216235) [edit]
> package toolkit's tree.xml into suite's toolkit.jar
> 
> I've taken the conservative approach there though, leaving a non-suite xpfe
> build alone (wherever they still may exist) and only touching suite builds.
> 
> I propose to take this approach for the whole migration of suite widgetry to
> the new toolkit, as it guarantees us not to break anyone else who might still
> rely on the old xpfe toolkit.jar - Camino still might do that, and while this
> tree.xml fix would fit for them, others we switch the same way might not, but
> we still can add those switches into that new #ifdef block.

On <http://wiki.mozilla.org/SeaMonkey:Toolkit_Transition#Status_of_each_widget>,
I read "Camino just has embed.jar and pipnss.jar, so no problems here.";
then, maybe the if switch is not needed ?
(Though I don't know either if there is any other "non-suite xpfe build")

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> On
> <http://wiki.mozilla.org/SeaMonkey:Toolkit_Transition#Status_of_each_widget>,

(Rather <http://wiki.mozilla.org/SeaMonkey:Toolkit_Transition#XUL_Widgets>.)

Comment 12

[Karsten Düsterloh]

> > The first is that the current index is not allowed to be -1
> > This breaks the mail thread pane
> 
> From what I see, Thunderbird uses the toolkit version and doesn't break...

The problem is that the toolkit tree does not allow the tree to have the focus without also having a row selected; it will automatically select the first row in this case:
-      <handler event="focus" action="this.treeBoxObject.focused = true;"/>
+      <handler event="focus">
+        <![CDATA[
+          this.treeBoxObject.focused = true;
+          if (this.currentIndex == -1 && this.view.rowCount > 0) {
+            this.currentIndex = this.treeBoxObject.getFirstVisibleRow();
+          }
+        ]]>
+      </handler>

Neil, what do mean by "break"?

Comment 13

[Karsten Düsterloh]

Comment on attachment 216235 [details] [diff] [review]
package toolkit's tree.xml into suite's toolkit.jar

>Index: mozilla/xpfe/global/jar.mn
>===================================================================
>         content/global/bindings/toolbarbutton.xml   (resources/content/bindings/toolbarbutton.xml)
>-        content/global/bindings/tree.xml            (resources/content/bindings/tree.xml)
>         content/global/bindings/wizard.xml          (resources/content/bindings/wizard.xml)
>+#ifdef MOZ_SUITE
>+*+      content/global/bindings/tree.xml            (/toolkit/content/widgets/tree.xml)
>+#else
>+        content/global/bindings/tree.xml            (resources/content/bindings/tree.xml)
>+#endif

The ifdef isn't necessary, not even Camino is using this. 
And you should keep the alphabetical order of the bindings. ;-)

r=me with that

Comment 14

[[e-mail]]

(In reply to comment #12)
>Neil, what do mean by "break"?
Bug 305140. Bug 304565. Bug 304676 comment 5. Bug 302516 comment 14.

Comment 15

[Karsten Düsterloh]

Setting open toolkit tree.xml problems as blocking.

Comment 16

[Robert Kaiser]

I don't think there's anything we still need to do here, get rid of this bug.