Make Metro about:config better

VERIFIED FIXED in Firefox 29

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 29
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=0 r=ff29)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 8359037 [details] [diff] [review]
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
Attachment #8359037 - Flags: review?(mbrubeck)
(Assignee)

Comment 1

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

Updated

5 years ago
Blocks: 838081
Whiteboard: [triage] [feature] p=0
(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?
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; }
Status: NEW → ASSIGNED
(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

Updated

5 years ago
Blocks: 861680
No longer blocks: 838081
Whiteboard: [triage] [feature] p=0 → [feature] p=0
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.
Flags: needinfo?(markcapella)
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 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.
Attachment #8359037 - Flags: review?(mbrubeck) → review-
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Flags: needinfo?(markcapella)
(Assignee)

Comment 8

5 years ago
Created attachment 8362176 [details] [diff] [review]
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 ...
Attachment #8362176 - Flags: review?(mbrubeck)
(Assignee)

Comment 9

5 years ago
Created attachment 8362188 [details] [diff] [review]
bug959031p3.diff retro fit Strings

This restores use of previously available Metro strings
Attachment #8362188 - Flags: review?(mbrubeck)
Created attachment 8363151 [details] [diff] [review]
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.)
Attachment #8359037 - Attachment is obsolete: true
Attachment #8362176 - Attachment is obsolete: true
Attachment #8362188 - Attachment is obsolete: true
Attachment #8362176 - Flags: review?(mbrubeck)
Attachment #8362188 - Flags: review?(mbrubeck)
Attachment #8363151 - Flags: review?(mbrubeck)
Blocks: 961181
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!
Attachment #8363151 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 12

5 years ago
Nits addressed, and push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=f246e5b05328
https://hg.mozilla.org/mozilla-central/rev/7b40032d4d18
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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.

Updated

5 years ago
Whiteboard: [feature] p=0 → p=0 r=ff29
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.