959031 - Make Metro about:config better

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Attachment: config.diff

Adapt the Android about:config to provide a Metro / Tablet style UI to the Win8 about:config page.

This patch functions, and produces results ... such as:

   Basic screen
      https://www.dropbox.com/s/tlvrh0utfvarjcw/MetroAboutConfigBasic.png

   New prefs dialog, w/filtered list results, and w/modified list items
      https://www.dropbox.com/s/evlpe5wk0cc978u/metroAboutConfigAdvanced.png


Outstanding issue: Metro appears to have a special auto-generated widget for increasing/decreasing an <input> type="number" field that duplicates code already in place for Android.

I'm trying to decide if we'll have to disable the auto-generated buttons, or disable the ones ported from Android.

Let me know what you think!  :-D

Comment 1

[Mark Capella [:capella]]

Ah! (full disclosure) ... my local machine has no touch screen, so I'm simulating that portion of the UI by touchpad.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Capella [:capella] from comment #0)
> Outstanding issue: Metro appears to have a special auto-generated widget for
> increasing/decreasing an <input> type="number" field that duplicates code
> already in place for Android.

That's not Metro-specific; that's how Gecko renders <input type="number"> on all platforms except Android.  On Android we don't show the spinbuttons because we use a special keyboard instead; see comments in bug 635240.  Maybe we should do the same on Metro where we have similar soft keyboard support, and where the spinbuttons are also not very usable with touch.  Want to file a follow-up bug and mark it as blocking both this bug and bug 344616?

Comment 3

[Matt Brubeck (:mbrubeck)]

For now it looks like you can disable the spinbuttons on about:config by adding the following CSS (bug 947728):

input[type="number"] { -moz-appearance: textfield; }

Comment 4

