Closed Bug 643640 Opened 9 years ago Closed 4 years ago

Make status labels in SyncUI wrap for better L10n support

Categories

(SeaMonkey :: Sync UI, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.45

People

(Reporter: InvisibleSmiley, Assigned: medka_15)

References

Details

(Whiteboard: [good first bug][lang=js][level=beginner])

Attachments

(3 files)

In bug 632952 I discovered that the contents of the various status labels used in Sync UI dialogs and wizards do not wrap, which is a potential l10n issue: Dialogs, which are not resizable, may have their text contents cut off.

There are at least two possible solutions, as described in bug 632952 comments 17 and 19:
a) use <description> instead of <label> tags in XUL
b) use .textContent instead of .value (and remove any value=" ")

Neil seems to prefer the second approach so it should be used unless it imposes other problems.
Jens would you be willing to mentor this bug?
Whiteboard: [good first bug][lang=js]
(In reply to Philip Chee from comment #1)
> Jens would you be willing to mentor this bug?

Sure! Note that depending on which approach is taken (see initial comment), the language will either be JS or XUL.
Whiteboard: [good first bug][lang=js] → [good first bug][mentor=InvisibleSmiley][lang=js]
Summary: Make status labels wrap for better l10n support → Make status labels in SyncUI wrap for better L10n support
Whiteboard: [good first bug][mentor=InvisibleSmiley][lang=js] → [good first bug][mentor=InvisibleSmiley][lang=js][level=beginner]
Mentor: jh
Whiteboard: [good first bug][mentor=InvisibleSmiley][lang=js][level=beginner] → [good first bug][lang=js][level=beginner]
Attached patch ahmed1.patchSplinter Review
Attachment #8696299 - Flags: review?(jh)
Mentor: jh
Comment on attachment 8696299 [details] [diff] [review]
ahmed1.patch

Sorry, totally forgot about those mentoring bugs. I haven't been active in SM development in quite a long time and cannot tell whether the patch you attached works, especially since Sync is currently broken on trunk (since Mozilla disabled the old implementation that SM still uses). The patch looks OK to be (matching what was requested in this bug) but I'd rather have someone look at it who is still more active than me, thus referring to Neil.
Attachment #8696299 - Flags: review?(jh) → review?(neil)
Comment on attachment 8696299 [details] [diff] [review]
ahmed1.patch

A good first attempt. But a wee bit too over broad.

> -          this._passphraseBox.value = pp;
> +          this._passphraseBox.textContent = pp;

_passphraseBox is a textbox not a label. Please don't change things that are not labels.

> -      Weave.Service.identity.basicPassword = this._firstBox.value;
> +      Weave.Service.identity.basicPassword = this._firstBox.textContent;
Not a label.

> -          <label id="status" class="status" value=" "/>
> +          <label id="status" class="status" />

> -            <label class="status" value=" "/>
> +            <label class="status" />
(multiple instances seen)

I think that the intent here is to have a non empty value. Please leave these alone.


> -    Weave.Service.identity.account = document.getElementById("existingAccountName").value;
> -    Weave.Service.identity.basicPassword = document.getElementById("existingPassword").value;
> +    Weave.Service.identity.account = document.getElementById("existingAccountName").textContent;
> +    Weave.Service.identity.basicPassword = document.getElementById("existingPassword").textContent;
> -    el.value = Weave.Utils.hyphenatePassphrase(passphrase);
> +    el.textContent = Weave.Utils.hyphenatePassphrase(passphrase);
These are not labels.

> -    let action = document.getElementById("mergeChoiceRadio").value;
> +    let action = document.getElementById("mergeChoiceRadio").textContent;
This is not a label

> -      document.getElementById("weaveEmail").value);
> +      document.getElementById("weaveEmail").textContent);
This is not a label

> -    if (password.value == document.getElementById("weavePassphrase").value) {
> +    if (password.textContent == document.getElementById("weavePassphrase").textContent) {
> -    el.value = Weave.Utils.hyphenatePassphrase(passphrase);
> +    el.textContent = Weave.Utils.hyphenatePassphrase(passphrase);
This is not a label

> -        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> +        document.getElementById("syncComputerName").textContent = Weave.Service.clientsEngine.localName;
This is not a label

> -          return node && node.value;
> +          return node && node.textContent;
Don't change this.

> -        let password = document.getElementById("weavePassword").value;
> +        let password = document.getElementById("weavePassword").textContent;
> -          document.getElementById("weaveEmail").value);
> +          document.getElementById("weaveEmail").textContent);
These are not labels.

> -          document.getElementById("existingAccountName").value);
> -        Weave.Service.identity.basicPassword = document.getElementById("existingPassword").value;
> -        let pp = document.getElementById("existingPassphrase").value;
> +          document.getElementById("existingAccountName").textContent);
> +        Weave.Service.identity.basicPassword = document.getElementById("existingPassword").textContent;
> +        let pp = document.getElementById("existingPassphrase").textContent;
These are not labels.

