Closed Bug 354204 Opened 13 years ago Closed 2 years ago

Import Address Book (csv) Dialogue for matching fields: click-selecting a field for moving up/down unchecks field (click target for (un)checking should be checkbox only)

Categories

(MailNews Core :: Address Book, defect, minor)

defect
Not set
minor

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: towsonu2003, Assigned: furyinbox)

Details

(Keywords: ux-error-prevention, ux-mode-error, Whiteboard: [good first bug])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060914 Firefox/1.5.0.7 (Swiftfox)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060914 Firefox/1.5.0.7 (Swiftfox)

using thunderbird 1.5.0.7 & ubuntu 6.06

To replicate: 
1. find a csv formatted address book
2. tools > import > next > choose file & open
3. To move down "Display Name" (just an example), click on it

Behavior now:
clicking unchecks the field. so now, you have to move it, and click on it again to let thunderbird import the field. not practical at all

expected behavior: 
clicking on the "check box" only unchecks field. clicking anywhere else on the field just highlights it to allow moving it up & down

Reproducible: Always
Assignee: mscott → nobody
Confirming for: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080903030139 Shredder/3.0b1pre
This should be Platform:All. I see the same behavior on Windows and OS X.
Is this still relevant in recent Thunderbird (like 8/9) ? Could you attach a sample addressbook in CSV?
Version: unspecified → 1.5
Attached file Sample CSV.
Yes, it is still relevant.

You can export your own address book as "Comma Separated" if you need a sample CSV to test with. Attaching a sample anyway, with a single row.
I think the original idea was to have bigger click target for checking/unchecking fields, but I tend to agree with reporter that it's kinda unexpected and unhelpful that when you just select a field for moving up/down, it gets unchecked by that. For unchecking, most users would probably click on the checkbox anyway, so we don't lose much if we improve this as requested.
-> confirming

Clearly a minor issue in a rare scenario, almost RFE.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Summary: import adress book (csv) match fields: click anywhere unchecks field → Import Address Book (csv) Dialogue for matching fields: click-selecting a field for moving up/down unchecks field (click target for (un)checking should be checkbox only)
Version: 1.5 → Trunk
Not 100% sure how hard this might be, but it might be possible. Let's try "good first bug".
Whiteboard: [good first bug]
I have a mostly working solution to this. Will publish a patch soon after my vacation this week.
Hi Everyone,

Can I take this as my first 'Good first bug' if no one has started?

I'm looking for a Mentor too.

Thanks,
Phil
Assignee: nobody → philipw
No activity in seven months.
Assignee: philipw → nobody
Assignee: nobody → furyinbox
Status: NEW → ASSIGNED
Attachment #8961066 - Flags: review?(acelists)
Comment on attachment 8961066 [details]
Bug 354204 - Fix click event for address book import items.

https://reviewboard.mozilla.org/r/229820/#review238334

Thanks for the nice patch!
Finding out about needing the 'allowevents' attribute may not have been trivial :)

The patch seems to work, you can just make the syntax cleanup I write below.
Also the patch is missing the mercurial headers. If you do not use mercurial (hg) we can add those for you.
Next time please just attach the patch directly to the bug, I do not like this reviewboard.

