643640 - Make status labels in SyncUI wrap for better L10n support

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

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.

Comment 1

[Philip Chee]

Jens would you be willing to mentor this bug?

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(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.

Comment 3

[Medka]

Attachment: ahmed1.patch

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

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.

Comment 5

[Philip Chee]

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.

Comment 6

[Medka]

Attachment: New patch: Only change text labels.

Comment 7

[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

Comment 8

[Philip Chee]

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.

Comment 9

[Medka]

Attachment: Patch 3: Updated |this._status.value| to |this._status.textContent|

Comment 10

[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.

Comment 11

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/4471d03fbfc9