> -        document.getElementById("easySetupPIN1").value = pin.slice(0, 4);
> -        document.getElementById("easySetupPIN2").value = pin.slice(4, 8);
> -        document.getElementById("easySetupPIN3").value = pin.slice(8);
> +        document.getElementById("easySetupPIN1").textContent = pin.slice(0, 4);
> +        document.getElementById("easySetupPIN2").textContent = pin.slice(4, 8);
> +        document.getElementById("easySetupPIN3").textContent = pin.slice(8);
These are not labels.

> -      control.value = "";
> +      control.textContent = "";
Notalabel.

> -    if (el.value) {
> +    if (el.textContent) {
Notalabel.

>    _validateServer: function (element, checkRemote) {
>      let valid = false;
> -    let val = element.value;
> +    let val = element.textContent;
> -      element.value = Weave.Service.serverURL;
> +      element.textContent = Weave.Service.serverURL;
Notalabel.

> -    let action = document.getElementById("mergeChoiceRadio").value;
> +    let action = document.getElementById("mergeChoiceRadio").textContent;
Notalabel.

> -          document.getElementById("historyCount").value =
> +          document.getElementById("historyCount").textContent =
> -          document.getElementById("bookmarkCount").value =
> +          document.getElementById("bookmarkCount").textContent =
> -          document.getElementById("passwordCount").value =
> +          document.getElementById("passwordCount").textContent =
> -          document.getElementById("addonCount").value =
> +          document.getElementById("addonCount").textContent =
These are labels. But I don't think it's necessary to wrap them.

>        <textbox id="easySetupPIN1"
>                 class="pin"
> -               value=""
>                 size="4"
>                 disabled="true"/>
>        <textbox id="easySetupPIN2"
>                 class="pin"
> -               value=""
>                 size="4"
>                 disabled="true"/>
>        <textbox id="easySetupPIN3"
>                 class="pin"
> -               value=""
>                 size="4"
>                 disabled="true"/>
These are not labels.

> -    Weave.Service.clientsEngine.localName = input.value;
> -    input.value = Weave.Service.clientsEngine.localName;
> +    Weave.Service.clientsEngine.localName = input.textContent;
> +    input.textContent = Weave.Service.clientsEngine.localName;
These are not labels.

>    _preparePPiframe: function(elid, callback) {
> -    let pp = document.getElementById(elid).value;
> +    let pp = document.getElementById(elid).textContent;
Notalabel.

>    validatePassword: function (el1, el2) {
>      let valid = false;
> -    let val1 = el1.value;
> -    let val2 = el2 ? el2.value : "";
> +    let val1 = el1.textContent;
> +    let val2 = el2 ? el2.textContent : "";
These are not labels.
Attachment #8696299 - Flags: review?(neil)
Attachment #8696542 - Flags: review?(philip.chee)
Comment on attachment 8696542 [details] [diff] [review]
New patch: Only change text labels.

> # Parent  78ea4c0d1bf585a85d4720e34fb676e1e5e50cc7
> use .textContent instead of .value
Commit message should have the bug number and a description of what the patch does. E.g.
Bug 643640 - Make status labels in SyncUI wrap for better L10n support
Attachment #8696542 - Attachment description: ahmed5.patch → New patch: Only change text labels.
Assignee: nobody → medka_15
Status: NEW → ASSIGNED
Comment on attachment 8696542 [details] [diff] [review]
New patch: Only change text labels.

OK. It looks like you've remove too much from the previous patch. Reading through the bug I think the only elements you need to change is the feedback/status section:

>       <vbox id="feedback" pack="center">
>         <hbox id="statusRow" align="center">
>           <image id="statusIcon" class="statusIcon"/>
>           <label id="status" class="status" value=" "/>
>         </hbox>
>       </vbox>

In syncGenericChange.js this corresponds to:
> this._status

So change each of |this._status.value| to |this._status.textContent|

And please remember to reference the bug number in your commit comment.
Please submit a new patch with the above changes.

Thank you.
Flags: needinfo?(medka_15)
Attachment #8696542 - Flags: review?(philip.chee) → review-
Flags: needinfo?(medka_15)
Attachment #8697451 - Flags: review?(philip.chee)
Comment on attachment 8697451 [details] [diff] [review]
Patch 3: Updated |this._status.value| to |this._status.textContent|

r=me. Thanks very much and apologies for the extreme delay
I'm going to check this in directly instead of waiting for checkin-needed due to being overdue.
Attachment #8697451 - Flags: review?(philip.chee) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/4471d03fbfc9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.45
You need to log in before you can comment on or make changes to this bug.