Closed Bug 840144 Opened 7 years ago Closed 6 years ago

Make about:config better

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified

People

(Reporter: wesj, Assigned: capella)

Details

Attachments

(3 files, 5 obsolete files)

about:config is kinda hard to use in its current form. We never fix it because, for the most part, people don't need it. But some day I'd like to make it feel like a right proper webapp.
What types of things would you want to do to make it better? This could be a good project for a motivated contributor.
Yeah. I've been playing with this a bit in my spare time, but there are a few parts I guess:

1.) Fix the styling. We should have a nice fixed position toolbar at the top like our other about pages (well... they're not fixed position, but they should be!). The buttons and search boxes we have currently should sit on the toolbar.

2.) Show a real list. Fill it in with values as you scroll.

3.) Get rid of dialogs where possible. You should be able to tap on a row and edit it right in the page. Number inputs should have an editable box, as well as buttons to increment/decrement them. Booleans should have an easy toggle button.

4.) Streamline create a new pref. Can we get rid of the dialogs here? At the very least it should be one simple dialog. Since you have to be able to flip between string/bool/int, it may be best to just write some custom in content ui.

5.) Responsive design. It doesn't need to be crazy multi-pane or anything, but a max width that keeps it from looking awful on tablets would be nice.
Attached patch WIP Patch (obsolete) — Splinter Review
This is a WIP I had started.
Would you want this work to supercede the bug 770101 we'd started? As-in... can we cancel / close that bugzilla and just do the bigger re-work under this # ?
I do kinda want to do that. You interested in finishing this up?
I forget to add that I'd love to continue in that manner... yes!
Blocks: 770101
I'd love the ability to be able to specify a filter in the address bar which would be executed on load - something like 'about:config?q=devtools' - also the ability to automatically toggle settings from the address bar 'about:config?devtools.debugger.log=true' would be handy although possibly a security risk - perhaps this could be an option which could be turned on manually if needed?
fwiw ... I'm tinkering with this on the side ... my javascript isnt as skilled as someone elses might be so I'm not moving fast, and am leaving this open for anyone who might want to jump in ...

let me know if you're interested ... I can pass over my current patch and short list of outstanding issues / project plan, otherwise I'm still hacking along
I changed my mind, I'll see if I can get this finished.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Well, as I mentioned, Javascript isn't my strong point, but here's what I consider my first ROUGH draft after your WIP got me started, with all the usual apologies for style, amateur mistakes, and etc.

I've left all my debug code in place in case you want to explore any fine points you might notice. (I'll remove them later of course).

Let me know :D
Attachment #738755 - Flags: feedback?(wjohnston)
Push build out to try server ... must find tester ....
https://tbpl.mozilla.org/?tree=Try&rev=589c049fdcf1
Attached patch Patch (v2) (obsolete) — Splinter Review
Fix bug when clearing filter using input button, removed show / hide of filter input field, remove show / hide of Filter Clear Button, remove some debug code, correct scroll to top after filter clear, finished Context menu copy name / copy value functions.

I couldn't change it to simply load the whole list each time, 1400+ items took too long to build a screen for ... so I bumped the buffer size from 50 to 100, it still loads fast and we get half the exposures to the throbber doing its thing.

We're you thinking of letting the UI hackathon group play further? I think most of the major code / functionality is in place ... but any further facelifts can only help :D
Attachment #738755 - Attachment is obsolete: true
Attachment #738755 - Flags: feedback?(wjohnston)
Attachment #741765 - Flags: review?(wjohnston)
Comment on attachment 741765 [details] [diff] [review]
Patch (v2)

