Closed Bug 664569 Opened 13 years ago Closed 13 years ago

Next button on Account Details page does not get enabled

Categories

(SeaMonkey :: Sync UI, defect)

defect
Not set
normal

Tracking

(seamonkey2.2 fixed, seamonkey2.3 fixed)

RESOLVED FIXED
seamonkey2.4
Tracking Status
seamonkey2.2 --- fixed
seamonkey2.3 --- fixed

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file, 1 obsolete file)

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). :-(
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.
The regression comes down to bug 634419 comment 25.
Depends on: 634419
Attached patch patch (obsolete) — Splinter Review
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).
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #539676 - Flags: review?(neil)
So what you're saying is that, unlike the email/password case, the server is initially valid?
(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.
(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.
(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.
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).
Attachment #539676 - Attachment is obsolete: true
Attachment #539676 - Flags: review?(neil)
Attachment #540153 - Flags: review?(neil)
Comment on attachment 540153 [details] [diff] [review]
patch v2 [Checkin: comments 10 and 11]

I see all the other cases compare selectedIndex == 0 already.
Attachment #540153 - Flags: review?(neil) → review+
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.
Attachment #540153 - Attachment description: patch v2 → patch v2 [Checkin: comment 10]
Attachment #540153 - Flags: approval-comm-beta?
Attachment #540153 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.4
Attachment #540153 - Flags: approval-comm-beta?
Attachment #540153 - Flags: approval-comm-beta+
Attachment #540153 - Flags: approval-comm-aurora?
Attachment #540153 - Flags: approval-comm-aurora+
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
Attachment #540153 - Attachment description: patch v2 [Checkin: comment 10] → patch v2 [Checkin: comments 10 and 11]
Target Milestone: seamonkey2.4 → seamonkey2.2
Target Milestone: seamonkey2.2 → seamonkey2.4
You need to log in before you can comment on or make changes to this bug.