Closed Bug 736661 Opened 8 years ago Closed 7 years ago

outgoing server (SMTP) cannot be highlighted even though selected

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: derek, Assigned: neil)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

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)
Component: General → Mail Window Front End
QA Contact: general → front-end
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.
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → Account Manager
Ever confirmed: true
OS: Mac OS X → All
Product: Thunderbird → MailNews Core
QA Contact: front-end → account-manager
Hardware: x86 → All
Summary: outgoing server cannot be highlighted even though selected → outgoing server (SMTP) cannot be highlighted even though selected
Version: 10 → Trunk
Yes, I highlight one higher up the list and then scroll down with the arrow key.
The selection is only missing on the server that has the (default) suffix regardless of its position. Derek, can you confirm that?
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?
Blocks: 793819
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
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.
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) { }
Attached patch Possible patchSplinter Review
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).
Attachment #666995 - Flags: ui-review?(bwinton)
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.
Blocks: 791948, 769198
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 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.
Attachment #666995 - Flags: feedback+
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.
(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.
I mean you to use the timeout only for the selectItem and drop the ensureElementIsVisible if needed.
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.
Blocks: 367347
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.
Attachment #666995 - Flags: ui-review?(bwinton) → ui-review+
Attachment #666995 - Flags: review?(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...
Attachment #666995 - Flags: review?(mconley) → review+
Assignee: nobody → neil
Pushed comm-central changeset 366b42862635.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.