I asked margaret to look at this too (since I wrote tons of it, and also she's a js/css wizard).
Attachment #741765 - Flags: review?(margaret.leibovic)
Comment on attachment 741765 [details] [diff] [review]
Patch (v2)

Review of attachment 741765 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is pretty big... one comment I have is that it would be really nice if this was split into two patches. One where the JS is moved into its own file, followed by another where it's modified, since right now it's hard to tell what's new vs. modified. Breaking it down into even more patches than that would be extra nice.

A general comment is that we use 2-space indentation for JS, not 4-space. For CSS, we try to avoid descendant selectors in favor of child selectors, although it's nice if we can avoid those altogether if they're not necessary.

I started off making some quick drive-by comments, but then ended up making a lot. I'll need some more time to really go through this (big!) patch, but I think there's plenty for you to work on here.

::: mobile/android/chrome/content/config.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

You should add "use strict"; to the top of the file. We try to use strict mode for all new JS files.

@@ +24,5 @@
> +// ***                                                                                                             *** //
> +// *** New Preference Dialog Object and methods                                                                    *** //
> +// ***                                                                                                             *** //
> +// ******************************************************************************************************************* //
> +-->

<!-- --> isn't valid JS comment syntax, I'm surprised this doesn't throw an error. This comment is a bit much, I think you should slim it down to something like:

/**
 * New Preference Dialog Object and methods 
 */

And maybe add a line about what this actually does.

@@ +26,5 @@
> +// ***                                                                                                             *** //
> +// ******************************************************************************************************************* //
> +-->
> +
> +var NewPrefDialog = {

I don't like all of the getters for elements in here, it feels like too much misdirection. I think it would be better to just make local variables where necessary. Or you could just store them in properties if they're not going to be changing.

Also, it's probably just a style nit of mine, but I prefer to prepend "_" to properties if they're only going to be used within the object, since that's a JS convention for indicating something is "private".

@@ +34,5 @@
> +    },
> +
> +    get shield() {
> +        delete this.shield;
> +        return this.shield = document.getElementsByClassName("shield")[0];

If there's only one specific element you want to get back, you should give it an id and use getElementById, instead of a class name.

@@ +73,5 @@
> +
> +        this.boolType.style.display = this.typeSelect.value == "boolean" ? "block" : "none";
> +        this.boolToggle.style.display = this.typeSelect.value == "boolean" ? "block" : "none";
> +        this.stringType.style.display = this.typeSelect.value == "string" ? "block" : "none";
> +        this.intType.style.display = this.typeSelect.value == "int" ? "block" : "none";

Let's do this with CSS instead of in the JS. You can set an attribute on the "pref-item" element, and use that to make CSS rules to decide what things to show/hide.

@@ +99,5 @@
> +    get positiveButton() {
> +        return this.dialog.getElementsByClassName("create")[0];
> +    },
> +
> +    set positiveButton(aPrefName) {

It's confusing that this setter isn't setting the value that's returned by the getter. It seems like a getter/setter isn't appropriate for this situation.

@@ +115,5 @@
> +            return;
> +        }
> +
> +        // If item already in list, it's being changed, else added
> +        let item = document.querySelector(".pref-item[name=\"" + aPrefName + "\"]");

Instead of manually making a quote, use quote(). So this would look like: ".pref-item[name=" + aPrefName.quote() + "]"

@@ +124,5 @@
> +        }
> +    },
> +
> +    toggleShowHide: function AC_toggleShowHide() {
> +        console.log(LOGTAG + " - NewPrefDialog.toggleShowHide() hears a call");

I don't think we need so much logging. Perhaps make a debug() helper function that will log things if some DEBUG flag is true.

@@ +136,5 @@
> +        this.shield.style.display = "block";
> +        setTimeout((function() {
> +            this.dialog.classList.add("show");
> +            this.shield.classList.add("show");
> +        }).bind(this), NEW_PREFS_SHOW_DELAY);

Why do you need this setTimeout?

@@ +163,5 @@
> +
> +        window.removeEventListener("keypress", this.keypress, false);
> +    },
> +
> +    keypress: function AC_keypress(aEvent) {

Nit: Name this something like "handleKeypress" to indicate this is an event handler.

@@ +182,5 @@
> +        this.positiveButton = aEvent.target.value;
> +    },
> +
> +    toggleBoolValue: function AC_toggleBoolValue() {
> +        this.boolType.value = (this.boolType.value == "true" ? "false" : "true");

Do these need to be strings? If not, you could just do |this.boolType.value = !this.boolType.value;|

@@ +209,5 @@
> +    },
> +
> +    cancel: function AC_cancel(aEvent) {
> +        console.log(LOGTAG + " - NewPrefDialog.cancel() hears a call");
> +        this.hide();

Why not make whatever calls this just call hide() directly instead?

@@ +432,5 @@
> +
> +         // Skip non-boolean prefs
> +        if (pref.type != Services.prefs.PREF_BOOL) {
> +            return;
> +        }

You can combine these two if statements into one (using an ||). Also, nit: brace single-line ifs everywhere or nowhere (just be consistent :).

@@ +516,5 @@
> +                gClipboardHelper.copyString(this.selected.getAttribute('name'));
> +                break;
> +            case 'value':
> +                gClipboardHelper.copyString(this.selected.getAttribute('value'));
> +        }

