116484 - column picker in outliner: I have to click twice to show / hide columns

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[(not reading, please use [e-mail] instead)]

column picker in outliner:  I have to click twice to show / hide columns

I'm seeing this in mail.  try to hide or show the columns using the column 
pickers.

sometimes, you have to click on the column name twice.

might be related to #116341

Comment 1

[[e-mail]]

Bug 116341 has bitrotted the patch I made for this, so you'll have to wait.

Comment 2

[[e-mail]]

Attachment: Possible patch (A)

Comment 3

[Olga]

I see it only on Linux.

Comment 4

[Oo.et.oO]

*** Bug 120416 has been marked as a duplicate of this bug. ***

Comment 5

[Oo.et.oO]

i see this in build 2002012306 (linux).
but i have to click an undetermined amount of times rapidly.

twice does NOT work.  for some reason this is only on my machine config at work.
at home i don't see this (nor a host of other probs with prefs not sticking).

Comment 6

[André Dahlqvist]

I see this to in News. It seams to be easier to AVOID it if you make sure you
stay still for around 2 seconds over your choice before you click on it, but I'm
not sure.

I'm using a CVS build from 2002-01-25-15 taken from the 0.9.8 branch.

Comment 7

[Jan Varga [:janv]]

*** Bug 118973 has been marked as a duplicate of this bug. ***

Comment 8

[Ninoschka Baca]

Note to QA: When checking this bug also go to the Address Book window to see if
its  Column Picker is also fixed.

Comment 9

[Jan Varga [:janv]]

Comment on attachment 65009 [details] [diff] [review]
Possible patch (A)

comments to the patch:
- why we have to use getElementsByTagName ?
- I thought we prefer remoceAttribute("hidden"), not setAttribute("hidden",
"false")

Comment 10

[[e-mail]]

- why we have to use getElementsByTagName ?
We don't have to use getElementsByTagName but the point was that only
outlinercol elements go in the menu, not splitters or pickers. Is
getElementsByTagName more expensive then a loop and test in JS?
- I thought we prefer removeAttribute("hidden"), not setAttribute("hidden", "false")
Doesn't persist. Used History recently? Try showing some of the hidden columns
while closing and opening the window a few times...

Comment 11

[Jan Varga [:janv]]

ok, forget removeAttribute(), but I still think getElementsByTagName() is less
efficient in this case.

Comment 12

[[e-mail]]

Attachment: change getElementsByTagName to .localName ==

Comment 13

[Jan Varga [:janv]]

Neil, I'm having problems with applying of your latest patch.

Comment 14

[[e-mail]]

Attachment: bitrot

Comment 15

[Jan Varga [:janv]]

Comment on attachment 67908 [details] [diff] [review]
bitrot

r=varga

Comment 16

[Chris Lyon]

*** Bug 127679 has been marked as a duplicate of this bug. ***

Comment 17

[(not reading, please use [e-mail] instead)]

testing this patch, it causes us to assert when you click on the column picker.

###!!! ASSERTION: getElementById(""), fix caller?: '!aId.IsEmpty()', file c:\bui
lds\bridge\mozilla\content\xul\document\src\nsXULDocument.cpp, line 3572

Comment 18

[(not reading, please use [e-mail] instead)]

adding an

if (!colid)
  return;

to the "command" event handler fixes that, but perhaps it's a problem that our 
original target has no colid, or that the this code gets fired when you click 
on the outliner picker.

Comment 19

[(not reading, please use [e-mail] instead)]

nomination for nsbeta1, since we have (almost) have a patch and this affects 
the users experience.

Comment 20

[Jan Varga [:janv]]

if (!colid)
  return;

Seems reasonable to me.

Comment 21

[[e-mail]]

Attachment: Put everything in command handler to avoid assertion

Comment 22

[Jan Varga [:janv]]

Comment on attachment 71691 [details] [diff] [review]
Put everything in command handler to avoid assertion

>+            // we no longer cache the picker content.
>+            // remove the old content
>+            while (aPopup.hasChildNodes())
>+              aPopup.removeChild(aPopup.firstChild);

what about aPopup.removeChild(aPopup.lastChild);
it should be faster, since it doesn't have to move elements internally

>-             while (currCol) {
>-               while (currCol && currCol.localName != "outlinercol")
>-                 currCol = currCol.nextSibling;
>- 
>-               if (currCol && (currCol != this)) {
>-                 // Construct an entry for each cell in the row, unless
>-                 // it is not being show
>-                 if (!currCol.hasAttribute("ignoreincolumnpicker")) {
>+            var columns = this.parentNode.childNodes;
>+            for (var i = 0; i < columns.length; i++) {
>+              // Construct an entry for each column in the row, unless
>+              // it is not being shown
>+              var currCol = columns[i];
>+              if (currCol.localName == "outlinercol" && !currCol.hasAttribute("ignoreincolumnpicker")) {

I like original while loop.
e.g.
var currCol = this.parentNode.firstChild;
while (currCol) {
  ...
  currCol = currCol.nextSibling;
}

Sorry for the nits.
Other than that r=varga

Comment 23

[[e-mail]]

Attachment: Address Jan's nits

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 71915 [details] [diff] [review]
Address Jan's nits

r=varga, sr=sspitzer

thanks for fixing this, neil.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 71915 [details] [diff] [review]
Address Jan's nits

a=asa (on behalf of drivers) for checkin to 0.9.9 and the mozilla trunk.

Comment 26

[Jan Varga [:janv]]

Neil, have you asked someone to check it in to the branch and trunk ?
If no, I can take care of it.

Comment 27

[Jan Varga [:janv]]

I'll be checking in the fix later today for Neil

Comment 28

[Jan Varga [:janv]]

Checked in on the trunk and branch.
Thanks Neil.

Comment 29

[André Dahlqvist]

Works fine with 2002-03-05-11 on Linux. VERIFIED.

Comment 30

[Olga]

Great!!

Comment 31

[Chris Lyon]

*** Bug 129040 has been marked as a duplicate of this bug. ***