16821 - [DOGFOOD][REGRESSION] pre-choose identity based on folder pane selection not working

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

this feature broke.  it's causing other bugs, see #16780

the current (broken) behaviour is the first identity always gets chosen.

I'm looking into it, as I implemented the feature.

Comment 1

[(not reading, please use [e-mail] instead)]

accepting

adding dependency

Comment 2

[(not reading, please use [e-mail] instead)]

the bug for the original feature was #10235

lisa, should this be marked a dup of 10235, and should 10235 be reopened?

Comment 3

[lchiang]

To make less confusion, let's just leave 10235 alone and use this one to track
the new failure.  Thanks.

Comment 4

[Ninoschka Baca]

Setting QA Contact to nbaca, adding lchiang to Cc: list.

Comment 5

[(not reading, please use [e-mail] instead)]

fix in hand.  the problem was caused by me.  I depricated the
FindServerUsingURI() method, and removed all C++ calls to it, but forgot a JS
call to it.

I'm finding the server (using the uri) in commandglue.js a different way, and now
it works.

fix in hand, awaiting review and then I'll check it in.

Comment 6

[(not reading, please use [e-mail] instead)]

fixed.

Comment 7

[lchiang]

Suresh, pls mark verified once you finish checking the respin builds. Thanks.

Comment 8

[Ninoschka Baca]

Build 1999110808M11: Win32/NT4, Linux/Redhat 6.0, Mac 8.5.1
Verified Fixed. Since bug# 10235 has been Verified Fixed and its description is
the same as this one, I am verifying this bug.

Comment 9

[Phil Peterson]

Not working for me on NT. Compose window always seems to default to <phil>rather
than my mail identity or news identity.

Comment 10

[Ninoschka Baca]

Build 1999111017M11: Win32/NT4, Linux/Redhat 5.0, Mac 8.5.1

- This was working with the 11/08 builds.
- I just checked the 11/09 builds and this is where the regression appears.
- It is also a problem in todays build, 11/10. Compose appears to be choosing
the identity from my first mail account.

Comment 11

[(not reading, please use [e-mail] instead)]

this is the bug that will not die.

I kill it every couple of weeks and it comes back like a zombie.

die bug! die!

accepting.

Comment 12

[(not reading, please use [e-mail] instead)]

according to alecf, the html select widget is "having problems" today.

I'm still going to look into this bug, I'm hoping it becomes a duplicate of html
select widget bug.

Comment 13

[leger]

Putting on the PDT+ radar.

Comment 14

[Ninoschka Baca]

Build 1999111017M11: NT4, Linux, Mac

Using multiple accounts I select a folder in the qatest20 account, select "New
Msg" and noticed that the From: field shows qatest22 (which is wrong, qatest22
is the first mail account listed). I sent the message and after receiving
the message noticed that the header states that it is from qatest20, which is
correct.

Comment 15

[(not reading, please use [e-mail] instead)]

adding rods and kmcclusk to the cc list.

one of them is the HTML select guru.

rod / kevin:  we have a problem where the html select isn't showing the option
that is actually selected.  is this a dup?

Comment 16

[Kevin McCluskey (gone)]

Re-assigning to rod. This is probably a dup of a known problem.

Comment 17

[(not reading, please use [e-mail] instead)]

adding phil to the cc list.

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: here's the simple xul to reproduce.

Comment 19

[rods (gone)]

look in CloseContainer of nsHTMLContentSink

Comment 20

[Chris Waterson]

rods: does this need to happen after the initial reflow? I notice that
nsHTMLSelectElement::DoneAddingContent() goes off and looks for frames n'
stuff. XUL builds the entire content model before it does an initial reflow.

Seems like the <select> needs to be smart enough to handle that case, no?
For example, you'll need to handle this case so that somebody can do a
'document.write()' of a <select> element. Or uses the L1 DOM APIs to add an
entire <select> tree, complete with <option> children?

Comment 21

[Chris Waterson]

Let me try to say that a bit more clearly.

1. In HTML, the initial reflow is done very early on. So your trick of using
the content sink to notify you when the <select> is closed is fine.

2. In XUL, the -entire- content model is constructed before initial reflow. So,
calling DoneAddingContent() during content model construction will be -almost-
be a no-op. There will be no frames there to initialize.

3. This case is isomorphic to the case where, through JS and DOM Level 1 APIs,
a user constructs and initializes a <select> element, complete with <option>
elements, and then adds the subtree en masse into the content model. I would
think you need to handle that case.

--

Note that in (2), I said "almost a no-op". I notice that you do store some
state in the nsHTMLSelectElement; namely, you set mIsDoneAddingContent to true.
So, if that's enough to fix the bug, I can certainly hack the XUL content
construction to ensure this gets called; however, I think this bug will come
back to bite you when somebody does what I describe in (3). Which seems to
indicate that we need to rethink how this part of the <select> is architected.

I'll go see if all nsHTMLSelectElement really cares about is the
mIsDoneAddingContent flag, in which case we can hack it for M11.

Comment 22

[Chris Waterson]

Ok, as I'd feared, calling DoneAddingContent() before the frames are
constructed ends up being a Very Bad Thing. The <select> doesn't even show up.

Comment 23

[Chris Waterson]

Attachment: HTML test case

Comment 24

[Chris Waterson]

rods: i've just attached an HTML test case that illustrates how badly broken
selects are via Level 1 DOM APIs.

1. Constructing the <option> and setting the 'selected' attribute via
setAttribute() doesn't work.

2. Constructing the <option> and setting "option.seleted = true" doesn't work.

3. Setting "option.selected = true" even -after- the option is added into the
content model doesn't work.

Reassigning back to you. This is not a problem with XUL. It is a problem with
<select>.

Comment 25

[rods (gone)]

reassigning to waterson to fix the XULDocument

Comment 26

[Chris Waterson]

rods: we need to talk. I don't understand why I need to do this. If a <select>
is created via DOM L1 document.createElement() API, then -when on earth- is
DoneAddingContent() ever called? Won't that case break also?

Furthermore, I -did- try adding a call to DoneAddingContent(), but it didn't
work. It seems that you expect the frame to already exist by that point, which
it doesn't.

Comment 27

[rods (gone)]

Send me a diff of your fix so I can play with it.

Comment 28

[rods (gone)]

Here is the diff that fixes the XUL's select:

Index: nsXULDocument.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/content/src/nsXULDocument.cpp,v
retrieving revision 1.188
diff -r1.188 nsXULDocument.cpp
117a118
> #include "nsISelectElement.h"
4739a4741,4745
>             nsresult srv;
>             nsCOMPtr<nsISelectElement> select = do_QueryInterface(element,
&srv);
>             if (NS_SUCCEEDED(srv)) {
>               srv = select->DoneAddingContent();
>             }

Comment 29

[Chris Waterson]

ok, if that works, check it in.

Comment 30

[Chris Waterson]

rods fixed this.

Comment 31

[Ninoschka Baca]

Build 1999111521M11: Win32/NT4
Build 1999111520M11: Linux/Redhat 6.0
Build 1999111520M11: Mac 8.5.1
Verified Fixed on all platforms. Select an account, select "New Msg" and the
Compose window is displaying the correct identity whether it's a POP, IMAP or
News account.