Closed Bug 618709 Opened 14 years ago Closed 14 years ago

Dialogs part of |Bug 576970 - Port Sync UI to SeaMonkey trunk|

Categories

(SeaMonkey :: Sync UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(7 files, 12 obsolete files)

16.66 KB, patch
neil
: review+
Details | Diff | Splinter Review
3.32 KB, patch
neil
: review+
Details | Diff | Splinter Review
6.89 KB, patch
InvisibleSmiley
: review+
Details | Diff | Splinter Review
13.31 KB, patch
neil
: review+
Details | Diff | Splinter Review
53.14 KB, patch
neil
: review+
Details | Diff | Splinter Review
4.21 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.08 KB, patch
neil
: review+
Details | Diff | Splinter Review
The "dialogs" patch from bug 576970 is probably too big to be reviewed easily, that's why I'm splitting it up into smaller bits and move the whole review of that part to this bug. HTH. Note: - all the other attachments on bug 576970 need to be applied first in order to test this. Some of them do not apply 100% right now due to recent landings - later FF4 Sync UI changes are up for bug 615950 - Modern is still not part of the first round - and larger changes should go to follow-up bugs
Attached patch generic (obsolete) — Splinter Review
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #497160 - Flags: review?(neil)
Attached patch key (obsolete) — Splinter Review
Attached patch notification (obsolete) — Splinter Review
Attached patch quota (obsolete) — Splinter Review
Attached patch setup (obsolete) — Splinter Review
Comment on attachment 497160 [details] [diff] [review] generic >+ get _passphraseBox() { >+ delete this._passphraseBox; >+ return this._passphraseBox = document.getElementById("passphraseBox"); Nit: might as well set this._passphraseBox in onLoad >+ introText.innerHTML = this._str("change.synckey.introText"); >+ introText2.innerHTML = this._str("change.synckey.introText2"); >+ warningText.innerHTML = this._str("change.synckey.warningText"); Where do these strings come from? (Or where are the Firefox versions?) >+ this._dialog.getButton("accept") >+ .setAttribute("label", this._str("change.password.acceptButton")); ... >+ this._dialog.getButton("accept").setAttribute("disabled", !error); Dialog buttons work just like other buttons, so you should be able to use properties rather than attributes. (Sadly there's no cancel button version of the buttondisabledaccept attribute...) >+ document.getElementById("passphraseStrengthRow").hidden = true; [This seems suboptimal to see, but I'd need to see it in action.] >+ // The generated 20 character passphrase has an entropy of 94 bits >+ // which we consider "strong". >+ if (bits > 94) >+ meter.className = "strong"; This doesn't class 94 as strong! Try >= >+ <column align="right"/> [Does this actually work?] >+ <label id="generatePassphraseButton" >+ value="&syncKeyGenerate.label;" >+ class="text-link inline-link" >+ onclick="event.stopPropagation(); >+ Change.doGeneratePassphrase();"/> Why isn't this a button? >+ <hbox align="right" flex="1"> This align makes no sense. (You probably could get rid of the whole box.)
(In reply to comment #6) > >+ introText.innerHTML = this._str("change.synckey.introText"); > >+ introText2.innerHTML = this._str("change.synckey.introText2"); > >+ warningText.innerHTML = this._str("change.synckey.warningText"); > Where do these strings come from? (Or where are the Firefox versions?) syncGenericChange.properties (part of the "locales" patch on the parent bug, like every other Sync-specific string that is under browser/ for FF).
Comment on attachment 497160 [details] [diff] [review] generic Some driveby comments: When you select "My Sync Key" or "Change Password" from preferences pane I get the following in the error console: Error: XPCOMUtils is not defined Source File: chrome://communicator/content/sync/syncUtils.js Line: 309 For all the dialogs available from the preferences pane, they all need more accessibility - generally need more accesskeys on most buttons and input fields, more specifically in "Change your Sync Key" dialog, is it possible to turn "Generate" into a button and in "Server Quota", there does not seem to be a way of using the keyboard to uncheck/check the items :( Not sure how easy it is to add accesskeys to the radio buttons in the wizard that "Reset Sync" calls.
Attached patch generic v1a (obsolete) — Splinter Review
(In reply to comment #6) > >+ return this._passphraseBox = document.getElementById("passphraseBox"); > Nit: might as well set this._passphraseBox in onLoad I also replaced other occurrences of d.gEBI("passphraseBox") by this._passphraseBox. > >+ <column align="right"/> > [Does this actually work?] Doesn't look like it; removed. > >+ <label id="generatePassphraseButton" > >+ value="&syncKeyGenerate.label;" > >+ class="text-link inline-link" > >+ onclick="event.stopPropagation(); > >+ Change.doGeneratePassphrase();"/> > Why isn't this a button? Ask the author; changed. The accesskey will be in the next version of the locales patch on the parent bug. > >+ <hbox align="right" flex="1"> > This align makes no sense. (You probably could get rid of the whole box.) Why not? The link (text) is aligned to the right of the passphrase strength meter (which has a label in front of it). Please check how it looks, then reconsider (I'll bring the parent bug into a state that allows applying all patches against current trunk again). Left as-is for now.
Attachment #497160 - Attachment is obsolete: true
Attachment #497643 - Flags: review?(neil)
Attachment #497160 - Flags: review?(neil)
(In reply to comment #8) > Some driveby comments: > When you select "My Sync Key" or "Change Password" from preferences pane I get > the following in the error console: > Error: XPCOMUtils is not defined > Source File: chrome://communicator/content/sync/syncUtils.js > Line: 309 Yeah, syncUtils.js is missing the XPCOMUtils include (which is not needed in the other places outside Preferences right now). I'll add the include in the new "common" patch that I'll upload on the parent bug in a minute, and assume r=Neil on that small change (the rest has already been reviewed by him). > For all the dialogs available from the preferences pane, they all need more > accessibility - generally need more accesskeys on most buttons and input > fields, more specifically in "Change your Sync Key" dialog, is it possible to > turn "Generate" into a button and in "Server Quota", there does not seem to be > a way of using the keyboard to uncheck/check the items :( "Generate" is now a button, and I'll assume r=you for the G accesskey addition to the "locales" patch on the parent bug. The Server Quota dialog has other issues (like a JS error regarding gSyncQuota.bundle.getFormattedString) that we'll need to deal with when that part is up for review. > Not sure how easy it is to add accesskeys to the radio buttons in the wizard > that "Reset Sync" calls. I'm not an expert there, but it seems not a big deal currently since you can just focus one radio button using Tab and then cycle through the options with the arrow keys. And "currently" is all I care about currently...
Comment on attachment 497643 [details] [diff] [review] generic v1a I almost forgot: I also added this._clearStatus(); in doGeneratePassphrase. Otherwise, if you have an error (like "passphrase too short") and then select Generate for a new passphrase, the error message is not cleared. Judging from the source, that is still an issue in FF land. Somebody remind me that I tell philiKON...
(In reply to comment #9) > (In reply to comment #6) > > >+ return this._passphraseBox = document.getElementById("passphraseBox"); > > Nit: might as well set this._passphraseBox in onLoad > I also replaced other occurrences of d.gEBI("passphraseBox") by > this._passphraseBox. Heh, I hadn't even noticed that :-) > > >+ <hbox align="right" flex="1"> > > This align makes no sense. (You probably could get rid of the whole box.) > Why not? The link (text) is aligned to the right of the passphrase strength > meter (which has a label in front of it). Ah, then you should probably be using pack="end" instead. (But I'm still unsure about the flex.)
(In reply to comment #7) > (In reply to comment #6) > > >+ introText.innerHTML = this._str("change.synckey.introText"); > > >+ introText2.innerHTML = this._str("change.synckey.introText2"); > > >+ warningText.innerHTML = this._str("change.synckey.warningText"); > > Where do these strings come from? (Or where are the Firefox versions?) > syncGenericChange.properties (part of the "locales" patch on the parent bug, > like every other Sync-specific string that is under browser/ for FF). Bah, I MXR'd for change.synckey.warningText but Firefox has had to change it to change.synckey2.warningText so I failed to find it :-( So I checked syncGenericChange.properties and it doesn't contain any HTML. So you don't need to use innerHTML. So you don't need to use <html:p> (or xmlns:html) either, just set the textContent directly on the description. >+ if (bits > 94) >+ meter.className = "strong"; I didn't make it clear that I would prefer you to use >= for 47 too.
(In reply to comment #9) > Please check how it looks What's the easiest way to do that? I've just been doing code reviews so far.
Attached patch generic v1b (obsolete) — Splinter Review
Attachment #497643 - Attachment is obsolete: true
Attachment #497842 - Flags: review?
Attachment #497643 - Flags: review?(neil)
(In reply to comment #13) > So I checked syncGenericChange.properties and it doesn't contain any HTML. So > you don't need to use innerHTML. So you don't need to use <html:p> (or > xmlns:html) either, just set the textContent directly on the description. I just realized you also wanted me to change the XUL... This would then require me to come up with solutions to these two problems: 1. The first <description> contains two <p>s, which automatically take care of the vertical space in between them. If I just modified the <description>'s textContext, I'd have to deal with that differently. 2. The second <description> has class="data" set which makes the text bold (the .data CSS rule is in the "theming" patch on the original bug). If I moved that to the <description>, would that still work? Frankly I don't see what that change would improve. If you still want me to change it, please help me with the above points, or allow me to move it to a follow-up.
Comment on attachment 497161 [details] [diff] [review] key I'm putting this up for review, too, since it's small and connected to the "generic" dialogs patch. This is the file that is saved if you press the button with id=saveSyncKeyButton, which calls gSyncUtils.passphraseSave (contained in the "common" patch of the parent bug). Note: FF bug 612584 (part of SM tracker bug 615950) has been reopened, which is about the encoding of the file contained in this patch. The issue only appeared because gSyncUtils.passphraseSave saves the XHTML file as HTML, so when the file is loaded with the .html extension later, the HTML5 parser ignores the UTF-8 statement in the XML declaration. If we decide to save the file as .xhtml, there would be no issue for us (except some slight deviation from FF Sync behavior). I'd just update the "common" patch with r+ if I get a positive reply to that part here.
Attachment #497161 - Flags: review?(neil)
(In reply to comment #16) > 1. The first <description> contains two <p>s, which automatically take care of > the vertical space in between them. If I just modified the <description>'s > textContext, I'd have to deal with that differently. Use two <description>s (i.e. change the outer description into a vbox flex="1" and change each html:p into a description). [While I was looking into this I noticed an <hbox align="top">; I don't ever remember this being legal, maybe they were thinking of valign="top" which is deprecated, see nsBoxFrame::GetInitialVAlignment] > 2. The second <description> has class="data" set which makes the text bold (the > .data CSS rule is in the "theming" patch on the original bug). If I moved that > to the <description>, would that still work? It should do. > Frankly I don't see what that change would improve. Code quality. It also stops HTML injection attacks by localisers ;-)
Strange; when I tried it there was no second paragraph of intro text.
Oh, I see now, it depends on which button I click...
(In reply to comment #6) >>+ document.getElementById("passphraseStrengthRow").hidden = true; >[This seems suboptimal to see, but I'd need to see it in action.] OK, so the dialog cheats by setting a height on the containing box, and then showing or hiding the inner boxes as desired. When opening the Sync Key dialog, my current sync key of 25 characters isn't hyphenated, should it be? Also when I click on Generate I only get a hyphenated 20 character sync key.
(In reply to comment #21) > When opening the Sync Key dialog, my current sync key of 25 characters isn't > hyphenated, should it be? Also when I click on Generate I only get a hyphenated > 20 character sync key. Yes, that's broken in the initial implementation, which only hyphenates if the Sync Key has exactly 20 characters. Fixed through attachment 493479 [details] [diff] [review] from FF bug 612699 which is on the depends list of our follow-up meta bug 615950.
Attached patch generic v1cSplinter Review
changed align=top to align=start (which we need, since otherwise the icon is stretched vertically)
Attachment #497842 - Attachment is obsolete: true
Attachment #498624 - Flags: review?(neil)
Attachment #497842 - Flags: review?
Comment on attachment 497161 [details] [diff] [review] key Um... how do you actually access this?
Comment on attachment 498624 [details] [diff] [review] generic v1c >+ // The generated 20 character passphrase has an entropy of 94 bits >+ // which we consider "strong". So, I thought I would check to see how strong the strength checker thought the generated passphrase was. To get a strength of 94 seems to require 78 alphabetic characters, or only 72 if you have any non-alphabetic characters. To get a strength of 47 is much easier; it only requires 25 if you have any non-alphabetic characters, or 30 if you only use alphabetic characters.
Attachment #498624 - Flags: review?(neil) → review+
(In reply to comment #24) > Comment on attachment 497161 [details] [diff] [review] > key > > Um... how do you actually access this? See comment 17.
(In reply to comment #21) > When opening the Sync Key dialog, my current sync key of 25 characters isn't > hyphenated, should it be? Also when I click on Generate I only get a hyphenated > 20 character sync key. Interestingly I happened to have saved my sync key when setting up sync. And that's a 20 character hyphenated key. So I don't know how the key in the dialog is being generated :-( (In reply to comment #26) > (In reply to comment #24) > > Um... how do you actually access this? > See comment 17 D'oh!
Comment on attachment 497161 [details] [diff] [review] key >+<!DOCTYPE html [ >+ <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd"> >+ %htmlDTD; >+ <!ENTITY % syncBrandDTD SYSTEM "chrome://communicator/locale/sync/syncBrand.dtd"> >+ %syncBrandDTD; >+ <!ENTITY % syncKeyDTD SYSTEM "chrome://communicator/locale/sync/syncKey.dtd"> >+ %syncKeyDTD; >+]> So, I'm a little confused; you're obviously reading this as xhtml in order for the entities to work, but I can't tell whether you're serialising this as HTML or XHTML. If you're serialising this as XHTML, then I'd recommend changing the file type, but if you're serialising this as HTML, then I'd recommend adding a meta tag to set the charset. >+ #synckey { font-size: 150% } Nit: by comparison, h2 uses 1.5em >+ footer { font-size: 70% } Nit: by comparison, h6 uses 0.67em >+ /* Bug 575675: Need to have an a:visited rule in a chrome document. */ >+ a:visited { color: purple; } This makes no sense once you've saved it. I don't think we'd miss visited link decoration on the printout (assuming this file is used for that too).
Attached patch key v1aSplinter Review
(In reply to comment #28) > I can't tell whether you're serialising this as HTML > or XHTML. If you're serialising this as XHTML, then I'd recommend changing the > file type, but if you're serialising this as HTML, then I'd recommend adding a > meta tag to set the charset. From passphraseSave: filepicker.defaultString = "SeaMonkey Sync Key.html"; ... let serializer = new XMLSerializer(); let output = serializer.serializeToString(iframe.contentDocument); output = output.replace(/<!DOCTYPE (.|\n)*?]>/, '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" ' + '"DTD/xhtml1-strict.dtd">'); output = Weave.Utils.encodeUTF8(output); So unless you object, I'll take an r+ for this patch as permission to also carrying forward r+ on the "common" patch on the parent bug and renaming "SeaMonkey Sync Key.html" to "SeaMonkey Sync Key.xhtml" in syncUtils.js. I'll also remove "Bug 612584 - Sync-Key file doesn't include text encoding" from the depends list of our follow-up bug 615950 then. > >+ /* Bug 575675: Need to have an a:visited rule in a chrome document. */ > >+ a:visited { color: purple; } > This makes no sense once you've saved it. I don't think we'd miss visited link > decoration on the printout (assuming this file is used for that too). Removed (yes, the file is used for Print, too).
Attachment #497161 - Attachment is obsolete: true
Attachment #498778 - Flags: review?(neil)
Attachment #497161 - Flags: review?(neil)
Comment on attachment 498778 [details] [diff] [review] key v1a r=me when saved as XHTML.
Attachment #498778 - Flags: review?(neil) → review+
Attachment #497162 - Flags: review?(neil)
Comment on attachment 497162 [details] [diff] [review] notification >+ <xul:vbox xbl:inherits="hidden=notificationshidden"> So, the difference here is that the notifications are all visible at the same time, rather than stacking as they do normally? >+ <xul:spacer/> [I don't actually know what the spacer does in the base binding either.] >+ Components.utils.import("resource://services-sync/ext/Observers.js"); >+ Components.utils.import("resource://services-sync/notifications.js"); [Shouldn't these be imported into a private scope rather than polluting the global scope?] >+ for each (var notification in Notifications.notifications) >+ this._appendNotification(notification); [As this is an array, shouldn't you be using Notifications.notifications.forEach(this._appendNotification, this); ?] >+ <!-- The children are the buttons defined by the notification. --> >+ <xul:hbox oncommand="document.getBindingParent(this)._doButtonCommand(event);"> >+ <children/> >+ </xul:hbox> >+ <xul:spacer flex="1"/> So the buttons are in the "middle", rather than on the right? Seems odd to me. But in that case you don't need the spacer. (And you could move the oncommand back to the parent box and remove the box too.) Alternatively, to move the buttons to the right, just remove the content completely, since you will then end up with the parent binding's content. >+ <!-- Note: this used to be a field, but for some reason it kept getting >+ - reset to its default value for TabNotification elements. >+ - As a property, that doesn't happen, even though the property >stores >+ - its value in a JS property |_notification| that is not defined >+ - in XBL as a field or property. Maybe this is wrong, but it works. >+ --> [Offhand I can't see why a field wouldn't work but FYI the other workaround is to simply remove the property altogether.] >+ Notifications.remove(this.notification); [Ah, here's the other global scope abuser.]
(In reply to comment #31) > So, the difference here is that the notifications are all visible at the same > time, rather than stacking as they do normally? I have no idea. I didn't write the original code, only ported it. That applies to everything related to Sync, actually, including, but not limited to, the below. Unfortunately I don't know how to trigger multiple notifications. The only way I know to trigger one at all is to use Tools/Sync Now or Connect on the Sync pref pane while you're offline. CC'ing philiKON: hope you can answer some of this. Feel free to remove yourself from the list if it distracts you too much. ;-) > >+ <xul:spacer/> > [I don't actually know what the spacer does in the base binding either.] philiKON? > >+ Components.utils.import("resource://services-sync/ext/Observers.js"); > >+ Components.utils.import("resource://services-sync/notifications.js"); > [Shouldn't these be imported into a private scope rather than polluting the > global scope?] You mean like let localScope = {}; .../Observers.js", localScope); localScope.Observers.add(... Well sure I guess that's possible, just let me know which scope variable names you'd like to have. IIUC, I'd have to do that separately in all affected methods (constructor, destructor, "close"), right? > >+ for each (var notification in Notifications.notifications) > >+ this._appendNotification(notification); > [As this is an array, shouldn't you be using > Notifications.notifications.forEach(this._appendNotification, this); > ?] Haven't checked. philiKON, anything speaking against that? > >+ <!-- The children are the buttons defined by the notification. --> > >+ <xul:hbox oncommand="document.getBindingParent(this)._doButtonCommand(event);"> > >+ <children/> > >+ </xul:hbox> > >+ <xul:spacer flex="1"/> > So the buttons are in the "middle", rather than on the right? Seems odd to me. The only button I could see, "Preferences..." (see scenario at the beginning of this comment), is indeed at the end of the message. At the far right of the notification bar is the red close button, and I'd rather not move any buttons next to that one... > But in that case you don't need the spacer. (And you could move the oncommand > back to the parent box and remove the box too.) I'd guess the spacer (with the flex) is needed to maintain the space between the button at the end of the message and the close button at the far right of the notification bar, no? > >+ <!-- Note: this used to be a field, but for some reason it kept getting > >+ - reset to its default value for TabNotification elements. > >+ - As a property, that doesn't happen, even though the property >stores > >+ - its value in a JS property |_notification| that is not defined > >+ - in XBL as a field or property. Maybe this is wrong, but it works. > >+ --> > [Offhand I can't see why a field wouldn't work but FYI the other workaround is > to simply remove the property altogether.] Well I don't understand the specifics either way, so if you want me to change something here, please be a little more specific, since if you leave me the choice, I simply won't change anything there. > >+ Notifications.remove(this.notification); > [Ah, here's the other global scope abuser.] I think you missed the 2x "Observers" in the destructor. ;-)
(In reply to comment #32) > > [I don't actually know what the spacer does in the base binding either.] > philiKON? He probably just cargo culted it too. It could go if it does nothing. > > [Shouldn't these be imported into a private scope rather than polluting the > > global scope?] > IIUC, I'd have to do that separately in all affected methods (constructor, > destructor, "close"), right? Well, I was thinking we could use "this" as the scope, but that doesn't work well for "close". > > So the buttons are in the "middle", rather than on the right? Seems odd to me. > The only button I could see, "Preferences..." (see scenario at the beginning of > this comment), is indeed at the end of the message. At the far right of the > notification bar is the red close button, and I'd rather not move any buttons > next to that one... Why not? Regular notifications do it that way... > > But in that case you don't need the spacer. (And you could move the oncommand > > back to the parent box and remove the box too.) > I'd guess the spacer (with the flex) is needed to maintain the space between > the button at the end of the message and the close button at the far right of > the notification bar, no? No, because there's yet another box flexing it to the right. > > [Offhand I can't see why a field wouldn't work but FYI the other workaround is > > to simply remove the property altogether.] > Well I don't understand the specifics either way, so if you want me to change > something here, please be a little more specific, since if you leave me the > choice, I simply won't change anything there. I guess it's better to be documented as being missing than just missing ;-) > > >+ Notifications.remove(this.notification); > > [Ah, here's the other global scope abuser.] > I think you missed the 2x "Observers" in the destructor. ;-) [What I meant was that this was the other code that needed to access the globals set in that constructor.]
Attached patch notification v2 (obsolete) — Splinter Review
v2, since this puts the Sync notifications back in line with the other ones (e.g. pop-up blocking) by removing the <content> (as suggested by Neil) that switched the spacer and the button(s) so that effectively the button(s) appeared right next to the description instead of next to the close button, as with the other notifications. Also addressed the other nits from comment 31, of course. Except for the first question, which I cannot easily answer.
Attachment #497162 - Attachment is obsolete: true
Attachment #499354 - Flags: review?(neil)
Attachment #497162 - Flags: review?(neil)
(In reply to comment #32) > (In reply to comment #31) > > So, the difference here is that the notifications are all visible at the same > > time, rather than stacking as they do normally? Not really. At least that's not what we intended. Then again, I didn't write this code (I only ported it from the add-on to m-c) so maybe? > > >+ <xul:spacer/> > > [I don't actually know what the spacer does in the base binding either.] > > philiKON? Nope. > > >+ for each (var notification in Notifications.notifications) > > >+ this._appendNotification(notification); > > [As this is an array, shouldn't you be using > > Notifications.notifications.forEach(this._appendNotification, this); > > ?] > > Haven't checked. philiKON, anything speaking against that? Not really. Definitely more elegant this way :) > > >+ <!-- The children are the buttons defined by the notification. --> > > >+ <xul:hbox oncommand="document.getBindingParent(this)._doButtonCommand(event);"> > > >+ <children/> > > >+ </xul:hbox> > > >+ <xul:spacer flex="1"/> > > So the buttons are in the "middle", rather than on the right? Seems odd to me. > > The only button I could see, "Preferences..." (see scenario at the beginning of > this comment), is indeed at the end of the message. At the far right of the > notification bar is the red close button, and I'd rather not move any buttons > next to that one... The close button should probably move to the left to match the layout of the Find bar. In any case, the styling of those notification bars isn't done yet. Bug 598799 tracks that, it also refers to some rather ambitious mockups. Not sure that we'll get those in by Firefox 4, but at this point anything would be an improvement over what we've got right now. > > But in that case you don't need the spacer. (And you could move the oncommand > > back to the parent box and remove the box too.) > > I'd guess the spacer (with the flex) is needed to maintain the space between > the button at the end of the message and the close button at the far right of > the notification bar, no? Correct. Any <content> modifications compared to the base binding were made so that the notifications would match the mockups, at least structurally. As said above, we still have some styling to do (e.g. restyle buttons as links). This may even involve more changes to the XBL.
(In reply to comment #35) First of all, thanks for you comments! > The close button should probably move to the left to match the layout of the > Find bar. Well, for Firefox that might make sense, but our version of the find bar is at the top, so not as close to the notification as in Firefox. I think for us the visual compatibility with existing notification bars is more important. > > I'd guess the spacer (with the flex) is needed to maintain the space between > > the button at the end of the message and the close button at the far right of > > the notification bar, no? > > Correct. As Neil explained, that should work without the spacer, too. > Any <content> modifications compared to the base binding were made so > that the notifications would match the mockups, at least structurally. I see now that it matches the first window in attachment 471288 [details], so that's the explanation for why Firefox is doing it like that, thanks. But I agree with Neil that our goal should be to match existing notification bars, which have the buttons on the right.
Comment on attachment 499354 [details] [diff] [review] notification v2 >+<!DOCTYPE bindings [ >+<!ENTITY % notificationDTD SYSTEM "chrome://global/locale/notification.dtd"> >+%notificationDTD; >+]> [I wonder why none of our XBL files seem to use the one-line doctype.] >+ <content> >+ <xul:vbox xbl:inherits="hidden=notificationshidden"> >+ <children includes="notification"/> >+ </xul:vbox> >+ <children/> >+ </content> [So, do we need this? If not, we could get rid of it and the namespaces.] >+ localScope.Notifications.notifications.forEach(this._appendNotification, this); [Would it be silly to merge _appendNotification into onNotificationAdded?] >+ var notifications = this.allNotifications; >+ for each (var notification in notifications) { This is an array too, right? So in that case, another forEach please? [Fortunately you don't need "this" this time saving you a parameter.]
Attachment #499354 - Flags: review?(neil) → review+
Attached patch notification v2aSplinter Review
(In reply to comment #37) > >+ <content> > >+ <xul:vbox xbl:inherits="hidden=notificationshidden"> > >+ <children includes="notification"/> > >+ </xul:vbox> > >+ <children/> > >+ </content> > [So, do we need this? If not, we could get rid of it and the namespaces.] Removed, including namespaces. > >+ localScope.Notifications.notifications.forEach(this._appendNotification, this); > [Would it be silly to merge _appendNotification into onNotificationAdded?] Kept as-is, since I find it cleaner (single param which describes exactly what it is (otherwise "data" would be the index of the forEach run), API-compatibility with FF in case more is added there in the future). > >+ var notifications = this.allNotifications; > >+ for each (var notification in notifications) { > This is an array too, right? So in that case, another forEach please? Kept as-is as discussed via IRC (ability to break early; array is from .jsm so it doesn't matter much).
Attachment #499354 - Attachment is obsolete: true
Attachment #499831 - Flags: review+
Comment on attachment 497163 [details] [diff] [review] quota I'll be afk until at least Sunday, so take your time.
Attachment #497163 - Flags: review?
FYI: Bug 622134 is about an issue in the Setup dialog; cf. attachment 498911 [details] [diff] [review]. The relevant code may be different due to bug 602715 which is not in our initial set.
(In reply to comment #28) > (From update of attachment 497161 [details] [diff] [review]) > >+ /* Bug 575675: Need to have an a:visited rule in a chrome document. */ > >+ a:visited { color: purple; } > This makes no sense once you've saved it. I don't think we'd miss visited link > decoration on the printout (assuming this file is used for that too). D'oh, it hadn't occurred to me to read bug 575675. I stumbled over it because it aborts in addressbook UI too. Apparently a chrome document containing a visited link crashes, unless you have a :visited rule somewhere. Can we work around the problem by adding type="content" to the frame used to print?
(In reply to comment #41) > Apparently a chrome document containing a visited link crashes, unless you > have a :visited rule somewhere. More precisely, from bug 575675 comment 30: "The style set assumes there's at least one :visited rule aroud somewhere; if there is not the asserts will fail (and they're NS_ABORT_IF_FALSE, so look like a crash in a debug build)." So I cannot experience this unless I create a debug build, which I see no reason to do. > Can we work around the problem by adding type="content" to the frame used to > print? You tell me. I'd go for the simple solution of re-adding the :visited style rule, which would then also give us a more complete print result again. BTW: It only appeared to me now that, unlike the FF version, the comments in the XHTML file make it to the final result: FF uses pre-processing, we don't (due to the self-censoring I applied so I wouldn't have to do it in reaction to reviews). I think we should also use pre-processing for that file and convert the comments back to hash-prefixed lines.
Attached patch setup v1a (obsolete) — Splinter Review
(In reply to comment #40) > FYI: Bug 622134 is about an issue in the Setup dialog; cf. attachment 498911 [details] [diff] [review]. Removed the two lines as in attachment 498911 [details] [diff] [review] which landed for FF Sync, so Bug 622134 is obsolete now.
Attachment #497164 - Attachment is obsolete: true
Depends on: 619995
Attached patch setup v1b (obsolete) — Splinter Review
Incorporated changes from bug 620995. Almost looks like only the *995 ones get added here. ;-)
Attachment #501346 - Attachment is obsolete: true
Depends on: 620995
Attachment #497163 - Flags: review? → review?(neil)
Comment on attachment 497163 [details] [diff] [review] quota >+ caption.firstChild.nodeValue = this.bundle.getString("quota.treeCaption.label"); Nit: should use caption.textContent throughout (also allows you to use <description/>) >+ /* >+ * Handle click events on the tree. >+ */ [I think there's a better way of doing this using a cycler cell. See EdSelectProps.js for example. Remind me to look into it once this lands.] >+ <vbox flex="1"> This vbox is probably unnecessary. >+ flex="1"> [Not sure whether this is necessary, would have to test.] >+ <treecol id="enabled" >+ type="checkbox" >+ fixed="true"/> >+ <splitter class="tree-splitter"/> [This looks wrong but it probably turns out to be necessary.]
Attached patch quota v2Splinter Review
(In reply to comment #46) > >+ <vbox flex="1"> > This vbox is probably unnecessary. So it seems. > >+ flex="1"> > [Not sure whether this is necessary, would have to test.] The flex on the tree is needed, otherwise the tree is collapsed vertically. The flex on the treecols is needed, too, otherwise they only have a minimum width. The flex on the treechildren element seems to be unnecessary so I removed it (if you can imagine a situation where we would need it I can bring it back). > >+ <treecol id="enabled" > >+ type="checkbox" > >+ fixed="true"/> > >+ <splitter class="tree-splitter"/> > [This looks wrong but it probably turns out to be necessary.] Without the first splitter the column separator (second splitter) between Type and Size is not draggable (don't ask me why). Also incorporated the displayUsageData changes from bug 617150, so I'm calling it v2.
Attachment #497163 - Attachment is obsolete: true
Attachment #503271 - Flags: review?(neil)
Attachment #497163 - Flags: review?(neil)
Depends on: 617150
(In reply to comment #47) > (In reply to comment #46) > > >+ flex="1"> > > [Not sure whether this is necessary, would have to test.] > The flex on the tree is needed, otherwise the tree is collapsed vertically. Ah, I hadn't noticed that the dialog is given a size and the tree expands.
Attachment #503271 - Flags: review?(neil) → review+
Attachment #501951 - Flags: review?(neil)
Comment on attachment 501951 [details] [diff] [review] setup v1b >+ if (bits > 94) >+ meter.className = "strong"; >+ else if (bits > 47) >= >+ // if NoScript isn't installed, or is disabled, bail out. >+ let ns = Cc["@maone.net/noscript-service;1"]; Nit: use if ("@maone.net/noscript-service;1" in Cc) for the test. >+ document.getElementById(id).firstChild.nodeValue = >+ string ? self._stringBundle.GetStringFromName(string) : ""; Use .textContent (and this means we don't have those silly strings in the XUL, such as <description> </description> or <html:span>replace me</html:span>, you can just use an empty tag instead). >+ let action = document.getElementById("mergeChoiceRadio").selectedItem.id; The <radio> elements should use value= instead of id= then you can use .value instead of .selectedItem.id >+ _setFeedback: function (element, success, string) { >+ element.hidden = success || !string; >+ let class = success ? "success" : "error"; >+ let image = element.firstChild.nextSibling.firstChild; >+ image.setAttribute("status", class); >+ let label = image.nextSibling; >+ label.value = string; I'm not sure I understand this. If we have a success, then the element is hidden, so why bother setting the status? Also, you can use element.lastChild.firstChild for the image and possibly element.lastChild.lastChild for the label. >+ <description style="padding: 0 7em;"> Nit: style belongs in CSS >+ <label id="generatePassphraseButton" >+ value="&syncKeyGenerate.label;" >+ class="text-link inline-link" >+ onclick="event.stopPropagation(); >+ gSyncSetup.onPassphraseGenerate();"/> [Not keyboard accessible?] >+ <wizardpage id="useExisting" >+ label="&setup.existingAccount.title.label;" >+ onextra1="gSyncSetup.onSyncOptions();" >+ onpageshow="gSyncSetup.onPageShow();"> >+ <grid> Nit: incorrect indentation >+ <row id="existingPasswordFeedbackRow" align="center" hidden="true"> Nit: doubled space >+ <groupbox id="syncOptions"> >+ <grid> Nit: incorrect indentation >+ <column flex="1" style="-moz-margin-end: 2px"/> [Odd.] >+ <label value="&syncMy.label;" /> [No space necessary before /.] >+ <wizardpage id="syncOptionsConfirm" >+ label="&setup.optionsConfirmPage.title;" >+ onpageshow="gSyncSetup.onPageShow();"> >+ <deck id="chosenActionDeck"> Nit: incorrect indentation >+ <separator flex="1"/> >+ </wizardpage> >+</wizard> [Probably unnecessary separator.]
Update: I'm in the process of addressing comment 49 but stumbled over bug 587070 (again, but now with an extra twist!).
The changes from bug 622408 will be in the next patch (already done locally).
Depends on: 622408
Attached patch setup v2 (obsolete) — Splinter Review
(In reply to comment #49) > >+ let action = document.getElementById("mergeChoiceRadio").selectedItem.id; > The <radio> elements should use value= instead of id= then you can use .value > instead of .selectedItem.id Kept id in addition to value for the label onclick (cf. bug 587070 comment 6). > >+ _setFeedback: function (element, success, string) { > >+ element.hidden = success || !string; > >+ let class = success ? "success" : "error"; > >+ let image = element.firstChild.nextSibling.firstChild; > >+ image.setAttribute("status", class); > >+ let label = image.nextSibling; > >+ label.value = string; > I'm not sure I understand this. If we have a success, then the element is > hidden, so why bother setting the status? Well, for consistency I would say. Leaving the wrong status behind, just hidden, doesn't look better to me. (If the element was removed then OK, but it's only hidden! If future code or extensions somehow check this I'd rather not have to keep this in mind.) > >+ <description style="padding: 0 7em;"> > Nit: style belongs in CSS Will add this to the theming patch on the base bug with r=you if you r+ this and don't object: --- syncSetup.css 2011-01-14 22:27:23.303289100 +0100 +++ syncSetup.css 2011-01-14 22:16:49.680829300 +0100 @@ -96,7 +96,7 @@ } .inputColumn { - -moz-margin-end: 2px + -moz-margin-end: 2px; } #connect-throbber image, @@ -108,3 +108,7 @@ /* TODO replace this with a 128px version (bug 591122) */ list-style-image: url("chrome://communicator/skin/sync/sync-32.png"); } + +#pickSetupDesc { + padding: 0 7em; +} > >+ <label id="generatePassphraseButton" > >+ value="&syncKeyGenerate.label;" > >+ class="text-link inline-link" > >+ onclick="event.stopPropagation(); > >+ gSyncSetup.onPassphraseGenerate();"/> > [Not keyboard accessible?] If you're talking about accesskeys then many more might need ones. In that case I'd like to leave that for a follow-up bug in the interest of a quick initial Sync landing. Agreed? > >+ <column flex="1" style="-moz-margin-end: 2px"/> > [Odd.] Regarding -moz-margin-end: - introduced through bug 551612 (New Account wizard page) - moved to CSS class inputColumn in bug 571897 (since Theme patch 0.3) - introduced to Options wizard page through bug 590805 (Part 5) without using the class (cut & paste from older code?) Replaced it by class="inputColumn" (and added the missing semicolon to the CSS, see above). > >+ <separator flex="1"/> > >+ </wizardpage> > >+</wizard> > [Probably unnecessary separator.] So it seems.
Attachment #501951 - Attachment is obsolete: true
Attachment #503978 - Flags: review?(neil)
Attachment #501951 - Flags: review?(neil)
This patch should have come first, since I obviously needed to set up sync to test out the other patches... I guess I have to retest with a spare profile. (In reply to comment #52) > (In reply to comment #49) > > >+ let action = document.getElementById("mergeChoiceRadio").selectedItem.id; > > The <radio> elements should use value= instead of id= then you can use .value > > instead of .selectedItem.id > Kept id in addition to value for the label onclick (cf. bug 587070 comment 6). Eww. Maybe we need a way of supporting arbitrary content for radio/checkbox. In the meantime, focusing a radio element is wrong, and you can use this.previousSibling to avoid giving it an id. > > >+ <description style="padding: 0 7em;"> > > Nit: style belongs in CSS > Will add this to the theming patch on the base bug with r=you if you r+ this Looks OK. > > [Not keyboard accessible?] > If you're talking about accesskeys then many more might need ones. In that case > I'd like to leave that for a follow-up bug in the interest of a quick initial > Sync landing. Agreed? Actually I was thinking about tabbing to it and following the link with the keyboard. (An ordinary text-link element tries to follow the link when you press Enter or Return while it is focused.) > > >+ <column flex="1" style="-moz-margin-end: 2px"/> > > [Odd.] > Replaced it by class="inputColumn" Nice.
Attached patch setup v2aSplinter Review
(In reply to comment #53) > This patch should have come first, since I obviously needed to set up sync to > test out the other patches... I guess I have to retest with a spare profile. Right, sorry, didn't think about that. But then I'm using different profiles for testing the various stages here, too. > In the meantime, focusing a radio element is wrong, and you can use > this.previousSibling to avoid giving it an id. OK, but had to remove the control attributes from the labels, too, since they referred to the IDs. Also changed the control attribute value of label with ID existingAccountLabel to "existingAccountName" (was "existingAccount", which doesn't exist on that page). > > > [Not keyboard accessible?] > > If you're talking about accesskeys then many more might need ones. In that case > > I'd like to leave that for a follow-up bug in the interest of a quick initial > > Sync landing. Agreed? > Actually I was thinking about tabbing to it and following the link with the > keyboard. (An ordinary text-link element tries to follow the link when you > press Enter or Return while it is focused.) Oh, that works, it just has a bad affordance. :-( Reason: The following rule in syncSetup.css: /* Override the text-link style from global.css */ .text-link, .text-link:focus { margin: 0px; padding: 0px; border: 0px; } which (as the comment states) overrides the rules from globals.css--one of which does this: .text-link:-moz-focusring { border: 1px dotted -moz-DialogText; } Since I don't want to selectively reintroduce the border (the color would likely not be the one a theme like Modern would define) and having no border (0px) in the non-focused case implies some bouncing in the UI (e.g. due to height changes upon focusing), I'd simply remove the "border: 0px;" line altogether. Agreed? BTW: I found that the inline-link CSS class is unused (it's only defined in FF's preferences.css, so it doesn't even work for FF's Sync), but since it's also used in syncGenericChange.xul and doesn't really harm (maybe useful in the future) I didn't remove it. For DOMI debugging sanity (and extensibility, think CSS) I also added IDs for the two wizardpages that missed ones and changed the attribute orders to a consistent ID, label, onextra1, onpageshow.
Attachment #503978 - Attachment is obsolete: true
Attachment #504096 - Flags: review?(neil)
Attachment #503978 - Flags: review?(neil)
(In reply to comment #54) > > In the meantime, focusing a radio element is wrong, and you can use > > this.previousSibling to avoid giving it an id. > OK, but had to remove the control attributes from the labels, too, since they > referred to the IDs. The control attribute obviously wasn't working otherwise the onclick wouldn't have been needed ;-) > > > > [Not keyboard accessible?] > > > If you're talking about accesskeys then many more might need ones. In that case > > > I'd like to leave that for a follow-up bug in the interest of a quick initial > > > Sync landing. Agreed? > > Actually I was thinking about tabbing to it and following the link with the > > keyboard. (An ordinary text-link element tries to follow the link when you > > press Enter or Return while it is focused.) > Oh, that works It does? I didn't see an event handler for the Return key. > Since I don't want to selectively reintroduce the border (the color would > likely not be the one a theme like Modern would define) and having no border > (0px) in the non-focused case implies some bouncing in the UI (e.g. due to > height changes upon focusing), I'd simply remove the "border: 0px;" line > altogether. Agreed? Seems reasonable.
(In reply to comment #55) > > OK, but had to remove the control attributes from the labels, too, since they > > referred to the IDs. > The control attribute obviously wasn't working otherwise the onclick wouldn't > have been needed ;-) Sure I know, just wanted to note that I did more than you explicitly requested. > > > Actually I was thinking about tabbing to it and following the link with the > > > keyboard. (An ordinary text-link element tries to follow the link when you > > > press Enter or Return while it is focused.) > > Oh, that works > It does? I didn't see an event handler for the Return key. http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/text.xml#369 Almost as invisible as me on IRC. :-) Anything else? Looking at the introduction of your comment 53 I guess you want to give it some more testing.
(In reply to comment #56) > (In reply to comment #55) > > It does? I didn't see an event handler for the Return key. > http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/text.xml#369 Oops, I'd misread that as "this.open()" which obviously wouldn't work.
Comment on attachment 504096 [details] [diff] [review] setup v2a There are two alternatives for the radio buttons. Thoughts? The first is copy the mail send format dialog: <radio value="resetClient" label="&choice2.merge.main.label;" class="mergeChoiceButton"/> <label class="recommended" value="&choice2.merge.recommended.label;"/> ... <radio value="wipeClient" label="&choice2.client.main.label;" class="mergeChoiceButton"/> ... <radio value="wipeRemote" label="&choice2.server.main.label;" class="mergeChoiceButton"/> The second is to switch the labels in the example above, so that the word recommended is the label of the radio, and the label has the rest of the text.
(In reply to comment #58) > Comment on attachment 504096 [details] [diff] [review] > setup v2a > > There are two alternatives for the radio buttons. Thoughts? Your first alternative sounds (and looks) good: + behaves like a normal radio group (needs no tweaks for label clicks) + looks like a normal radio group (At least on my Kubuntu Linux, the radio button and label of an active radio button line with the label defined as an attribute shows embossed and with a blue underline. With a separate label, no effects are shown. That's probably due to the same reason why the label click needs a tweak.) - the "Recommended:" label needs to be changed and requires a new class (font-weight:bold). More subtle differences to Sync UI in FF (think localizers). ? aria-labelledby is not needed in this case AFAIU, but I don't know much about this topic! Your second alternative doesn't sound better since I guess that would have a11y implications (and it feels like using "here" as a link label in HTML). As for the "Recommended:" label change, which of the following would you advise? a) remove the colon b) convert it to lower case c) put it in parentheses (Multiple answers allowed, and not all are really optional. ;-))
Patch on top of "setup v2a". Maybe I'll get an answer to comment 59 this way...
Attachment #506259 - Flags: review?(neil)
Comment on attachment 506259 [details] [diff] [review] setup radio changes Compare line 4 of askSendFormat.properties ;-)
Attachment #506259 - Flags: review?(neil) → review+
Attachment #504096 - Flags: review?(neil) → review+
Hmm, we left some questions unanswered after comment 42...
(In reply to comment #42) > (In reply to comment #41) > > Can we work around the problem by adding type="content" to the frame used to > > print? > You tell me. Well as yet I don't actually know where the sync key printing code is... > I'd go for the simple solution of re-adding the :visited style > rule, which would then also give us a more complete print result again. But not as complete as a content frame, since that would use the user's colours. > BTW: It only appeared to me now that, unlike the FF version, the comments in > the XHTML file make it to the final result Maybe we can delete the comment in script before we serialise the page to disk.
Attached patch Sync key comments (obsolete) — Splinter Review
This is on top of the Sync key patch. (In reply to comment #63) > Well as yet I don't actually know where the sync key printing code is... Sorry, I thought you'd find it. Here it is (part of the "common" patch on the base bug): syncUtils.js: _preparePPiframe (iframe creation) passphrasePrint (print) passphraseSave (save) > > I'd go for the simple solution of re-adding the :visited style > > rule, which would then also give us a more complete print result again. > But not as complete as a content frame, since that would use the user's > colours. Hmm. If I add iframe.setAttribute("type", "content") in _preparePPiframe, both Print and Save stop working (but nothing in the Error Console). If I add iframe.type = "content" instead, they keep working but I don't know if it makes a difference (AFAICT the issue only appears on Mac, which I cannot test). The CSS rule might not be perfect, but it is the quickest and safest option, so I added it back. If you still want the alternative: Help wanted! > > BTW: It only appeared to me now that, unlike the FF version, the comments in > > the XHTML file make it to the final result > Maybe we can delete the comment in script before we serialise the page to disk. Good idea, implemented (including removal of any CSS-like comments; note: \s removes all leading whitespace, including the line break).
Attachment #507201 - Flags: review?(neil)
(In reply to comment #64) > Hmm. If I add iframe.setAttribute("type", "content") in _preparePPiframe, both > Print and Save stop working (but nothing in the Error Console). If I add > iframe.type = "content" instead, they keep working but I don't know if it makes > a difference (AFAICT the issue only appears on Mac, which I cannot test). I don't think the property works. The attribute has the side-effect that the listener gets thrown away when the new document loads. The workaround is to use a capturing event listener on the iframe instead e.g. iframe.removeEventListener("load", loadListener, true);
FYI: Changed the name of a variable called "class" to "classname" in syncSetup.js locally. Will update "dialogs v5" on the base bug including this case (and carry forward r+ for it) once we've sorted out the Sync Key changes here. I had updated the patches here as well, but "setup v2a" is incomplete alone and "setup radio changes" includes changes to other files so it's much effort vs. little benefit. Cf. bug 628893 comment 3 / bug 497869 / http://hg.mozilla.org/mozilla-central/rev/b21d0f75e50a
Comment on attachment 507201 [details] [diff] [review] Sync key comments >+ let regexp = new RegExp("^<!-- " + stars + " BEGIN LICENSE BLOCK " + stars + >+ "(.|\\n)*" + stars + " END LICENSE BLOCK " + stars + >+ " -->$", "m"); MDN suggests using [\s\S] [Actually I was thinking you could do DOM manipulation on the document to remove the comment.]
(In reply to comment #67) > MDN suggests using [\s\S] Had that first, but then used the same syntax as a few lines below (DOCTYPE change). Anyway, I think that's obsolete now: > [Actually I was thinking you could do DOM manipulation on the document to > remove the comment.] I have to admit I didn't think that far. Now that I checked with DOMI and found that it's just a single node from the DOM POV, I added this to _preparePPiframe locally instead: // Remove the license block. let node = iframe.contentDocument.firstChild.nextSibling; if (node && node.nodeType == Node.COMMENT_NODE) iframe.contentDocument.removeChild(node);
(In reply to comment #68) > let node = iframe.contentDocument.firstChild.nextSibling; Err, forget about the .nextSibling. The <?xml header doesn't appear in the DOM...
Verified: - no license block in the saved XHTML file - links underlined in the print output (printed to PDF) :-)
Attachment #507201 - Attachment is obsolete: true
Attachment #507538 - Flags: review?(neil)
Attachment #507201 - Flags: review?(neil)
Attachment #507538 - Flags: review?(neil) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
Component: UI Design → Sync UI
QA Contact: ui-design → sync-ui
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: