540009 - revise about:config screen design for touch use

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

mockups to come.  We need to make it finger friendly and not so reliant on context-menus.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-early-stage

Because I think it can be a great example of how to transform a desktop designed interface to a finger friendly one for showing at Fosdem, I've start to hack around that (and discover some small front end bugs!)

The current wip did not allow to create a new pref or fully localize the UI, but you can already modify them thought a much more friendly interface. (and btw, this resolved bug 512840)

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Early screenshot

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-2

This wip still missed a nice graphical way to create a new pref

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-3

This wip is functionnal - I need to clean it up a bit and see if I can find any hidden bugs.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Fully functional patch.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(It still miss the wince themes change)

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

Screenshot of the initial view

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Screenshot when clicking on "Integer" in the Add Preference row

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

This one handle winmo case and correct a small bug with the editing part of a preference

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 432564 [details] [diff] [review]
Patch v0.2

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>-      <!-- This won't be updated when the window size changes, but that's OK,
>-           since we don't need an exact value - we just use it to get an
>-           approximate scroll position for deciding whether to append additional
>-           items (see batchSize/_insertItems()).
>-        -->
>-      <field name="_childrenHeight">this.scrollBoxObject.height;</field>

You don't cache this in the new code. Could be a small performance issue. We can see if it makes a difference later.

>-      <method name="close">
 
>-      </method>

Looks like you don't do this in the new code. Is it an issue?

>-
>-      <method name="_insertItems">

>-            if (!itemsRemaining && this._scrollListenerAdded) {
>-              // Can get rid of the scroll listener now that all items are added.
>-              this._children.removeEventListener("scroll", this._scrollListener, false);
>-              this._scrollListenerAdded = false;
>-            }

Looks like you don't remove the listener in the new code. Any reason not to?

>+  <binding id="richlistbox-batch" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox">
>+    <handlers>
>+      <handler event="scroll">

I see why you don't add & remove the scroll listener now. You just use a handler and it's always connected. We should see if there are performance issues with keeping the scroll handler always hooked up.

Are there times when the "scroll" event it fired too many times? We should look at the original bug and see if there are any comments.