Why not just gClipboardHelper.copyString(this.selected.getAttribute(aField))? Also, aMouseEvent is unused.

@@ +582,5 @@
> +    test: function AC_test(aValue) {
> +        return aValue ? aValue.test(this.name) : true;
> +    },
> +
> +    getHTML: function AC_getHTML() {

getHTML isn't a good name for this because this doesn't return HTML, it returns a DOM node. Perhaps getElement() would be better.

@@ +607,5 @@
> +                        gStringBundle.GetStringFromName("pref.toggleButton") +
> +                    "</div>" +
> +                    "<div class='pref-button up' onclick='AboutConfig.incrDecrValue(event, 1);'></div>" +
> +                    "<div class='pref-button down' onclick='AboutConfig.incrDecrValue(event, -1);'></div>" +
> +                "</div>";

I don't like using .innerHTML. I know creating all this with DOM APIs would be a lot of code, so maybe instead you could include this in the HTML as a hidden node, then clone it to make new elements. However, creating this with DOM APIs would let you add event listeners in JS instead of in the HTML, which would be nicer.

@@ +626,5 @@
> +
> +        switch(this.type) {
> +            case Services.prefs.PREF_BOOL:
> +                valDiv.setAttribute("type", "text");
> +                valDiv.setAttribute('disabled', true);

Nit: Use double quotes, not single quotes.

::: mobile/android/chrome/content/config.xhtml
@@ +25,1 @@
>  <body dir="&locale.dir;" onload="AboutConfig.init();" onunload="AboutConfig.uninit();">

I know that the current code uses onfoo listeners in the HTML, but now that this is getting more complicated, I would prefer if we add all our event listeners in JS. It will make it easier to read the JS logic.

@@ +46,3 @@
>  
> +        <div id="new-pref-container">
> +            <li class='pref-item'>

Nit: Use double quotes.

@@ +84,4 @@
>  
> +    <menu type="context" id="prefs-context-menu">
> +        <menuitem label="Copy selected pref Name" onclick="AboutConfig.clipboardCopy(event, 'name');"></menuitem>
> +        <menuitem label="Copy selected pref Value" onclick="AboutConfig.clipboardCopy(event, 'value');"></menuitem>

These strings need to be localized.

::: mobile/android/themes/core/config.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +html,
> +body {
> +    margin: 0px;

Nit: Just use 0, no units necessary. Same comment applies everywhere 0 is specified.

@@ +8,5 @@
> +    padding: 0px;
> +    background-color: #ced7de;
> +    font-family: sans-serif;
> +    -moz-user-select: none;
> +    font-family: Roboto,"Droid Sans",helvetica,arial,clean,sans-serif;

You declared font-family in here twice. I also believe we prefer using "Open Sans" now. Probably font-family:"Open Sans",sans-serif; would be sufficient.

@@ +9,5 @@
> +    background-color: #ced7de;
> +    font-family: sans-serif;
> +    -moz-user-select: none;
> +    font-family: Roboto,"Droid Sans",helvetica,arial,clean,sans-serif;
> +    font-size: 18px;

I think you should use em for font-size, since it seems like you're using that elsewhere for height values. We should probably just be consistent on this.

@@ +115,1 @@
>  }

Nit: Put general element styles like this together at near the top of the file.

@@ +135,5 @@
>  }
>  
> +li {
> +    list-style-type: none;
> +    border-bottom: 1px solid lightgray;

Nit: Use hex values for colors. It's easy to know what white and black are, but it's less obvious for something like lightgray.

@@ +143,4 @@
>  }
>  
> +.pref-item .pref-name,
> +.pref-item .pref-value {

Are there any .pref-name or .pref-value elements that aren't children of a .pref-item element? If not, you could get rid of the .pref-item part of the selector.

@@ +184,5 @@
> +    display: flex;
> +    flex-direction: row;
> +}
> +
> +.pref-button.up {

I would prefer we just use unique class names, rather than put together two class names.

@@ +196,5 @@
> +    background-position: center center;
> +    background-repeat: no-repeat;
> +}
> +
> +#new-pref-container .pref-buttons .pref-button {

Same comment here about .pref-button parent elements. It would be nice if we could just set these styles on .pref-button.

@@ +276,5 @@
> +    transition-duration: 500ms;
> +    display: none;
> +}
> +
> +.shield.show {

Instead of a classname, you could just set an attribute on the .shield element to do this.
Attachment #741765 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch (v3) (obsolete) — Splinter Review
Posting this update .... the majority of the review issues are addressed ... I'm still hacking around the edges a little but am starting to look at some other things ... ( I've been toying with this since February and frankly boredom sets in :D )
Quick push to try for testable builds
https://tbpl.mozilla.org/?tree=Try&rev=cbf5eb588a78

I see Robocop-2 tests turning orange, it looks like | testSystemPages | has a test for about:config ... looks like we've got something there to fix before we go to m-c
... ?

Odd that in a second push just for RC-2: https://tbpl.mozilla.org/?tree=Try&rev=d99891994458 one of the tests goes green ... ??
Comment on attachment 746200 [details] [diff] [review]
Patch (v3)

So what's to do with this guy I had been wondering. I suppose starting with another review request ... I believe I finished everything you pointed out ... only thing I remember punting on was:

|I don't like using .innerHTML. I know creating all this with DOM APIs would be a lot of code, so maybe instead you could include this in the HTML as a hidden node, then clone it to make new elements. However, creating this with DOM APIs would let you add event listeners in JS instead of in the HTML, which would be nicer.|

iirc, I wasn't sure of the benefit of your idea over the code as-is, and further unsure how much user demand for the patch in general exists in any case (as we irq-d) ie: was it worth much more time/attention if no one is behind this)?

But let's kick it back to you for now (tag you're it) ...
Attachment #746200 - Flags: review?(margaret.leibovic)
Attachment #741765 - Flags: review?(wjohnston)
Attachment #746200 - Flags: review?(margaret.leibovic)
No longer blocks: 770101
Comment on attachment 746200 [details] [diff] [review]
Patch (v3)

I talked to you at summit about this. I'm going to take this review.
Attachment #746200 - Flags: review?(wjohnston)
This has been on the shelf for a bit, and I haven't had a chance to rebase (if req'd) and test it yet ... but I'm working in that direction :)
Attached patch bug840144 (v4) (obsolete) — Splinter Review
Rebased and re-posted for you to look at, catch up to where we left things... I did some quick testing, and there's some minor UI love needed (text sizes, etc) but you'll notice that ... when I last posted it there were no issues I was aware of, but we'll definately need to expose it to use
Attachment #814642 - Flags: feedback?
Comment on attachment 814642 [details] [diff] [review]
bug840144 (v4)

Ouch! threw my feedback to the wind
Attachment #814642 - Flags: feedback? → feedback?(wjohnston)
Comment on attachment 814642 [details] [diff] [review]
bug840144 (v4)

Review of attachment 814642 [details] [diff] [review]:
-----------------------------------------------------------------

Wow. I did forget how big this was. I gave it a first pass over. I'll probably find more if I do it again, but some comments. If you can make the icons look a little less... reszied, maybe you could throw a build up for ibarlow?

::: mobile/android/chrome/content/config.js
@@ +5,5 @@
> +
> +const {classes: Cc, interfaces: Ci, manager: Cm, utils: Cu} = Components;
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const LOGTAG = "aboutConfig";   // Debugging Tag

I don't think you use this (and we never do this in js anyway).

@@ +29,5 @@
> +
> +  _newPrefsDialog: null,
> +  _newPrefItem: null,
> +  _newPrefName: null,
> +  _newPrefType: null,

Its a bit of a guessing game what these are. Can we name them something like _prefTypeSelectElt. _prefNameInputElt

@@ +90,5 @@
> +
> +    // If item already in list, it's being changed, else added
> +    let item = document.querySelector(".pref-item[name=" + aPrefName.quote() + "]");
> +    if (item) {
> +      this._positiveButton.textContent = gStringBundle.GetStringFromName("newPref.changeButton");

If this changes to change, where do we flip it back? Also, it stays disabled here.

@@ +106,5 @@
> +  },
> +
> +  _show: function AC_show() {
> +    // Show new prefs dialog and the prefs list shield
> +    this._prefsShield.style.display = "block";

I think you could remove this and just set raised here.

@@ +126,5 @@
> +  hide: function AC_hide() {
> +    // Hide new prefs dialog and the prefs list shield
> +    this._newPrefsDialog.classList.remove("show");
> +    this._prefsShield.removeAttribute("raised");
> +    this._prefsShield.style.display = "none";

Same here. We'll just hide this unless "raised" (maybe we should rename that to "shown"?)

@@ +204,5 @@
> +    // Clear any previous selection and disable context menu
> +    if (currentSelection) {
> +      currentSelection.classList.remove("selected");
> +      currentSelection.removeEventListener("keypress", this._handleKeypress, false);
> +      this._prefsContainer.removeAttribute("contextmenu");

I would rather make this context menu stuff work on whatever row you tap, whether its selected or not. For that to work right, we should probably add an :active style for the rows.

@@ +227,5 @@
> +    // Init the NewPref dialog
> +    NewPrefDialog.init();
> +
> +    // Init the main AboutConfig dialog
> +    this._filterContainer = document.getElementById("filter-container");

This ain't used.

@@ +375,5 @@
> +  editValue: function AC_editValue(aEvent) {
> +    // Skipped locked, or boolean
> +    let pref = this._getPrefForEvent(aEvent);
> +    if (pref.locked || pref.type == Services.prefs.PREF_BOOL) {
> +      return;

I think we should probably toggle this if its a boolean (I keep trying to do it and getting frustrated when it doesn't work).

@@ +459,5 @@
> +      default:
> +      case Services.prefs.PREF_STRING:
> +        return Services.prefs.getCharPref(this.name);
> +    }
> +  },

Add a space between methods.

::: mobile/android/chrome/content/config.xhtml
@@ +26,5 @@
> +    <div class="toolbar">
> +        <div class="toolbar-container">
> +            <input id="new-pref-toggle-button" type="image"
> +                onclick="NewPrefDialog.toggleShowHide();" alt="New"
> +                src="chrome://browser/skin/images/reader-plus-icon-xhdpi.png"/>

Hmm.. These icons don't look great on my phone. I think maybe we should use something less strange here. Just a span with a background-image set in CSS? Then we have better control over the icon size.

@@ +32,5 @@
> +            <div class="toolbar-item" id="filter-container">
> +                <input id="filter-search-button" type="image" alt="Search"
> +                    src="chrome://browser/skin/images/search.png"/>
> +                <input id="filter-input" type="search" placeholder="&toolbar.searchPlaceholder;" value=""
> +                    oninput="AboutConfig.bufferFilterInput();"/>

Margaret didn't really like having these listeners defined in XML. I kinda do (it makes it easy to figure out what happens when this button is pressed). Up to you IMO.

@@ +43,2 @@
>  
> +    <div id="content" ontouchstart="AboutConfig.filterInput.blur();">

We should make this touchstart smarter if we want to try showing context menus regardless of what's selected.

::: mobile/android/locales/en-US/chrome/config.dtd
@@ +16,5 @@
> +
> +<!ENTITY newPref.toggleButton          "Toggle">
> +<!ENTITY newPref.cancelButton          "Cancel">
> +
> +<!ENTITY contextMenu.copyPrefName      "Copy selected pref Name">

Just Copy Name/Value will work here.

::: mobile/android/themes/core/config.css
@@ +74,5 @@
> +}
> +
> +#new-pref-toggle-button,
> +#filter-search-button,
> +#filter-input-clear-button {

It might be good to hide this clear button when the input is empty:

#filter-input[value=""] + #filter-input-clear-button {
    visibility: invisible;
}

@@ +166,5 @@
> +    background-image: none;
> +}
> +
> +.pref-value {
> +    /* padding: 15px 10px; */

Remove this
Attachment #814642 - Flags: feedback?(wjohnston) → feedback+
Attachment #746200 - Flags: review?(wjohnston)
Gonna dribble in some updates as I get time...

re; |const LOGTAG = "aboutConfig";   // Debugging Tag|

   Yah, removed.

re: |If this changes to change, where do we flip it back? Also, it stays disabled here.|
    |this._positiveButton.textContent = gStringBundle.GetStringFromName("newPref.changeButton");|

   The NewPrefDialog._updatePositiveButton() mehod works properly as far as I can tell, setting both the (disabled/enabled) state of the the button and the (create/change) display text.

   The routine is called at NewPrefDialog._show() for initial dialog display state, 2) newPrefDialog.focusName() via xhtml input element "onfocus=" when user taps back into field, and importantly 3) newPrefDialog.updateName() via xhtml input element "oninput=" every time user keys / changes the input value.

                    <input class="pref-name" id="new-pref-name" type="text" placeholder="&newPref.namePlaceholder;"
                        onfocus="NewPrefDialog.focusName(event);"
                        oninput="NewPrefDialog.updateName(event);"/>

re: |(maybe we should rename that to "shown"?)|
    |this._prefsShield.removeAttribute("raised");|

   Okay, I renamed it, but seriously, "Raise the shields!!!" <--- I don't care who you are, that was funny right there ...  ;-)

re: |This ain't used.|
    |this._filterContainer = document.getElementById("filter-container");|

   Someone else did it...

re: |Add a space between methods.|

   Done.

re: |having these listeners defined in XML|

   I'm agreeing with you.

re: |Just Copy Name/Value will work here.|

   'mkay. I was pointing out that what you already noticed, the copy/paste works on the selected item vs. making it |context menu stuff work on whatever row you tap|, which is now on my list.

re: |Remove this /* padding: 15px 10px; */|

   Did.

re: |It might be good to hide this clear button when the input is empty|

   Nice touch.

re: |I think you could remove this and just set raised here.|
    |this._prefsShield.style.display = "block";|

   Logcially consistent, done.

re; |I think we should probably toggle this if its a boolean (I keep trying to do it|

   Agrees... started work here and exposed an isssue in SelectionHandler.js ... switching gears and will return for the rest of your comments

:-D
Attached patch bug840144 (v5)Splinter Review
This patch finishes addressing all issues you pointed to in comment 23 ... I think it's coming along nicely ... Your next pass over shouldn't be so bad ... the Javascript is only around 700 lines and much of that is comments  :-)


re: |Can we name them something like |_prefTypeSelectElt. _prefNameInputElt|

   Done.

re: |I would rather make this context menu stuff work on whatever row you tap,|

   Done, but I did it by storing the LI node in a contextmenu callback (different than your first thought).

re: |I think we should probably toggle this if its a boolean|

   Done, but I had to restructure a little more than I thought I would to properly handle the UI mouse / click events. The end result is cleaner all around and more readable, so this was a good exercise.

re: |These icons don't look great on my phone ... maybe ... background-image set in CSS|

   Done, but CSS makes me sad.
Attachment #716273 - Attachment is obsolete: true
Attachment #741765 - Attachment is obsolete: true
Attachment #746200 - Attachment is obsolete: true
Attachment #814642 - Attachment is obsolete: true
Attachment #822942 - Flags: review?(wjohnston)
Comment on attachment 822942 [details] [diff] [review]
bug840144 (v5)

Review of attachment 822942 [details] [diff] [review]:
-----------------------------------------------------------------

I really like this :) I talked to UX, and I'd like to land this and incrementally fix/restyle as we go. I still want them to give a signoff that they're ok with that though. This is rarely seen. There are some screenshots and a build in here:

http://people.mozilla.org/~wjohnston/aboutConfig/

::: mobile/android/chrome/content/config.js
@@ +8,5 @@
> +
> +const VKB_ENTER_KEY = 13;   // User press of VKB enter key
> +const INITIAL_PAGE_DELAY = 500;   // Initial pause on program start for scroll alignment
> +const PREFS_BUFFER_MAX = 30;   // Max prefs buffer size for getPrefsBuffer()
> +const PAGE_SCROLL_TRIGGER = 200;     // Triggers additional getPrefsBuffer() on user scroll-to-bottom

This is a height from the bottom? I probably wrote this, but we should make the comment clearer. Maybe something in units of screenheight would be nice. I also see this catching a lot on my phone, I wonder if we should bump it :) Maybe we'd be better to fill in the list with empty rows and then fill them later so that things scroll quickly... those are ideas for follow up bugs though.

@@ +72,5 @@
> +
> +    this._positiveButton = document.getElementById("positive-button");
> +  },
> +
> +  // Called to update positive button tdisplay text ("Create"/"Change), and enabled/disabled status

spelling 'tdisplay'

@@ +88,5 @@
> +      return;
> +    }
> +
> +    // If item already in list, it's being changed, else added
> +    let item = document.querySelector(".pref-item[name=" + aPrefName.quote() + "]");

I love the quote() function :)

@@ +118,5 @@
> +
> +    this.type = "boolean";
> +    this._booleanValue.value = "false";
> +    this._stringValue.value = "";
> +    this._intValue.value = "";

I think we should auto-focus the _prefnameInputElt as well.

@@ +136,5 @@
> +  _handleKeypress: function AC_handleKeypress(aEvent) {
> +    // Hide VKB on new pref enter key press
> +    if (aEvent.keyCode == VKB_ENTER_KEY) {
> +      aEvent.target.blur();
> +    }

No need for braces here.

@@ +202,5 @@
> +    let list = Services.prefs.getChildList("", {}).filter(function(aElement) {
> +      // Avoid "private" preferences
> +      return !(/^capability\./.test(aElement));
> +    });
> +    this._list = list.sort().map(this._getMapPref, this);

_getMapPref sounds weird. Lets just inline this function I think...

@@ +271,5 @@
> +    this._prefsContainer.parentNode.replaceChild(empty, this._prefsContainer); 
> +    this._prefsContainer = empty;
> +
> +    // Quick clear the prefs li.HTML list
> +    for (let i = 0; i < this._list.length; i++) {

We could probably do this as a this._list.forEach(function(item) {
  delete item.li;
});

@@ +337,5 @@
> +    if (aSelection == currentSelection) {
> +      return;
> +    }
> +
> +    // Clear any previous selection and disable context menu

context menu stuff is moved now :)

@@ +343,5 @@
> +      currentSelection.classList.remove("selected");
> +      currentSelection.removeEventListener("keypress", this._handleKeypress, false);
> +    }
> +
> +    // Set any current selection and enable context menu

Same here.

