249359 - Minor UI clean-up for the Roaming Preferences panes

Status: RESOLVED FIXED

(Core Graveyard :: Profile: Roaming, defect)

Description

[Stefan Borggraefe]

The patch I'm going to attach fixes the following minor UI issues:

1. Add a "Browse..." button to the File Copy <deck>. (See bug 81572)
2. Fix duplicate access key for _U_sername and Base _U_RL
3. Correct case of accesskeys (See
http://www.mozilla.org/projects/ui/accessibility/accesskey.html)
4. Add colons to some labels (consistency with the majority of Mozilla's UI)
5. Remove groupbox from Item Selection <listbox> for consistency with similar
<listbox>es in the Preferences window
6. Fix checkboxes to be only clickable on their text (See bug 244413)
7. Remove unused function EnableElement(...)
8. Fix some JavaScript strict warnings I noticed

Comment 1

[Stefan Borggraefe]

Attachment: Patch

Comment 2

[Stefan Borggraefe]

Comment on attachment 152097 [details] [diff] [review]
Patch

Ben: Do you agree with all changes I made?

Comment 3

[Ben Bucksch (:BenB)]

Comment on attachment 152097 [details] [diff] [review]
Patch

> +    <label value="&files.label;" />

I'd completely remove this, then.

Comment 4

[Ben Bucksch (:BenB)]

Comment on attachment 152097 [details] [diff] [review]
Patch

Patch looks good, r=BenB. A few (non-blocking) comments:

+    <label value="&files.label;" />

I'd completely remove this, then.

+  catch (ex) {
+    dump("initWithPath failed.");
+  }

Include ex in output

-      var checkbox = children[i2];
+      checkbox = children[i2];

Why that? This looks wrong (at least when looking at the context in the diff)

Comment 5

[Stefan Borggraefe]

Attachment: Patch V1.1

> -	 var checkbox = children[i2];
> +	 checkbox = children[i2];
> 
> Why that? This looks wrong (at least when looking at the context in the diff)


checkbox was declared earlier already (not included in the diff). So declaring
it again at this point causes a JavaScript strict warning.

I addressed the other two review comments.

Comment 6

[Stefan Borggraefe]

Comment on attachment 161656 [details] [diff] [review]
Patch V1.1

Moving forward BenB's r+ and asking Neil for sr.

Comment 7

[Ben Bucksch (:BenB)]

> checkbox was declared earlier already

I just don't see where:
<http://lxr.mozilla.org/seamonkey/source/extensions/sroaming/resources/content/prefs/files.js#138>.
I see no applicable definition of checkbox, only that inside the earlier for
loop, and I thought that the block is its own scope and variables declared
inside that for block are not declared outside. Would be helpful, if the warning
would also tell the original declaration, but it doesn't.

Just wondering, if we're overwriting another, global var or if I misunderstood
the JS language rules. Feel free to ignore me.

Comment 8

[Ben Bucksch (:BenB)]

Ah, found it in the spec. Seems like JS differs from C++ here.

<http://www.mozilla.org/js/language/E262-3.pdf>, Chapter 12.2. says:
"If the variable statement occurs inside a FunctionDeclaration, the variables
are defined with function-local scope in that function, as described in section
10.1.3. Otherwise, they are defined with global scope [...] Variables are
created when the execution scope is entered. A Block does not define a new
execution scope. Only Program and FunctionDeclaration produce a new scope."

Esp. the last 2 sentences are pretty clear.

Comment 9

[[e-mail]]

Comment on attachment 161656 [details] [diff] [review]
Patch V1.1

>+<!ENTITY streamURL.label          "Base URL:">
>+<!ENTITY streamURL.accesskey      "a">
I can't wait for that deck access key bug to be fixed, then we can change this
to the more natural B.

>@@ -449,8 +440,34 @@ function InitElement(elementID)
>   var e = document.getElementById(elementID);
>   eval(e.getAttribute("oncommand").replace(/this/g, "e")); //hackish, but WFM
> }
This function doesn't appear to be used either, but for future information,
please use e.doCommand(); to fire the oncommand attribute.

Comment 10

[Stefan Borggraefe]

Fix checked in. Thanks for the reviews. :-) I filed bug 263774 for removal of
the other unused function.