Closed Bug 282190 Opened 17 years ago Closed 15 years ago

sync xpfe tree.xml with toolkit tree.xml

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla1.9alpha1

People

(Reporter: mconnor, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: [bug 272059 should fix most/all of this bug :-)])

Attachments

(2 files, 1 obsolete file)

 
Blocks: 282177
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)".
URL:
Depends on: 272059
Whiteboard: [bug 272059 should fix most/all of this bug :-)]
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.
Attachment #175223 - Attachment is obsolete: true
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...
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.
Attachment #216235 - Flags: superreview?(neil)
Attachment #216235 - Flags: review?(mnyromyr)
Attachment #216231 - Attachment description: Current diff (2006060325) → Current diff (20060325)
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
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...
(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.
(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")
Depends on: 263146
Target Milestone: --- → mozilla1.9alpha
> > 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 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
Attachment #216235 - Flags: review?(mnyromyr) → review+
(In reply to comment #12)
>Neil, what do mean by "break"?
Bug 305140. Bug 304565. Bug 304676 comment 5. Bug 302516 comment 14.
Setting open toolkit tree.xml problems as blocking.
Depends on: 304676, 305140
I don't think there's anything we still need to do here, get rid of this bug.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Attachment #216235 - Flags: superreview?(neil)
You need to log in before you can comment on or make changes to this bug.