@@ +391,5 @@
> +      return;
> +    }
> +
> +    this.toggleBoolPref(aEvent);
> +  },

Hmm.. I originally thought this should toggle only if you tapped the "true" or "false" part of the row (or the "Toggle" button). But I kinda like this. lets leave it :)

@@ +432,5 @@
> +    let node = this.getNodeForEvent(aEvent);
> +
> +    // Skip if locked, or not boolean
> +    let pref = this._getPrefForNode(node);
> +    if (pref.locked || (pref.type != Services.prefs.PREF_BOOL)) {

We usually would avoid this pref.type check. i.e. this shouldn't be called with a non-bool pref, and it should throw if it was.

@@ +518,5 @@
> +        return Services.prefs.getBoolPref(this.name);
> +      case Services.prefs.PREF_INT:
> +        return Services.prefs.getIntPref(this.name);
> +      default:
> +      case Services.prefs.PREF_STRING:

Put default on the bottom :)

@@ +532,5 @@
> +      case Services.prefs.PREF_INT:
> +        Services.prefs.setIntPref(this.name, aPrefValue);
> +        break;
> +      default:
> +      case Services.prefs.PREF_STRING:

Same here.

@@ +606,5 @@
> +            "onclick='AboutConfig.incrOrDecrIntPref(event, -1);'>" +
> +          "</div>" +
> +        "</div>";
> +
> +      // Somebody else wrote this

Heh. I think this should probably go. I'm not sure that we need this delay either. Lets ax it!

@@ +641,5 @@
> +        break;
> +    }
> +
> +    this.li.setAttribute("default", this.default);
> +    if (this.default) { 

whitespce at end.

::: mobile/android/themes/core/config.css
@@ +8,5 @@
> +    padding: 0;
> +    background-color: #ced7de;
> +    -moz-user-select: none;
> +    font-family: "Open Sans", sans-serif;
> +    font-size: 1em;

I don't think we need this 1em anymore :)

@@ +215,5 @@
> +
> +#new-pref-type {
> +    display: inline-block !important;
> +    border-left: 1px solid #d3d3d3;
> +    width: 10em;

I think we may need text-align: right here so that if this box at 10em is too big, it still has the arrow and text near each other?

@@ +259,5 @@
> +}
> +
> +.pref-item.selected {
> +    background-color: rgba(0,0,255,0.05);
> +}

We should add a style for .pref-item:active as well. Something kinda subtle maybe? #eee?
Attachment #822942 - Flags: review?(wjohnston)
Attachment #822942 - Flags: review+
Attachment #822942 - Flags: feedback?(abc)
Nice! fyi, I took out the |// Somebody else wrote this| and the list was suddenly non-existent ... I modified as such based on what I think happens ... 

   // Delay providing the list item values, until the LI is returned and added to the document
   setTimeout(this._valueSetup.bind(this), INNERHTML_VALUE_DELAY);

And I added the CSS change, but CSS is probably the least of my skills at the moment ... I don't understand what you're trying to achieve functionally, but by itself the following doesn't seem to do anything ... am I to be dynamically adding/removing an "active" class to the pref list item value input element as it gains/loses focus or somesuch thing?

.pref-item.active {
    background-color: #eeeeee;
}
Attached patch bug840144 (v6)Splinter Review
This addresses nits from comment 27, and corrects a bug preventing keypresses after NewPref dialog is cancelled, or toggled closed vs. Create button press.
Attached patch bug840144 (v7)Splinter Review
Latest version, and push-to-try with correction for previous push Robocop failures ... (leakage into testSystemPages)

https://tbpl.mozilla.org/?tree=Try&rev=8e600f5d5d49
Landed. We can address any UX concerns in other bugs:

https://hg.mozilla.org/integration/fx-team/rev/f7cfce366060
https://hg.mozilla.org/mozilla-central/rev/f7cfce366060
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Looks good

Nit: High DPI devices such as my Galaxy S4, the big + and the magnifying glass are very blurry.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.