137530 - Edit Cipher dialog does not work

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

A couple of days ago, I checked in the patch from bug 102633 to the trunk, which
is a rewrite of the cipher preferences dialog.

The implementation uses a <listbox> with multiple columns, where one columns has
a checkbox in each row.

At the time I wrote the patch, the dialog worked correctly, it was possible to
click on lines in the listbox, and that toggled the checkbox of the clicked line.

Today I saw that this does not work anymore. I must confess that I'm unsure
whether it worked on the time I checked it in, I assumed my patch does still
work, but it worked for sure a couple of days before.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch for discussion

I saw that Edit/Prefs/Advanced/Scripts uses a list of checkboxes, too.
I didn't find any other place with a similar UI.
However, that list uses only a single column.

As I don't understand why this doesn't work anymore, I though it is best to
rewrite my dialog to something simpler, and more similar to that other working
UI.

The attached patch changes the code to a single column listbox.
But still, this patch does not produce the expected result.

As the list is long, more than do fit on screen, the user has to scroll to see
all items.

However, click-to-toggle seems to work only for the first few items in the
list, I think only for those, that are initially visible.

In addition, setting the default value from JS also does not work for all the
checkbox items in the list. Only the first few items show the correct
checked-or-not state. All items that are not initially visible are always
unchecked.

Note that the patch has code to potentially disable a listitem checkbox. But I
traced to ensure that none of my preferences are locked, and the disable
statement is never reached.

Expected behaviour:
Either:
- the existing dialog as checked in on the trunk should work

Or, if checkboxes in multi-column-listboxes are not supported by our XUL
implementation

- the code including the attached patch should produce a dialog, where all
checkboxes receive their default values from JS and can be toggled by the user.

Comment 2

[Kai Engert [:KaiE:]]

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

Comment 3

[Kai Engert [:KaiE:]]

Joe, doe you have an idea for a solution

Should we back out my patch from 102633 as long as we don't have a solution?

Comment 4

[John Unruh]

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

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Suggested Fix

Jan Varga was so kind and helped me on irc. All that's required is attribute
allowevents="true" on the listem.

Comment 6

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 7

[Javier Delgadillo]

Comment on attachment 82623 [details] [diff] [review]
Suggested Fix

r=javi

Comment 8

[Kai Engert [:KaiE:]]

-> me

Comment 9

[Joe Hewitt (gone)]

Comment on attachment 82623 [details] [diff] [review]
Suggested Fix

sr=hewitt

Comment 10

[Kai Engert [:KaiE:]]

Checked in to trunk, marking fixed.

John, when you verify this patch, I'd like to ask you to test this on all
platforms. This is a new kind of UI that seems to be used nowhere else in Mozilla.

The bug is verified, if you can toggle the checkboxes in the cipher dialog.
Thanks.

Comment 11

[Georg Wild]

Well, on linux I can now check this options,... but I now can't mark one topic
so that I can read the explanation at the botton for each cipher!

Comment 12

[Georg Wild]

Well, sometimes I can mark? Just test!

Comment 13

[Kai Engert [:KaiE:]]

I don't know how to fix the problem.

I need help from XUL experts.

If checkboxes in listboxes do not work as expected, and nobody is able to help,
I only have one idea left:

Instead of embedding the checkboxes in the list, we could place the checkbox
below the listbox, and make the listbox a select-and-read only. We could display
the word "enabled" or "disables" in the listbox, and the checkbox below that
list would be used to toggle that cipher.

Comment 14

[Kai Engert [:KaiE:]]

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

Comment 15

[Javier Delgadillo]

r=javi for backout.

Comment 16

[Kai Engert [:KaiE:]]

I backed everything out, both the fix attempt and the patches from bug 102633.

Comment 17

[John Unruh]

V