::: mailnews/import/content/fieldMapImport.js:84
(Diff revision 2)
>  
>  function CreateField( name, index, on)
>  {
>    var item = document.createElement('listitem');
>    item.setAttribute('field-index', index);
>    item.setAttribute('type', "checkbox");

Remove this 'type' on the listitem as we aren't using this feature. We are using our own cell to watch 'checked' state instead of the automatic one this would create.

::: mailnews/import/content/fieldMapImport.js:86
(Diff revision 2)
>  {
>    var item = document.createElement('listitem');
>    item.setAttribute('field-index', index);
>    item.setAttribute('type', "checkbox");
> -  var cell = document.createElement('listcell');
> -  var cCell = document.createElement( 'listcell');
> +  item.setAttribute('allowevents', 'true');
> +

We prefer double quotes for strings now, so please use them everywhere in this function.

::: mailnews/import/content/fieldMapImport.js:89
(Diff revision 2)
>    item.setAttribute('type', "checkbox");
> -  var cell = document.createElement('listcell');
> -  var cCell = document.createElement( 'listcell');
> -  cCell.setAttribute('type', "checkbox");
> -  cCell.setAttribute( 'label', name);
> +  item.setAttribute('allowevents', 'true');
> +
> +  var secondCell = document.createElement('listcell');
> +  var checkboxCell = document.createElement('listcell');
> +  var firstCell = document.createElement('listcell');

Thanks for cleaning up the variable names.

Please create the elements in the logical order (checkbox, first, second) and group the creation with the attribute settings: c1=createElement; c1.setAttribute(1); c2=createElement(); c2.setAttribute()

::: mailnews/import/content/fieldMapImport.js:94
(Diff revision 2)
> +  var firstCell = document.createElement('listcell');
> +  checkboxCell.setAttribute('type', 'checkbox');
> +  checkboxCell.addEventListener('click', cellClicked);
> +  firstCell.setAttribute('label', name);
> +
>    if (on == true)

'if (on)' should be enough here, we pass a boolean as the argument.

::: mailnews/import/content/fieldMapImport.js:100
(Diff revision 2)
> -    cCell.setAttribute( 'checked', "true");
> -  item.appendChild( cCell);
> -  cell.setAttribute( "class", "importsampledata");
> -  cell.setAttribute( 'label', "");
> -  item.appendChild( cell);
> -  return( item);
> +    checkboxCell.setAttribute('checked', "true");
> +
> +  item.appendChild(checkboxCell);
> +  secondCell.setAttribute('class', 'importsampledata');
> +  secondCell.setAttribute('label', '');
> +  item.appendChild(firstCell);

Also append checkbox cell here as the first one.

::: mailnews/import/content/fieldMapImport.xul:45
(Diff revision 2)
>    <!-- field list -->
>    <hbox flex="1">
> -    <listbox id="fieldList" flex="1" onselect="disableMoveButtons();"
> +    <listbox id="fieldList" flex="1" onselect="disableMoveButtons();">
> -             onclick="itemClicked(event);">
>        <listcols>
> +        <listcol flex="2"/>

The checkbox does not need to expand, so flex="1".

::: mailnews/import/content/fieldMapImport.xul:51
(Diff revision 2)
>          <listcol flex="7"/>
>          <listcol flex="13"/>
>        </listcols>
>  
>        <listhead>
> +        <listheader />

We tend to use no space before />
Attachment #8961066 - Flags: review?(acelists) → review+
Product: Thunderbird → MailNews Core
Comment on attachment 8961066 [details]
Bug 354204 - Fix click event for address book import items.

https://reviewboard.mozilla.org/r/229820/#review238340

::: mailnews/import/content/fieldMapImport.js:102
(Diff revision 2)
> -  cell.setAttribute( "class", "importsampledata");
> -  cell.setAttribute( 'label', "");
> -  item.appendChild( cell);
> -  return( item);
> +  item.appendChild(checkboxCell);
> +  secondCell.setAttribute('class', 'importsampledata');
> +  secondCell.setAttribute('label', '');
> +  item.appendChild(firstCell);
> +  item.appendChild(secondCell);
> +  return(item);

No brackets needed with return.
Attached patch fix-ab-import-checkbox.patch (obsolete) — Splinter Review
thanks for the feedback! 

I've tried to address every change. Please take a look at latest patch.
Attachment #8961066 - Attachment is obsolete: true
Attachment #8964133 - Flags: review?(acelists)
Comment on attachment 8964133 [details] [diff] [review]
fix-ab-import-checkbox.patch

Review of attachment 8964133 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks great now.
You may want to separate setting up the 3 cells with empty lines for readability.

::: mailnews/import/content/fieldMapImport.js
@@ +90,5 @@
> +  firstCell.setAttribute("label", name);
> +  var secondCell = document.createElement("listcell");
> +
> +  if (on)
> +    checkboxCell.setAttribute("checked", "true");

Move this up where setting other of the checkboxCell properties.
Attachment #8964133 - Flags: review?(acelists) → review+
Comment on attachment 8964133 [details] [diff] [review]
fix-ab-import-checkbox.patch

Review of attachment 8964133 [details] [diff] [review]:
-----------------------------------------------------------------

"Drive-by" means that people will occasionally look at patches without really being involved in the bug and ask silly questions ;-) There are even unsolicited "drive-by" reviews.

Thanks for you contribution, we need volunteers from the community to report and fix little and big problems. Much appreciated.

::: mailnews/import/content/fieldMapImport.xul
@@ +47,5 @@
>          <listcol flex="13"/>
>        </listcols>
>  
>        <listhead>
> +        <listheader/>

Drive-by comment: Why is there another listheader here now?
Haha, thanks for the explanation Jorg! 

Previously we had only two columns in the listbox. The patch adds additional column (you can see it in listcols tag). As far as I understand listcols and listhead must be aligned in terms of elements number. So that's why additional empty listheader was added. It's empty because we don't need any label in header for the checkbox.
Thanks, now I see the other column. So it was a silly question ;-) - Happy Easter!
ok, now the code should look a bit better. I've moved condition to where checkbox cell configuration is happening and also separated some logic blocks with empty lines for readability. 

Please let me know if I can improve anything else.
Attachment #8964133 - Attachment is obsolete: true
Attachment #8964169 - Flags: review?(acelists)
Comment on attachment 8964169 [details] [diff] [review]
fix-ab-import-checkbox.patch

That looks very neat now and it works great. Trying it out I realised that you have a new column for the check boxes and there's an empty header now.

I'm the sheriff who will check this in and also the code manager for releases. I think this is a no/low-risk improvement that we should have in TB 60 ESR.

So assuming this will be an r+ now, I'll stick my flag onto it, too :-)
Attachment #8964169 - Flags: feedback+
Attachment #8964169 - Flags: approval-comm-beta+
Hooray! thank you!
Comment on attachment 8964169 [details] [diff] [review]
fix-ab-import-checkbox.patch

Review of attachment 8964169 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks :)
Attachment #8964169 - Flags: review?(acelists) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1cf4c920817e
Fix click event for address book import items. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
You need to log in before you can comment on or make changes to this bug.