Closed
Bug 684537
Opened 13 years ago
Closed 13 years ago
Port Bug 626949 |Sync UI: Style generic change dialogs like the setup and Add a Device wizards| to suite
Categories
(SeaMonkey :: Sync UI, enhancement)
SeaMonkey
Sync UI
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.7
People
(Reporter: Callek, Assigned: Callek)
References
Details
Attachments
(2 files, 5 obsolete files)
99.40 KB,
image/png
|
neil
:
ui-review+
|
Details |
14.08 KB,
patch
|
Callek
:
review+
Callek
:
ui-review+
|
Details | Diff | Splinter Review |
Bug 626949 is a bit more invasive with Sync changes, so I wanted to do it in its own bug. I went as far as completely copying the xul from firefox, with adding communicator paths. Getting review from Jens before ui-r from Neil, as I can probably clean up the important stuff before neil checks the UI side alone. This patch applies on top of the one in Bug 684536 If a -w patch is actually helpful, let me know.
Attachment #558100 -
Flags: review?(jh)
Assignee | ||
Comment 1•13 years ago
|
||
Accidentally left out a big part of this patch, in another bug.
Attachment #558100 -
Attachment is obsolete: true
Attachment #558100 -
Flags: review?(jh)
Attachment #558102 -
Flags: review?(jh)
Comment 2•13 years ago
|
||
Comment on attachment 558102 [details] [diff] [review] v1.1 The following is wrong; we don't preprocess this file so no change is needed/wanted. It might actually break things. >+++ b/suite/common/sync/syncGenericChange.xul >@@ -1,45 +1,46 @@ > <?xml version="1.0"?> > >-<!-- ***** BEGIN LICENSE BLOCK ***** >- - Version: MPL 1.1/GPL 2.0/LGPL 2.1 >- - >- - The contents of this file are subject to the Mozilla Public License Version >- - 1.1 (the "License"); you may not use this file except in compliance with >- - the License. You may obtain a copy of the License at >- - http://www.mozilla.org/MPL/ >- - >- - Software distributed under the License is distributed on an "AS IS" basis, >- - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >- - for the specific language governing rights and limitations under the >- - License. >- - >- - The Original Code is Weave. >- - >- - The Initial Developer of the Original Code is the Mozilla Foundation. >- - Portions created by the Initial Developer are Copyright (C) 2009 >- - the Initial Developer. All Rights Reserved. >- - >- - Contributor(s): >- - Edward Lee <edilee@mozilla.com> >- - Mike Connor <mconnor@mozilla.com> >- - Paul OâShannessy <paul@oshannessy.com> >- - Philipp von Weitershausen <philipp@weitershausen.de> >- - >- - Alternatively, the contents of this file may be used under the terms of >- - either the GNU General Public License Version 2 or later (the "GPL"), or >- - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >- - in which case the provisions of the GPL or the LGPL are applicable instead >- - of those above. If you wish to allow use of your version of this file only >- - under the terms of either the GPL or the LGPL, and not to allow others to >- - use your version of this file under the terms of the MPL, indicate your >- - decision by deleting the provisions above and replace them with the notice >- - and other provisions required by the GPL or the LGPL. If you do not delete >- - the provisions above, a recipient may use your version of this file under >- - the terms of any one of the MPL, the GPL or the LGPL. >- - >- - ***** END LICENSE BLOCK ***** --> >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is Weave. >+# >+# The Initial Developer of the Original Code is the Mozilla Foundation. >+# Portions created by the Initial Developer are Copyright (C) 2009 >+# the Initial Developer. All Rights Reserved. >+# >+# Contributor(s): >+# Edward Lee <edilee@mozilla.com> >+# Mike Connor <mconnor@mozilla.com> >+# Paul OâShannessy <paul@oshannessy.com> >+# Philipp von Weitershausen <philipp@weitershausen.de> >+# >+# Alternatively, the contents of this file may be used under the terms of >+# either the GNU General Public License Version 2 or later (the "GPL"), or >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+# in which case the provisions of the GPL or the LGPL are applicable instead >+# of those above. If you wish to allow use of your version of this file only >+# under the terms of either the GPL or the LGPL, and not to allow others to >+# use your version of this file under the terms of the MPL, indicate your >+# decision by deleting the provisions above and replace them with the notice >+# and other provisions required by the GPL or the LGPL. If you do not delete >+# the provisions above, a recipient may use your version of this file under >+# the terms of any one of the MPL, the GPL or the LGPL. >+# >+# ***** END LICENSE BLOCK ***** >+ onwizardnext="Change.onLoad()" Missing semicolon. >+ <rows> >+ <row id="textBox1Row" align="center"> >+ <label id="textBox1Label" control="textBox1"/> >+ <textbox id="textBox1" type="password" oninput="Change.validate()"/> >+ <spacer/> >+ </row> >+ <row id="textBox2Row" align="center"> >+ <label id="textBox2Label" control="textBox2"/> >+ <textbox id="textBox2" type="password" oninput="Change.validate()"/> >+ <spacer/> >+ </row> >+ </rows> And twice here. >+ <textbox id="passphraseBox" >+ flex="1" >+ onfocus="this.select()" >+ oninput="Change.validate()"/> >+ </vbox> And here.
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #2) > Comment on attachment 558102 [details] [diff] [review] > v1.1 > > The following is wrong; we don't preprocess this file so no change is > needed/wanted. It might actually break things. O yea :/ fixed locally. > > >+ onwizardnext="Change.onLoad()" > > Missing semicolon. I'd argue this is purely cosmetic and not required in attribs like this (and that I copied directly from Firefox), but for consistency and clarity I added them anyway. (whole file).
Comment 4•13 years ago
|
||
Comment on attachment 558102 [details] [diff] [review] v1.1 Review of attachment 558102 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Justin Wood (:Callek) from comment #3) > > >+ onwizardnext="Change.onLoad()" > > > > Missing semicolon. > > I'd argue this is purely cosmetic and not required in attribs like this (and > that I copied directly from Firefox), but for consistency and clarity I > added them anyway. (whole file). Thanks. You're right, it's purely cosmetic, but it doesn't (shouldn't) hurt either. General note: The whole transformation of the dialogs to wizard pages is purely cosmetic, too, but I'm not opposed to it. I like consistency. :-) r- for now since we badly need a new patch (bitrot etc.). ::: suite/common/sync/syncGenericChange.xul @@ +99,5 @@ > <label id="passphraseLabel" control="passphraseBox"/> > + <spacer flex="1"/> > + <label id="generatePassphraseButton" > + hidden="true" > + value="&syncGenerateNewKey.label;" We didn't port bug 652483 yet so we don't have this entity, and consequently opening the Change Password dialog shows "XML Parsing Error: undefined entity". Please either keep the old entity or wrap porting bug 652483 into this one, then actually test your patch (!), then upload a new one for review.
Attachment #558102 -
Flags: review?(jh) → review-
Updated•13 years ago
|
Severity: normal → enhancement
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 5•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4) > We didn't port bug 652483 yet so we don't have this entity Similar with &existingRecoveryKey.description; (bug 656492) but I do *not* want to have that sneaked into this one, so please revert/remove that change. With the above changed as well I actually see some contents now, but still not the wizard page layout/design. You need to fix the CSS include: it's chrome://communicator/skin/sync/syncSetup.css not chrome://communicator/skin/syncSetup.css (unlike the FF guys, I actually gave some thought to keeping Sync-related files together...).
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4) > We didn't port bug 652483 yet so we don't have this entity, and consequently > opening the Change Password dialog shows "XML Parsing Error: undefined > entity". Please either keep the old entity or wrap porting bug 652483 into > this one, then actually test your patch (!), then upload a new one for > review. (In reply to Jens Hatlak (:InvisibleSmiley) from comment #5) > Similar with &existingRecoveryKey.description; (bug 656492) but I do *not* > want to have that sneaked into this one, so please revert/remove that change. Can we/you possibly get the Port Tracking Bug fleshed out with these bugs, so I can be sure of stuff, I (thought) I was caught up with this bug actually since the port tracker didn't have any others. :-P But sure, I'll fix that (I *thought* I did test this, apparently not)
Comment 7•13 years ago
|
||
(In reply to Justin Wood (:Callek) from comment #6) > Can we/you possibly get the Port Tracking Bug fleshed out with these bugs, > so I can be sure of stuff, I (thought) I was caught up with this bug > actually since the port tracker didn't have any others. :-P As I already said several times, the existing tracker bug is for Sync changes that happened *before FF4* but which we (I) missed. We should create a new tracker bug and add all new dependencies there. I will do that eventually, but with all the other SM-related stuff around I cannot give you a good estimate when that will be (creating the bug is cheap, finding all dependencies... not so much). > But sure, I'll fix that (I *thought* I did test this, apparently not) Keep in mind that porting that one will make this bug affect l10n, and there are only two weeks left until the next uplift.
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #7) > (In reply to Justin Wood (:Callek) from comment #6) > > Can we/you possibly get the Port Tracking Bug fleshed out with these bugs, > > so I can be sure of stuff, I (thought) I was caught up with this bug > > actually since the port tracker didn't have any others. :-P > > As I already said several times, the existing tracker bug is for Sync > changes that happened *before FF4* but which we (I) missed. I keep forgetting that, but means with this bug we'll be able to resolve *that* bug. > We should create a new tracker bug and add all new dependencies there. I > will do that eventually, but with all the other SM-related stuff around I > cannot give you a good estimate when that will be (creating the bug is > cheap, finding all dependencies... not so much). > > > But sure, I'll fix that (I *thought* I did test this, apparently not) > > Keep in mind that porting that one will make this bug affect l10n, and there > are only two weeks left until the next uplift. By "fix that" I more meant, "revert the l10n changes, if reasonably possible". And do the port(s) in another bug.
Assignee | ||
Comment 9•13 years ago
|
||
I opted to adjust the .dtd/etc. here for the new strings rather than tracking down which bugs changed this stuff. Let me know if we need to revert some of this, or do this bug over from scratch, and *attempt* to do it a different way.
Attachment #558102 -
Attachment is obsolete: true
Attachment #560759 -
Flags: review?(jh)
Comment 10•13 years ago
|
||
Comment on attachment 560759 [details] [diff] [review] v2 Review of attachment 560759 [details] [diff] [review]: ----------------------------------------------------------------- r- since I need answers to some of my questions. Otherwise I'm through with the review, though. Most things are just nits. The only major discussion point is the button vs label question. You might want to consult Neil for this specifically to cut down overall review time. ::: suite/common/sync/syncGenericChange.js @@ +100,3 @@ > } > else { > + document.getElementById("generatePassphraseButton").hidden = false; You missed/skipped the following line, without which the Print/Save buttons won't get shown (cf. attachment 505313 [details] [diff] [review]): document.getElementById("passphraseBackupButtons").hidden = false; @@ -104,2 @@ > document.title = this._str("change.synckey.title"); > - introText.textContent = this._str("change.synckey.introText"); With that, change.synckey.introText is now unused. FF removed it in unrelated bug 681519 (comment 2 over there), we should do it here. @@ +138,5 @@ > } > break; > } > + document.getElementById("change-page") > + .setAttribute("label", document.title); Please check whether .label = document.title works, too, or Neil will hunt us down. ::: suite/common/sync/syncGenericChange.xul @@ +68,5 @@ > + <wizardpage id="change-page" > + label=""> > + > + <description id="introText"> > + </description> I don't see why this should not stay <description id="introText"/>. Please revert unless you find a reason. @@ +99,3 @@ > <label id="passphraseLabel" control="passphraseBox"/> > + <spacer flex="1"/> > + <label id="generatePassphraseButton" I see that this has been a label for FF before, but for us it was a button with an accesskey. I think we should keep the button, but with the longer label, we should probably move it below the text box (right aligned, and with the small font size). Note that with the old code the button was shown by default, while with the new code, the label is hidden by default (cf. syncGenericChange.js changes). Ultimately I'll leave that decision to the UI reviewer (Neil), though. @@ +136,5 @@ > > + <vbox id="passphraseHelpBox" > + hidden="true"> > + <description> > + &existingRecoveryKey.description; See comment 5. /Please/ test your patch before upload. There is no way this has been working on your side! s/existingRecoveryKey/existingSyncKey/ should do it. @@ +146,5 @@ > + </vbox> > + > + <spacer id="passphraseSpacer" > + flex="1" > + hidden="true"/> This all fits onto one line, no? @@ +149,5 @@ > + flex="1" > + hidden="true"/> > + > + <description id="warningText" class="data"> > + </description> Again, no end tag please. Listing the ID first is fine, though. ::: suite/locales/en-US/chrome/common/sync/syncSetup.dtd @@ +44,4 @@ > <!ENTITY setup.newSyncKeyPage.description.label "To ensure your total privacy, all of your data is encrypted prior to being uploaded. The Sync Key which is necessary to decrypt your data is not uploaded."> > <!ENTITY syncKeyEntry.label "Your Sync Key"> > <!ENTITY syncKeyEntry.accesskey "K"> > +<!ENTITY syncGenerateNewKey.label "Generate a new key"> Please align (one more space). @@ -44,5 @@ > <!ENTITY setup.newSyncKeyPage.description.label "To ensure your total privacy, all of your data is encrypted prior to being uploaded. The Sync Key which is necessary to decrypt your data is not uploaded."> > <!ENTITY syncKeyEntry.label "Your Sync Key"> > <!ENTITY syncKeyEntry.accesskey "K"> > -<!ENTITY syncKeyGenerate.label "Generate"> > -<!ENTITY syncKeyGenerate.accesskey "G"> If we stick with the label ("Generate a new key"), then the accesskey is indeed obsolete since it doesn't work on labels. But if we go for the button, the accesskey needs to stay.
Attachment #560759 -
Flags: review?(jh) → review-
Updated•13 years ago
|
Status: NEW → ASSIGNED
Comment 11•13 years ago
|
||
The patch needs updating now that bug 689018 landed. In particular: * we have existingRecoveryKey now instead of existingSyncKey so don't need to change it anymore * change.synckey.introText still needs to be removed from the l10n file With the above, bug 681519 should be fully ported, too. Callek, to drive this forward I suggest you address all nits except the button vs label one, then upload a new patch and ask Neil for ui-review (and continue r?=me). Either you or me can create an old vs new screen shot then so Neil can decide faster.
Assignee | ||
Comment 12•13 years ago
|
||
Disclaimer: not yet tested but meticulously checked. This patch was written from scratch but with a strong attempt at addressing review comments, and an effort to match Firefox code where we differ elsewhere. I prefer the non-button version (if for no other reason than to match Firefox UI docs on the subject) But leaving it to you to create before/after screenshots for neil if you want to rely on him for approval on the change.
Attachment #560759 -
Attachment is obsolete: true
Attachment #567027 -
Flags: review?(jh)
Comment 13•13 years ago
|
||
Comment on attachment 567027 [details] [diff] [review] v3 Regressions from the last patch: >+++ b/suite/common/sync/syncGenericChange.js In this file, the following line (out of patch context, unfortunately) needs to be removed: let introText2 = document.getElementById("introText2"); > this._passphraseBox.value = pp; > document.title = this._str("change.recoverykey.title"); In between these two lines, the following needs to be added: this._passphraseBox.focus(); >- this._dialog.getButton("accept").label = this._str("new.password.acceptButton"); >+ this._dialog.getButton("finish").label = this._str("new.password.acceptButton"); > document.getElementById("textBox2Row").hidden = true; > } > else { >@@ -130,10 +133,12 @@ > box2label.value = this._str("new.password.confirm"); > introText.textContent = this._str("change.password3.introText"); > warningText.textContent = this._str("change.password.warningText"); >- this._dialog.getButton("accept").label = this._str("change.password.acceptButton"); >+ this._dialog.getButton("finish").label = this._str("change.password.acceptButton"); These two occurrences should be wrapped like in FF code, like this: this._dialog.getButton("finish").label = this._str("change.password.acceptButton"); (with "new" and "change") >+++ b/suite/common/sync/syncGenericChange.xul > (...) >+ persist="screenX screenY" This was only added with bug 564560, which did it for multiple files. I'd prefer to have that left for its own porting bug (which should be quick and easy then). >\ No newline at end of file Please fix. BTW, an answer to my own question from comment 10: > > + document.getElementById("change-page") > > + .setAttribute("label", document.title); > Please check whether .label = document.title works, too, or Neil will hunt us down. It doesn't. I'll attach screen shots for ui-review in a second. Feel free to provide an updated patch or leave this one for now; anyway I'll only make my final decision once I got feedback from Neil regarding the button vs link question. The rest is fine with me.
Comment 14•13 years ago
|
||
Neil, the focus of the UI review request is on whether we want to somehow keep the Generate button (current dialog in upper part) or go with the link (proposed dialog in lower part). The former has an accesskey, the latter doesn't (but matches FF and the documentation that exists for it). Otherwise I hope you agree with Callek and me that using the wizard style is actually an improvement. :-)
Attachment #567359 -
Flags: ui-review?(neil)
Comment 15•13 years ago
|
||
Comment on attachment 567359 [details]
key dialog comparison
Sure, this is OK. Note: I happened to notice that the field for the entry of the recovery key on the "I don't have the device with me" page looks wrong.
Attachment #567359 -
Flags: ui-review?(neil) → ui-review+
Comment 16•13 years ago
|
||
Actually, how does this compare to the "New Recovery Key" page in Sync setup?
Comment 17•13 years ago
|
||
[If I was mean I'd let the assignee try to answer these. ;-) Let's solve it together.] (In reply to neil@parkwaycc.co.uk from comment #15) > I happened to notice that the field for the entry of > the recovery key on the "I don't have the device with me" page looks wrong. Define "wrong" (platform? screen shot?). (In reply to neil@parkwaycc.co.uk from comment #16) > Actually, how does this compare to the "New Recovery Key" page in Sync setup? I didn't find a page named like that. Did you mean one of the two below? * "Change your Recovery Key" (from the "I have lost my other device" link)? This is the same as the one accessible from a set-up Sync account through Preferences. * "Update Recovery Key" (from an account in state "Wrong Sync Key", accessible through Preferences)? This is done by syncGenericChange.xul, too.
Comment 18•13 years ago
|
||
Comment on attachment 567027 [details] [diff] [review] v3 r=me provided all my nits will be addressed and Neil's questions answered. Don't hesitate to re-request review on a final patch if you feel the need. :-)
Attachment #567027 -
Flags: review?(jh) → review+
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #18) > Comment on attachment 567027 [details] [diff] [review] [diff] [details] [review] > v3 > > r=me provided all my nits will be addressed and Neil's questions answered. > Don't hesitate to re-request review on a final patch if you feel the need. > :-) According to Neil in IRC, his Questions were not against the patch/bug I am working on here, so shouldn't block things, if I am wrong I'm happy to do a followup ASAP of course.
Assignee | ||
Comment 20•13 years ago
|
||
For Checkin.
Attachment #567027 -
Attachment is obsolete: true
Attachment #567687 -
Flags: ui-review+
Attachment #567687 -
Flags: review+
Comment 21•13 years ago
|
||
Comment on attachment 567687 [details] [diff] [review] v3.1 Sorry pal, this is the wrong patch!
Attachment #567687 -
Attachment is obsolete: true
Attachment #567687 -
Flags: ui-review+
Attachment #567687 -
Flags: review+
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #21) > Comment on attachment 567687 [details] [diff] [review] [diff] [details] [review] > v3.1 > > Sorry pal, this is the wrong patch! So it was :( (I can't land yet anyway, *our* tree is closed due to hardware issues)
Attachment #567690 -
Flags: ui-review+
Attachment #567690 -
Flags: review+
Comment 23•13 years ago
|
||
Comment on attachment 567690 [details] [diff] [review] v3.2 [Checkin: comment 25] Interdiff looks good. :-)
Comment 24•13 years ago
|
||
(In reply to Jens Hatlak from comment #17) > (In reply to comment #16) > > Actually, how does this compare to the "New Recovery Key" page in Sync setup? > I didn't find a page named like that. Did you mean one of the two below? > * "Change your Recovery Key" (from the "I have lost my other device" link)? > This is the same as the one accessible from a set-up Sync account through > Preferences. > * "Update Recovery Key" (from an account in state "Wrong Sync Key", > accessible through Preferences)? This is done by syncGenericChange.xul, too. Sorry, I meant "My Recovery Key" but as you mention that's also the same file.
Comment 25•13 years ago
|
||
Comment on attachment 567690 [details] [diff] [review] v3.2 [Checkin: comment 25] http://hg.mozilla.org/comm-central/rev/24f6f528d54a
Attachment #567690 -
Attachment description: v3.2 → v3.2 [Checkin: comment 25]
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.7
You need to log in
before you can comment on or make changes to this bug.
Description
•