664569 - Next button on Account Details page does not get enabled

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

STR:
1. Tools/Set Up Sync
2. Create a New Account
3. enter valid email address, 2x password, check "I agree ..."

Expected:
"Next >" button is enabled.

Actual results:
"Next >" button stays disabled.

Workaround:
Change the Server drop-down to "Use a custom server" and back to "SeaMonkey Sync Server".

This problem does not exist in Firefox, so it was somehow broken through the changes we made to the SeaMonkey port (a.k.a. review). :-(

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Hmm, for some reason this.status.server is false in syncSetup.js (while this.status.password and this.status.email are true) when readyToAdvance is called. Thus the check exits early and returns false on line 217. Firefox uses roughly the same code there, so it's really that this.status.server has the wrong value by then.

this.status.server is set by either onServerCommand or checkServer (the initial value is false of course). The difference between FF and SM is that FF calls onServerCommand from onPageShow (case NEW_ACCOUNT_START_PAGE) while this was removed for SM during review. :-(

I think we need to fix this ASAP. If not for 2.2, then maybe 2.3. The more/earlier, the better.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

The regression comes down to bug 634419 comment 25.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Neil, I'm inclined to fix this the easy way, by undoing your original request and bringing us back in line with what Firefox does (here).

Comment 4

[[e-mail]]

So what you're saying is that, unlike the email/password case, the server is initially valid?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> So what you're saying is that, unlike the email/password case, the server is
> initially valid?

It's a valid server since it's the default one, but the Next button should only unlock if you accept the ToS.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #4)
> So what you're saying is that, unlike the email/password case, the server is
> initially valid?

Yes (like philiKON said), but the init code also needs to do the correct thing if you come back to this page (it's a wizard!) after navigating elsewhere. So I wouldn't like to set this.status.server to true by default, because that might only work for the very initial case.

Comment 7

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #4)
> > So what you're saying is that, unlike the email/password case, the server is
> > initially valid?
> 
> It's a valid server since it's the default one, but the Next button should
> only unlock if you accept the ToS.
Sure, but there's a separate test for that. (In theory the server validity could have been made to depend on the ToS checkbox, but it doesn't.)

(In reply to comment #6)
> Yes (like philiKON said), but the init code also needs to do the correct
> thing if you come back to this page (it's a wizard!) after navigating
> elsewhere. So I wouldn't like to set this.status.server to true by default,
> because that might only work for the very initial case.
I've just double-checked and the page is nicely self-contained and I can't find anything on the other pages that could affect its validity.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2 [Checkin: comments 10 and 11]

OK, just flipping the default seems to do.

I found another issue, though (not directly related): If you change the server type to custom but don't enter anything into the (then) editable field, the Next button gets enabled. I found that control.selectedIndex == -1 in onServerCommand while it's only checked against > 0 (and only in SM; I guess I am to blame for having introduced this in bug 634419).

Comment 9

[[e-mail]]

Comment on attachment 540153 [details] [diff] [review]
patch v2 [Checkin: comments 10 and 11]

I see all the other cases compare selectedIndex == 0 already.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 540153 [details] [diff] [review]
patch v2 [Checkin: comments 10 and 11]

http://hg.mozilla.org/comm-central/rev/9685aa9dadd4

Requesting aurora/beta approval for simple and safe patch that 
1. fixes the case where you could not create a Sync account at all unless you found that switching the Sync server back-and-forth helped
2. fixes the case where you could proceed with an empty custom server.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 540153 [details] [diff] [review]
patch v2 [Checkin: comments 10 and 11]

http://hg.mozilla.org/releases/comm-aurora/rev/49d9e3760168
http://hg.mozilla.org/releases/comm-beta/rev/c9d3e5785364