736661 - outgoing server (SMTP) cannot be highlighted even though selected

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Derek Williams]

Attachment: Screen Shot 2012-03-17 at 00.12.44.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120310173008

Steps to reproduce:

Selected an outgoing server by clicking a selectable one then scrolling down to one that was not selectable
I then set as Default


Actual results:

the desired server at  the bottom of the server list could not be selected unless I selected a selectable one and then scrolled down, but even then it did not highlight in blue


Expected results:

the selected server should be highlighted blue, and it should be the server chosen when set as Default

It should be possible to select any server by clicking on it
Once selected, any message should direct out through that server, not the previously selected one (2nd attachment shows that the current message is still attempting to exit via the previously selected outgoing server)

Comment 1

[Derek Williams]

Attachment: currently open message still trying to go from no longer default server

Comment 2

[:aceman]

Can you go to the wished server using arrow keys?
It looks like I can reproduce this on Win XP TB 14. I added some new SMTP servers and the last one can't be selected by mouse and does not get the blue highlight. But it is selectable by keys (still doesn't get highlight).

It look very similar to bug 549623 in filter list.

May be some general problem with the list widget but on all platforms.
Roc, can you please make some widget people look at this? I am not sure the Filter or Account manager people can help with this.

Comment 3

[Derek Williams]

Yes, I highlight one higher up the list and then scroll down with the arrow key.

Comment 4

[:aceman]

The selection is only missing on the server that has the (default) suffix regardless of its position. Derek, can you confirm that?

Comment 5

[:aceman]

I have analyzed the behaviour via DOM inspector and it looks like there is this problem (for the toolkit guys, the code is at http://hg.mozilla.org/comm-central/file/95ecbcca7df6/mailnews/base/prefs/content/am-smtp.js):
1. the default server (listitem) has the "default" attribute set to "true".
2. once clicked or moved to by keys it does not get the proper highlight, because it does not get the "selected" and "current" attributes set to "true".

I don't know why that is. When I used default=true in the identities list dialog (also using listitems http://hg.mozilla.org/comm-central/file/95ecbcca7df6/mailnews/base/prefs/content/am-identities-list.js) it worked fine.

Is this a toolkit widget problem or mailnews one? Neil, Gavin, can you please look at it?

Comment 6

[Derek Williams]

aceman

Here is what happened (TB 15.0.1):
1. Open Outgoing Server dialog
2. Default server is correct but not showing as highlighted
3. Click any server and then scroll down so one below the scrollbars window is highlighted
4. The server remains highlighted (didn't use to)
5. Click Set Default
6. The default server unhighlights, and cannot be rehighlighted (if in bottom of list), however others either side of it can
7. Select another server in the top of the list
8. Click Set Default
9. The default is set, and shows correctly AND the server can be highlighted even though it is default

Comment 7

[:aceman]

Thanks, confirming what Derek says.

The default account is not highlighted on selection only when it is below the first screenful of items, i.e. at the 7th position or below (1-based). I also noticed in the DOM Inspector that the some listitems do not get the proper anonymous (red) elements created (like xul:listcell, xul:label) as childs of them so can't be expanded in the inspector.

Comment 8

[[e-mail]]

Indeed, XBL is getting confused, although in my case I had to add 10 SMTP servers to get the problem to appear; adding an 11th server resulted in an "unselectable" server, although only the most recently added or defaulted server after the 10th is "unselectable".

In fact this happens to all of our listboxes. For instance, it happens to the listbox in the profile manager. SeaMonkey's profile manager "solved" the problem by switching from a listbox to a tree, which doesn't have the XBL issue, but it would e.g. make styling the default item harder.

By comparison, Toolkit's profile manager "solved" the problem by showing the relevant item on a timeout, which then allows it to be selected.

try {
  if (profile === gProfileService.selectedProfile) {
    setTimeout(function(a) {
      profilesElement.ensureElementIsVisible(a);
      profilesElement.selectItem(a);
    }, 0, listitem);
  }
}
catch(e) { }

Comment 9

[[e-mail]]

Attachment: Possible patch

Note that this patch has the additional advantage of scrolling the selected or default server into view; previously the list always scrolled back to the top after a server was added (I didn't try editing a server).

Comment 10

[:aceman]

So is it better to do this workaround than to fix the XBL (maybe nobody knows the cause)?

Thanks for the scrolling, I'll do the rest of the actions in bug 791948.

Also I'll check if this workaround can be applied in bug 769198.

Comment 11

[[e-mail]]

It's due to a subtle interaction between xbl and the listbox body frame, which (as a performance measure) tries really hard to ignore listitems that aren't in view, including not forcing xbl to bind to them. (Ordinary hidden items do have xbl bound to them.)

Comment 12

[:aceman]

Comment on attachment 666995 [details] [diff] [review]
Possible patch

The patch fixes the problem for me. However I think it flickers a bit, probably moving scroll to top, draw, move scroll to selected item. Maybe aServerList.ensureElementIsVisible() could be called directly without the timer. I'll try it out.

Comment 13

[:aceman]

Neil, if you can't fix the flickering and bwinton does not like it, then just drop the scrolling and I'll try to rework the list rebuilding as in bug 780473, maybe that will help.

Comment 14

[[e-mail]]

(In reply to aceman from comment #13)
> Neil, if you can't fix the flickering and bwinton does not like it, then
> just drop the scrolling and I'll try to rework the list rebuilding as in bug
> 780473, maybe that will help.
That won't help when building the list for the first time; the timeout is definitely needed then. If it's a real problem then a hacky way around it is to pass a "use timeout" flag when building the list.

Comment 15

[:aceman]

I mean you to use the timeout only for the selectItem and drop the ensureElementIsVisible if needed.

Comment 16

[[e-mail]]

No, the point is that the listitem needs to be in view to be selectable. (Alternatively you can create the listitem in C++, but that means using RDF, which apparently everyone hates, even though removing it creates more problems than it solves.) And you need the timeout to allow the listbox a chance to calculate its row height so that you actually scroll to the right place.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 666995 [details] [diff] [review]
Possible patch

I think this seems reasonable, although it would be _way_ easier for me to check the behaviour if we had a MozMill test...  I'm just sayin'.

Later,
Blake.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 666995 [details] [diff] [review]
Possible patch

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

Yep, this looks OK to me. I agree, a test would be lovely...

Comment 19

[[e-mail]]

Pushed comm-central changeset 366b42862635.