410755 - [Trunk] It is not possible to resize and reorder the columns of the task list

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Christian Jansen]

- Go to the task mode, or open the today pane
- hover with the mouse over a column separator of the task list
- try to reorder respectively resize the a column.

It should be possible to reorder and resize the columns in a way of the Inbox.

Comment 1

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.12pre) Gecko/20080103 Calendar/0.8pre

Works for me as long as the task list is empty. Although it's working the following error is displayed:

Error: item has no properties
Source File: chrome://calendar/content/calendar-task-tree.xml Line: 1052

As soon as a task is displayed in the list resize and reorder is broken. But in this case no error is displayed.

This used to work in previous Sunbird builds -> regression.

Comment 2

[Stefan Sitter [:ssitter]]

Regression range:
 Works using Sunbird 0.8pre (2007-12-14-03)
 Fails using Sunbird 0.8pre (2007-12-15-05)
 
Checkins during regression range:
 http://tinyurl.com/2jjboz  -> Bug 388018, Bug 405111

Comment 3

[Sebastian Schwieger]

Attachment: [checked in] patch

The draggestures where initialized even on the column headers. The patch checks if an item is dragged.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 295364 [details] [diff] [review]
[checked in] patch

r=philipp

Comment 5

[Martin Schröder [:mschroeder]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 6

[Sven Grull]

Trunk build 2008010423
Resizing of the columns is still not possible, reordering is now possible but is not saved after closing Sunbird.

Comment 7

[Sebastian Schwieger]

(In reply to comment #6)
This seems to be a trunk-only problem, now. Task is fixed on Branch (Lightning 0.8pre 20080105). Reopening based on comment 6.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

If this is fixed on branch, then we should remove the blocking state.

Comment 9

[Stefan Sitter [:ssitter]]

Based on the recent post from Prasad Sunkari in mozilla.dev.apps.calendar and the comment on http://developer.mozilla.org/en/docs/getElementsByTagName this only works "by accident" on the MOZILLA_1_8_BRANCH because of Bug 206053.

In the branch builds e.g. getElementsByTagName("treecol") will also return the "xul:treecol" elements. On Trunk Bug 206053 is fixed and nothing is returned.

The solution is too use getElementsByTagNameNS() instead.

Comment 10

[Stefan Sitter [:ssitter]]

Attachment: use getElementsByTagNameNS()

Comment 11

[Prasad Sunkari [:prasad]]

While the change to getElementsByTagNameNS helps us store and restore the visibile columns, there could still be a problem with the splitters between the column headers.

We might have to explicitly set ordinals for the splitters too.  It's pretty easy, just loop over the iterators and set the ordinals (0, 2, 4, 6 ...)

Also, in the above patch using this.namespaceURI, could be better than using "*" for the namespace.

Comment 12

[Prasad Sunkari [:prasad]]

Attachment: Uses getElementsByTagnameNS and sets Splitter Ordinals

This patch uses getElementsByTagNameNS instead of getElementsByTagName.  It also explicitly sets ordinals to the tree splitters.

Not obsoleting the patch by Stefan Sitter (That can probably be done after the review)

Comment 13

[Stefan Sitter [:ssitter]]

getElementsByTagNameNS("*" was already used in the calendar code, therefore I assumed it's correct usage. But looking at other exmaples in CVS I think e.g.
  - var treechildren = tree.getElementsByTagName("treechildren")[0];
  + var treechildren = tree.getElementsByTagNameNS(this.namespaceURI,
should be
  + var treechildren = tree.getElementsByTagNameNS(tree.namespaceURI,

Comment 14

[Prasad Sunkari [:prasad]]

(In reply to comment #13)
> getElementsByTagNameNS("*" was already used in the calendar code, therefore I
> assumed it's correct usage. But looking at other exmaples in CVS I think e.g.
>   - var treechildren = tree.getElementsByTagName("treechildren")[0];
>   + var treechildren = tree.getElementsByTagNameNS(this.namespaceURI,
> should be
>   + var treechildren = tree.getElementsByTagNameNS(tree.namespaceURI,
> 

Yes, tree.namespaceURI is definitely better to be used.  However, in the context of this file, there are places where we are directly calling getElementsByTagNameNS on document.getAnonymousNodes(this.binding)[0], in which cases the 'tree' is not always defined.

Well, if you really think that we should change it, I have no problem with it :)

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Which of these patches need review? Or do you guys want to post a new patch that takes care of the latest comments? I'd like to clean out my review queue so please obsolete any patches that are obsolete :-)

Comment 16

[Stefan Sitter [:ssitter]]

(In reply to comment #14)

I don't meant to say that tree.namespaceURI must be used in all places. I did understand that namespaceURI should be retrieved from the object you call getElementsByTagNameNS() on.

Example: 
+ var treecols = document.getAnonymousNodes(this)[0]
+                        .getElementsByTagNameNS(this.namespaceURI, "treecol");

I can't say if "document.getAnonymousNodes(this)[0]" is the same as "this". In this case this.namespaceURI might be correct, I don't know.

Comment 17

[Prasad Sunkari [:prasad]]

> 
> I don't meant to say that tree.namespaceURI must be used in all places. I did
> understand that namespaceURI should be retrieved from the object you call
> getElementsByTagNameNS() on.

What I meant was that there are places where we use getAnonymousNodes(this) where we can't really take the objects namespace directly.

> 
> Example: 
> + var treecols = document.getAnonymousNodes(this)[0]
> +                        .getElementsByTagNameNS(this.namespaceURI, "treecol");

Where ever this is possible I will change it to use the parent object.  In other places will save document.getAnonymousNodes(this)[0] to a variable and use it.  That should be clean.

> 
> I can't say if "document.getAnonymousNodes(this)[0]" is the same as "this". In
> this case this.namespaceURI might be correct, I don't know.
> 

This is not same as document.getAnonymousNodes(this)[0].

Comment 18

[Prasad Sunkari [:prasad]]

Attachment: Uses getElementsByTagnameNS and sets Splitter Ordinals (Version 2)

updates as per the comments above by Simon.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 309093 [details] [diff] [review]
Uses getElementsByTagnameNS and sets Splitter Ordinals (Version 2)


>+            var tree = document.getAnonymousElementByAttribute( this.binding, 
>+                                                                "anonid", 
>+                                                                "calendar-task-tree");
Remove space after bracket, realign.

>+            var treechildren = tree.getElementsByTagNameNS(tree.namespaceURI, 
>+                                                           "treechildren")[0];
>             
>             if (!this._getItemFromEvent(event)) {
>                 tree.view.selection.clearSelection();
>             }
>           },
treechildren isn't used here, can be removed?

>+        // Set the ordinals for splitters - does not work on 1.9 without this.
>+        var treesplitters = tree.getElementsByTagNameNS(tree.namespaceURI, "splitter");
>+        for (var i = 0; i < treesplitters.length; i++) 
>+          treesplitters[i].ordinal = 2 * (i+1);
>+
Why is this done in the constructor? Can't it be done on the content itself?


r=philipp
Please supply a new patch with comments processed for checkin.
Sorry I haven't tested, but given this works on branch, can we take it for 0.8?

Comment 20

[Simon Paquet [:sipaq]]

(In reply to comment #19)
> Sorry I haven't tested, but given this works on branch, can we take it for 0.8?

Given that every additional patch introduces additional risk and that we're in release mode right now, I would strongly suggest to take this in after 0.8 is out.

Comment 21

[Prasad Sunkari [:prasad]]

Attachment: Uses getElementsByTagnameNS and sets Splitter Ordinals - Corrected as per Phillips Comments

Updated the patch as per Philipp's comments.
r=philipp

Can someone help me check this patch in?

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Reviewer normally takes care of checkin if patch writer can't. I watch checkin-needed bugs once in a while, in case I forget to check it in :-)

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 24

[Andreas Treumann]

Checked with thunderbird 2008070303 and lightning 2008070221 -> VERIFIED.