32726 - Account Settings | Server Setting dialog does not work appropriately

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P3)

Description

[Karen Huang]

Used 03-21-06-M15-nb1b commercial build:

Account Settings | Server Setting dialog does not work appropriately

1) Login to a POP Mail Account
2) Select Edit| Mail/News Account Setting
3) Select "Server" from this Account Setting dialog
4) Checked Leave message on server...etc.
5) Select OK.
6) Actual Results: This OK button is not working.

Expected results: Should allow to select OK button and allow to select the other 
options from this Account Setting dialog. This "server" option shouln't lock you 
up....

Additional Info: Once you select the "Server" option from the Account Setting, 
it will lock you up....you cannot select "OK" or select other option (ex: Copies 
and Folders)except you select Cancel for dismiss this dialog....

Not try on IMAP and other platforms yet.

Comment 1

[Karen Huang]

I used/created new profile from today's build.
Adding pp...still have time for fix on Beta1? 

Comment 2

[Karen Huang]

No problem on IMAP mail account.
This only occurred on POP account.
Regression bug, I am just adding beta1 here, let PDT team to decide PDT+ or 
PDT-.

Comment 3

[lchiang]

It's too late for beta now.

Comment 4

[Karen Huang]

OK. Removing beta1... adding relnote.

Comment 5

[Karen Huang]

Oops. Remove relnote.
Let Ninoschka decide whether it's relnote or not since it's her feature now...

Comment 6

[leger]

Putting on NEED INFO radar whilst phil checks into this.

Comment 7

[Ninoschka Baca]

Build 2000-03-21-06M15nb1b: NT4, Mac 8.5.1
Build 2000-03-21-15M15nb15: Linux 6.0
The problem occurs on all platforms in the branch builds. The trunk/tip builds 
are ok.

Comment 8

[leger]

Putting on PDT+ radar.

Comment 9

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

re-assign to mscott.   we've tracked down the problem

the checkin for #32119 caused this. 

more details in a second...

(cc'ing the players in this sorted drama)

Comment 10

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

the problem is we are doing a foo.value from our account manager javascript,
where foo is a HTMLSelectElement object (of type select-one)

(it's the one named imap.deleteModel, see am-server.xul)

when configuring a pop server, this select element is not visible, but when we
iterate over all the input elements and call our function getFormElementValue()
[in AccountManager.js] we end up calling .value on the HTMLSelectElement object.
and that raises a JS exception.

it raises an exception because 
nsHTMLSelectElement::GetValue() returns !NS_OK

and it returns !NS_OK because GetSelectedIndex() returns something !NS_OK

This code changed with ducarroz/rods last checkin to nsHTMLSelectElement.cpp

Comment 11

[rods (gone)]

Boy there is a lot of ways we can go with this. I actually think two things are 
incorrect.

1) If there is no frame and although there may be a presstate if it doesn't have 
a cached value we should pass back the default value.

2) GetValue should always pass back NS_OK, and for that matter I think 
GetSelectedIndex should too. I think now I remember vidur saying something about 
these DOM calls always need to pass back NS_OK.

Comment 12

[Jean-Francois Ducarroz]

Possible work around: Try to use element.getAttribute("value") instead of 
element.value. Same when you are setting the value. Ussually works much better 
for element without a frame (case when element not visible).

Comment 13

[rods (gone)]

Attachment: patch against the source on the TIP

Comment 14

[rods (gone)]

The above patch is against the source on the tip. Here is the method I changed 
in its entirety. My build here at home is the tip and it would take to long to 
build on the branch but this file should be almost the same. If not, here is the  
entire method I changed:

NS_IMETHODIMP
nsHTMLSelectElement::GetSelectedIndex(PRInt32* aValue)
{
  nsIFormControlFrame* formControlFrame = nsnull;
  nsresult rv = nsGenericHTMLElement::GetPrimaryFrame(this, formControlFrame);
  if (NS_SUCCEEDED(rv)) {
    nsString value;
    rv = formControlFrame->GetProperty(nsHTMLAtoms::selectedindex, value);
    if (NS_SUCCEEDED(rv) && (value.Length() > 0)) {
      PRInt32 retval = 0;
      PRInt32 error = 0;
      retval = value.ToInteger(&error, 10); // Convert to integer, base 10
      if (!error) {
        *aValue = retval;
      } else {
        rv = NS_ERROR_UNEXPECTED;
      }
    }
  } else { // The frame hasn't been created yet.  Use the options array
    *aValue = -1;
    // Call the helper method to get the PresState and 
    // the SupportsArray that contains the value
    //
    nsCOMPtr<nsIPresState> presState;
    nsCOMPtr<nsISupportsArray> value;
    nsresult res = GetPresState(getter_AddRefs(presState), 
getter_AddRefs(value));
    if (NS_SUCCEEDED(res) && presState) {
      nsCOMPtr<nsISupports> supp;
      presState->GetStatePropertyAsSupports("selecteditems", 
getter_AddRefs(supp));

      res = NS_ERROR_NULL_POINTER;
      if (supp) {

        nsCOMPtr<nsISupportsArray> value = do_QueryInterface(supp);
        if (value) {

          PRUint32 count = 0;
          value->Count(&count);

          nsCOMPtr<nsISupportsPRInt32> thisVal;
          for (PRUint32 i=0; i<count; i++) {
            nsCOMPtr<nsISupports> suppval = getter_AddRefs(value->ElementAt(i));
            thisVal = do_QueryInterface(suppval);
            if (thisVal) {
              res = thisVal->GetData(aValue);
              if (NS_SUCCEEDED(res)) {
                // if we find the value the return NS_OK
                return NS_OK;
              }
            } else {
              res = NS_ERROR_UNEXPECTED;
            }
            if (!NS_SUCCEEDED(res)) break;
          }
        }
      }

      // At this point we have no frame
      // and the PresState didn't have the value we were looking for
      // so now go get the default value.
      if (NS_FAILED(res)) {
        // If we are a combo box, our default selectedIndex is 0, not -1;
        // XXX The logic here is duplicated in
        // nsCSSFrameConstructor::ConstructSelectFrame and
        // nsSelectControlFrame::GetDesiredSize
        PRBool isMultiple;
        rv = GetMultiple(&isMultiple);         // Must not be multiple
        if (NS_SUCCEEDED(rv) && !isMultiple) {
          PRInt32 size = 1;
          rv = GetSize(&size);                 // Size 1 or not set
          if (NS_SUCCEEDED(rv) && ((1 >= size) || (NS_CONTENT_ATTR_NOT_THERE == 
size))) {
            *aValue = 0;
          }
        }
        nsCOMPtr<nsIDOMNSHTMLOptionCollection> options;
        rv = GetOptions(getter_AddRefs(options));
        if (NS_SUCCEEDED(rv) && options) {
          PRUint32 numOptions;
          rv = options->GetLength(&numOptions);
          if (NS_SUCCEEDED(rv)) {
            for (PRUint32 i = 0; i < numOptions; i++) {
              nsCOMPtr<nsIDOMNode> node;
              rv = options->Item(i, getter_AddRefs(node));
              if (NS_SUCCEEDED(rv) && node) {
                nsCOMPtr<nsIDOMHTMLOptionElement> option = 
do_QueryInterface(node, &rv);
                if (NS_SUCCEEDED(rv) && option) {
                  PRBool selected;
	                rv = option->GetDefaultSelected(&selected); // 
DefaultSelected == HTML Selected
                  if (NS_SUCCEEDED(rv) && selected) {
                    *aValue = i;
                    break;
                  }
                }
              }
            }
          }
        }
      }
    }
  }

  return NS_OK;
}

Comment 15

[rods (gone)]

Here is what I did:

As before it checks to see if there is a frame.
If not, then it checks for a PresState, if it fails to get the value from the 
PresState then it gets the default value. The default value in the case of xul 
will be zero. I think for html it will be the same.

The return value is always set to a -1 if it fails to find a frame and it goes 
on to set it zero if it determines it is a combobox.

I think it is fine to always return NS_OK for GetSelectedIndex because no matter 
what it has a reasonable value.

I am still wondering if we are to ever pass back anything but NS_OK back from 
these DOM calls, but that is for another day.

Comment 16

[Jean-Francois Ducarroz]

I am trying to apply this patch on the B1 branch right now...

Comment 17

[rods (gone)]

Attachment: tests a bunch of things

Comment 18

[rods (gone)]

The attachment I just added enables you to test sefveral of the code paths.

Load the page and press the GetSelectedInx, this tests the new code path where 
the PresState is there but has no value. Hit show and then GetSelectedInx to 
test it with a frame. Set a value then hide it and press GetSelectedInx to test 
the code path where it get the value from the PresState.

I tested these and they all seemed to work.

Comment 19

[Scott MacGregor]

rods change to GetSelectedIndex fixes this bug. And I've verified that it hasn't
caused a regression in the compose bug which the change was originally trying to
fix. I have to run the test cases he attached here next. But things look good!

Comment 20

[Jean-Francois Ducarroz]

message composition look good too. Now, as rickg will ask to pass the top 100 
web sites test, somebody need to do it (and it's very borring!)

Comment 21

[Scott MacGregor]

I've already got approval from rickg. I've got to run through some of my
favorite sites just to make sure nothing looks weird.

I'll be checking this in later tonight.

Comment 22

[Jean-Francois Ducarroz]

great. Thanks for taking care of this.

Comment 23

[Scott MacGregor]

Okay a=rickg, I've checked this change into the branch. rods, do you want the
honors of checking it into the tip?

Marking as fixed!! And this is the last of the engineering PDT+ bugs as far as I
can tell! Yeah!!!

Comment 24

[rods (gone)]

Yes, I'll check the change into the tip tomorrow (wed.)

Comment 25

[Ninoschka Baca]

Build 2000-22-06M15nb1b: Win95
Build 2000-22-05M15nb1b: Linux 6.0, Mac 8.5.1
Verified Fixed. Great!

- After selecting a POP Server panel then:
   - switching to another panel refreshes the right pane (i.e. Identity panel, 
    Copies & Folders panel, Local Folders and Outgoing (SMTP))
   - selecting any checkbox such as "Leave messages on server" and selecting the 
   OK button, closes the Account Settings dialog as expected. I double checked 
   Account Settings and the changes are saved.