[Wesley Johnston (:wesj)]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> (In reply to Mark Capella [:capella] from comment #0)
> > Outstanding issue: Metro appears to have a special auto-generated widget for
> > increasing/decreasing an <input> type="number" field that duplicates code
> > already in place for Android.
> 
> That's not Metro-specific; that's how Gecko renders <input type="number"> on
> all platforms except Android.  On Android we don't show the spinbuttons
> because we use a special keyboard instead; see comments in bug 635240. 
> Maybe we should do the same on Metro where we have similar soft keyboard
> support, and where the spinbuttons are also not very usable with touch. 
> Want to file a follow-up bug and mark it as blocking both this bug and bug
> 344616?

It'd be interesting to try and make those spin buttons usable via css and then kill these fakes ones. Should be able to play with some styles at:

http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#896

Comment 5

[Matt Brubeck (:mbrubeck)]

If you have time, could you create a version of this patch that uses "hg cp" to copy the files from /mobile/android to /browser/metro before applying the changes to them?  (I guess you'd need to remove the current Metro files, then "hg cp" the Android ones, then overwrite them with your modifications.)  This would be helpful both for reviewing and for browsing the history later on.

I did some diffing and can see that the changes from Android are pretty minimal.  I'm still going to do a review pass just in case I spot anything that won't work in the Metro environment.

Comment 6

[Wesley Johnston (:wesj)]

If there's some way we can share this, I'd love it! The code right now is better than nothing, but we could do better :)

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8359037 [details] [diff] [review]
config.diff

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

This looks good overall.  There are a few more nits and thing below, and some minor cleanup that's not directly related to your work but that I'd like to address while we're touching this code.

I agree with Wes that it would be ideal to share the bulk of this code with Android.  Maybe we could add a new "aboutConfigTouch" module to toolkit.  I'm not sure if we should attempt that before landing this, or fork it now and merge it later.  Certainly this patch would leave us no worse, since we already have a forked about:config for Metro, and this is just replacing it with one that's closer to Android's.

::: browser/metro/base/content/config.js
@@ +8,2 @@
>  
> +const VKB_ENTER_KEY = 13;   // User press of VKB enter key

Can you remove this const and use KeyEvent.DOM_VK_RETURN instead?

@@ +86,3 @@
>  
> +    // Avoid "private" preferences
> +    if (/^capability\./.test(aPrefName)) {

Nit: You can now write this as aPrefName.startsWith("capability.")

(no functional difference, but possibly a bit clearer)

::: browser/metro/base/jar.mn
@@ +68,1 @@
>    content/config.js                            (content/config.js)

config.js should probably move into the "pages" subdir.  (It's just an oversight that it hasn't already moved there.)

::: browser/metro/locales/en-US/chrome/config.dtd
@@ -4,5 @@
>  
> -<!ENTITY empty.label            "Search">
> -<!ENTITY newpref.label          "Add a New Preference">
> -<!ENTITY addpref.name           "Name">
> -<!ENTITY addpref.value          "Value">

For strings that are keeping the same value, we should keep the same key also, to avoid needless work for localizers.

(Though if we can find a way to share all this code with Android, then it would be okay to move to a new set of shared strings.)

::: browser/metro/theme/config.css
@@ +7,5 @@
> +    margin: 0;
> +    padding: 0;
> +    background-color: #ced7de;
> +    -moz-user-select: none;
> +    font-family: "Open Sans", sans-serif;

Since we're not shipping Open Sans on Metro, this should be:

  font-family: "Segoe UI", sans-serif;

(This might change in bug 904901.)

@@ +20,5 @@
> +    left: 0;
> +    z-index: 10;
> +    box-shadow: 0 0 3px #444;
> +    background-color: #ced7de;
> +    color: #000000;

We'll probably want to tweak the layout and colors to be more Metro-y eventually, but we can do that in follow-up bugs.

@@ +323,5 @@
> +    opacity: 1;
> +}
> +
> +#loading-container > li {
> +    background-image: url(chrome://browser/skin/images/throbber.png);

Instead of adding a new image just for this, maybe we could use chrome://global/skin/media/throbber.png, or our <cssthrobber> XUL widget.

Comment 8

[Mark Capella [:capella]]

Attachment: bug959031p2.diff

This follow-on patch to the original, addresses the issues / corrections that you pointed out.

I'm going to post another patch that retro-fits the Android strings to use the original Metro versions ...

Comment 9

[Mark Capella [:capella]]

Attachment: bug959031p3.diff retro fit Strings

This restores use of previously available Metro strings

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: roll-up patch

Thanks!

This patch is just a rollup of the previous three patches, with the history rejiggered a bit to show which lines were copied from Android where possible.  (I couldn't get this to work with the CSS file.)

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8363151 [details] [diff] [review]
roll-up patch

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

Thanks again!  I think this is ready to land, modulo some tiny nits (below).

I'm still not sure about the best way to share code with Android... I guess we could put it the shared bits in toolkit, but for the sake of package size we might want a way to avoid building it in apps where it's not used (Thunderbird, Seamonkey, Firefox for Mac/Linux).  We don't do a great job at this sort of subsetting in general.

::: browser/metro/base/content/pages/config.js
@@ +455,5 @@
>    observe: function AC_observe(aSubject, aTopic, aPrefName) {
>      let pref = new Pref(aPrefName);
>  
> +    // Ignore uninteresting preference changes, and external changes to "private" preferences
> +    if ((aTopic != "nsPref:changed") || (pref.name).startsWith(PRIVATE_PREF_PREFIX)) {

Style suggestion: The parentheses around != and pref.name are not needed and can be removed... but if you find they help the readability then I won't argue.

::: browser/metro/theme/config.css
@@ +232,5 @@
> +
> +/* Disable newPref dialog spinbuttons, use custom version from Android */
> +#new-pref-value-int {
> +    -moz-appearance: textfield;
> +}

Could you file a follow-up bug about fixing the default spinbutton style on Metro (and Android?) to be touch-friendly, and include that bug number in the comment above?  Thanks!

Comment 12

[Mark Capella [:capella]]

Nits addressed, and push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=f246e5b05328

Comment 13

[Mark Capella [:capella]]

Nad into fx-team: https://hg.mozilla.org/integration/fx-team/rev/7b40032d4d18

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b40032d4d18

Comment 15

[Wesley Johnston (:wesj)]

I filed bug 959754 for the spin buttons on Android. Note you can't style them from content, but you can from UA stylesheets. I'd be fine with updating the style for sites to have something touch friendly.

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Nightly (build ID: 20140226030202) and latest Aurora (build ID: 20140226004001) using a Surface Pro 2 device. 
The design is changed accordingly.