1588006 - UI for adding LDAP servers broken: Add/edit panel doesn't open and textbox needs replacing

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Ben Campbell]

In the last day or two the dialog box I was using to set up an LDAP addressbook seems to have stopped working (this is doing my own builds from C-C).

Steps to reproduce:

1. Right click on account in folder view, and pick "Settings"
2. Pick Composition & Addressing
3. Under "Addressing", select "Use a different LDAP Server" radiobutton.
4. Click the "Edit Directories" button to bring up a "LDAP Directory Servers" dialog box.
5. Click the "Add" button to bring up "New LDAP Directory".

Observed:

While the labels are visible, the text-entry widgets for "Name:", "Hostname:" "Base DN" etc are not visible.

Expected:

Should be able to enter LDAP details in the fields.

There is also a more modern looking interface in the in the global preferences, which doesn't seem to work either:

Steps to reproduce:

1. Menu -> "Preferences" -> "Preferences"
2. click on "Composition" page
3. Under "Addressing", click the "Edit Directories..." button to bring up the LDAP server dialog.
4. Click "Add" button.

Observed:
Nothing happens when you click "Add".

Expected:
I expected to be prompted to enter the LDAP server details.

Comment 1

[Ben Campbell]

I'm not really up on what's happening on the UI side, but I'm guessing this is a side effect of big overhauls there (eg Bug 1521483 )...

Comment 2

[Jorg K (CEST = GMT+2)]

I get this when trying to edit a server:
NS_ERROR_ILLEGAL_VALUE: pref-editdirectories.js:144
editDirectory chrome://messenger/content/addressbook/pref-editdirectories.js:144
oncommand chrome://messenger/content/addressbook/pref-editdirectories.xul:1

https://searchfox.org/comm-central/rev/db35bf838043bf2ba0aa78d3906c0ee7d4cddacb/mailnews/addrbook/prefs/content/pref-editdirectories.js#144

That's a fallout from bug 1553804, see bug 1582941 for a similar fix. We should really have a test for this stuff.

And you can CC me on regressions in general.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1588006-ldap-panel.patch

Actually, bug 1553804 had two variants for a fix, adding chrome=no and changing the opener, like here:
https://hg.mozilla.org/mozill
EDIT: Hmm, somehow that got truncated, see comment #4 for the full text.

Comment 4

[Jorg K (CEST = GMT+2)]

Wow, BMO truncated my comment, I'll try again:

Actually, bug 1553804 had two variants for a fix, adding chrome=no and changing the opener, like here:
https://hg.mozilla.org/mozilla-central/rev/9dfd2afee1bb#l1.12

The panel now opens but looks shocking, but that's something for the de-XBL boys forgot
mailnews/addrbook/prefs/content/pref-directory-add.xul
https://searchfox.org/comm-central/search?q=xul%3Atextbox&case=false&regexp=false&path=

Comment 5

[:aceman]

Comment on attachment 9100691 [details] [diff] [review]
1588006-ldap-panel.patch

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

Thanks, the dialog opens now.

Comment 6

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/af0b4f8de1a398719748161891be9f0517d0c53f
Port bug 1553804: Use correct window opener when opening LDAP panel for add/edit. r=aceman

Comment 7

[Jorg K (CEST = GMT+2)]

Ben, if this is blocking you, change the xul:textbox to html:input, that should get you going:
https://searchfox.org/comm-central/search?q=xul%3Atextbox&case=false&regexp=false&path=pref-directory-add.xul

Comment 8

[Magnus Melin [:mkmelin]]

I'll fix those textboxes now unless someone else is already on it.

Comment 9

[Jorg K (CEST = GMT+2)]

Not me, it will be faster for someone with experience.

Comment 10

[Khushil Mistry [:khushil324]]

I can submit a patch by tonight if it's feasible. Magnus, Should I?

Comment 11

[Magnus Melin [:mkmelin]]

I'll let you review it instead.

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: bug1588006_ldap_textbox.patch

Comment 13

[Khushil Mistry [:khushil324]]

Comment on attachment 9100742 [details] [diff] [review]
bug1588006_ldap_textbox.patch

It looks good on Mac. r=khushil.

Comment 14

[Jorg K (CEST = GMT+2)]

Magnus, can you make the boxes a little bigger, the cursor is taller than the box and reaches outside. Also, the port number should be that input-inline class to avoid the padding of the number spinbuttons.

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: bug1588006_ldap_textbox.patch

Added input-inline to the port.
Wasn't needed on linux, I don't see a change. Also no problem with cursor or size, so I'll leave everything else alone.

Comment 16

[Jorg K (CEST = GMT+2)]

Added input-inline to the port. Wasn't needed on linux, I don't see a change.

Yes, that's visible on Windows only. It looks much better now, so thanks.

Richard, on Windows the cursor goes from the top border to the bottom border of the text box, in TB 68 there's some padding. Richard, can you take a look. I'll land it now, but I think some CSS needs tweaking.

Comment 17

[Richard Marti (:Paenglab)]

It's because of the input-inline class's padding: 0;. A padding: 1px 0; or padding: 1px; would fix it.

Comment 18

[Jorg K (CEST = GMT+2)]

I'm talking about the text boxes, the (inline) number boxes are fine.

Comment 19

[Richard Marti (:Paenglab)]

The text boxes have the input-inline class too. When you remove the class from the text boxes they would be taller than the number box, and this looks not good.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f8fca88b8f1e
change xul:textbox -> html:input in pref-directory-add.xul. r=khushil