>+        <![CDATA[
>+          let contentHeight = this.scrollBoxObject.height;

Does this change or can it be a <field> again. If it changes do to resizes, then maybe we could leave it as you have it.

Could you move the comments in the original batch code into your new code?

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>   close: function() {
>     BrowserUI.updateStar();
> 
>-    this._bookmarks.close();
>-

Why remove this?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>@@ -1365,18 +1365,20 @@ Browser.MainDragger.prototype = {

>     // do HTML overflow or XUL panning
>-    if (this.contentScrollbox && !doffset.isZero())
>+    if (this.contentScrollbox && !doffset.isZero()) {
>       this._panScrollbox(this.contentScrollbox, doffset);
>+      render = true;
>+    }

Was this an old bug? if not, why do we need it now?

>diff --git a/chrome/content/config.js b/chrome/content/config.js

>+  filter: function filter(aValue) {
>+    let row = document.getElementById("editor-row");
>+    row.setAttribute("hidden", aValue != "");

I wonder if this matters. We should be able to add at any time, right? Or is there a reason.

>+    let container = this._container;
>+    container.scrollBoxObject.scrollTo(0, 0);
>+    while(container.childNodes.length > 1)
>+      container.removeChild(container.lastChild);

There is another pattern for removing children. I wonder if it's faster.
See http://mxr.mozilla.org/mobile-browser/source/chrome/content/downloads.js#105

>+    label.setAttribute("class", "preftitle");
>+    label.setAttribute("flex", "1");
>+    label.setAttribute("value", aPref.name);
>+    label.setAttribute("crop", "end");

You could add the flex and crop to the CSS for preftitle

>+    label = document.createElement("label");
>+    label.setAttribute("class", "prefvalue");
>+    label.setAttribute("flex", "4");
>+    label.setAttribute("value", aPref.value);
>+    label.setAttribute("crop", "end");

You could add the flex and crop to the CSS for prefvalue

>diff --git a/locales/jar.mn b/locales/jar.mn

>+  locale/@AB_CD@/browser/config.dtd               (%chrome/config.dtd)

Did you forget to add the DTD file itself?

>diff --git a/themes/hildon/config.css b/themes/hildon/config.css

>+richlistitem {
>+  padding: 2px;

Why 2px? that is very small for devices. 4px is usually our "small" padding unless some other padding is being added to it from another element

>+  min-height: 70px !important; /* row size */
>+  max-height: 70px !important;

Do we need the max-height?

>+  border-bottom: 1px solid rgb(207,207,207);
>+  -moz-box-align: center;
>+}

Isn't some of this pulled in from platform.css ?

>+richlistitem .preftitle {
>+  min-width: 200px;

Why 200px? Could it be bigger? We are 480px if portrait.


>+#editor-setting .prefbox {
>+  border-bottom: 0px solid transparent !important;
>+}

Do you want the border to be 0px or transparent? 0px will cause a layout change. Transparent will just make the border disappear, but won't change the layout.

(Same for wince CSS)

r- until I can test it on device (need the DTD for that)

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> There is another pattern for removing children. I wonder if it's faster.
> See
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/downloads.js#105
Yeah, this is much faster (the downloads.js way).

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

(In reply to comment #12)
> >-      <field name="_childrenHeight">this.scrollBoxObject.height;</field>
> 
> You don't cache this in the new code. Could be a small performance issue. We
> can see if it makes a difference later.

I've revert that back after have read https://bugzilla.mozilla.org/show_bug.cgi?id=522066#c8
 
> >-      <method name="close">
> 
> >-      </method>
> 
> Looks like you don't do this in the new code. Is it an issue?

I don't do that because we don't need to remove the event listener now, and the only issue I see can be a bigger memory footprint because of the fact that we're not releasing this._childItems as before which is keep as an internal array of the batchable binding.

> >+  <binding id="richlistbox-batch" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox">
> >+    <handlers>
> >+      <handler event="scroll">
> 
> I see why you don't add & remove the scroll listener now. You just use a
> handler and it's always connected. We should see if there are performance
> issues with keeping the scroll handler always hooked up.

I don't think there is any performance difference between adding a listener and using directly scroll but I can't fully assert on that.
> 
> Are there times when the "scroll" event it fired too many times? We should look
> at the original bug and see if there are any comments.

The comment refering to that is:
https://bugzilla.mozilla.org/show_bug.cgi?id=522066#c6

I think code should be ok now too because I have just moved the executed callback when there is a scroll event.

> 
> >+        <![CDATA[
> >+          let contentHeight = this.scrollBoxObject.height;
> 
> Does this change or can it be a <field> again. If it changes do to resizes,
> then maybe we could leave it as you have it.

I've re-move this as a field.

> 
> Could you move the comments in the original batch code into your new code?
> 

Done.

> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >   close: function() {
> >     BrowserUI.updateStar();
> > 
> >-    this._bookmarks.close();
> >-
> 
> Why remove this?
> 

See comments above.

> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >@@ -1365,18 +1365,20 @@ Browser.MainDragger.prototype = {
> 
> >     // do HTML overflow or XUL panning
> >-    if (this.contentScrollbox && !doffset.isZero())
> >+    if (this.contentScrollbox && !doffset.isZero()) {
> >       this._panScrollbox(this.contentScrollbox, doffset);
> >+      render = true;
> >+    }
> 
> Was this an old bug? if not, why do we need it now?

IMO this is an old bug because without this flag we can't see the content of a scrollable div while panning, we have to wait for the release.

> 
> >diff --git a/chrome/content/config.js b/chrome/content/config.js
> 
> >+  filter: function filter(aValue) {
> >+    let row = document.getElementById("editor-row");
> >+    row.setAttribute("hidden", aValue != "");
> 
> I wonder if this matters. We should be able to add at any time, right? Or is
> there a reason.

I've tried to do the same as we do in the awesomepanel with the "See All Bookmarks" row.
> 
> >+    let container = this._container;
> >+    container.scrollBoxObject.scrollTo(0, 0);
> >+    while(container.childNodes.length > 1)
> >+      container.removeChild(container.lastChild);
> 
> There is another pattern for removing children. I wonder if it's faster.
> See
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/downloads.js#105
> 

Done.

> >+    label.setAttribute("class", "preftitle");
> >+    label.setAttribute("flex", "1");
> >+    label.setAttribute("value", aPref.name);
> >+    label.setAttribute("crop", "end");
> 
> You could add the flex and crop to the CSS for preftitle

Done except for the crop. I'm not sure I can do that in CSS.

> 
> >+    label = document.createElement("label");
> >+    label.setAttribute("class", "prefvalue");
> >+    label.setAttribute("flex", "4");
> >+    label.setAttribute("value", aPref.value);
> >+    label.setAttribute("crop", "end");
> 
> You could add the flex and crop to the CSS for prefvalue

Same as above.

> 
> >diff --git a/locales/jar.mn b/locales/jar.mn
> 
> >+  locale/@AB_CD@/browser/config.dtd               (%chrome/config.dtd)
> 
> Did you forget to add the DTD file itself?

Ooops!

> 
> >diff --git a/themes/hildon/config.css b/themes/hildon/config.css
> 
> >+richlistitem {
> >+  padding: 2px;
> 
> Why 2px? that is very small for devices. 4px is usually our "small" padding
> unless some other padding is being added to it from another element

There is no real reason that I can remember.
Fixed.

> 
> >+  min-height: 70px !important; /* row size */
> >+  max-height: 70px !important;
> 
> Do we need the max-height?

no, Fixed.

> 
> >+  border-bottom: 1px solid rgb(207,207,207);
> >+  -moz-box-align: center;
> >+}
> 
> Isn't some of this pulled in from platform.css ?

Fixed.

> 
> >+richlistitem .preftitle {
> >+  min-width: 200px;
> 
> Why 200px? Could it be bigger? We are 480px if portrait.

That is because I want to share the space between the title and the value. I want the title to be readable a bit, not completely shrunk by the value. (and idem in the other sense)

> 
> 
> >+#editor-setting .prefbox {
> >+  border-bottom: 0px solid transparent !important;
> >+}
> 
> Do you want the border to be 0px or transparent? 0px will cause a layout
> change. Transparent will just make the border disappear, but won't change the
> layout.

Fixed

> 
> (Same for wince CSS)
> 

Fixed

> r- until I can test it on device (need the DTD for that)

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

getChildList changed from m-192 to m-c and the patch uses the m-c version (with an optional second param).

To make this patch work in m-192 use:

this._branch.getChildList("", {});

See bug 524270

Also, I think we have a performance issue during editing. Anytime we edit (type in textbox or increment an integer) we change the preference, which fires a pref change notification, which is caught in Utils.observe and we call getPrefIndex - which scans the list of all preferences.

Can we check to see if we are editing and if the aPrefName == the current editing item, and return early?

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.4

(In reply to comment #15)
> getChildList changed from m-192 to m-c and the patch uses the m-c version (with
> an optional second param).
> 
> To make this patch work in m-192 use:
> 
> this._branch.getChildList("", {});

Done.

> Can we check to see if we are editing and if the aPrefName == the current
> editing item, and return early?

As said on IRC, I've used a binary search to speed up the thing mainly because Utils didn't (and should not) know anything about what's happening in the "editor".

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 434240 [details] [diff] [review]
Patch v0.4

Feels pretty good on device. We can add more optimizations if we get feedback about performance.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/373513dd886f

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100325 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a3pre) Gecko/20100325 Namoroka/3.7a3pre Fennec/1.1a2pre


followup bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=554956
https://bugzilla.mozilla.org/show_bug.cgi?id=554957