Last Comment Bug 664569 - Next button on Account Details page does not get enabled
: Next button on Account Details page does not get enabled
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Sync UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.4
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 634419
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 14:59 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-08-03 22:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
patch (977 bytes, patch)
2011-06-15 15:34 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
patch v2 [Checkin: comments 10 and 11] (1.38 KB, patch)
2011-06-17 15:34 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-06-15 14:59:48 PDT
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) 2011-06-15 15:20:08 PDT
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) 2011-06-15 15:28:37 PDT
The regression comes down to bug 634419 comment 25.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-06-15 15:34:52 PDT
Created attachment 539676 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-06-15 16:06:08 PDT
So what you're saying is that, unlike the email/password case, the server is initially valid?
Comment 5 Philipp von Weitershausen [:philikon] 2011-06-15 16:07:52 PDT
(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) 2011-06-16 00:26:07 PDT
(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 neil@parkwaycc.co.uk 2011-06-17 05:47:45 PDT
(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) 2011-06-17 15:34:49 PDT
Created attachment 540153 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-06-17 16:10:17 PDT
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) 2011-06-17 16:23:51 PDT
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) 2011-06-18 01:10:22 PDT
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

Note You need to log in before you can comment on or make changes to this bug.