Closed
Bug 540009
Opened 15 years ago
Closed 14 years ago
revise about:config screen design for touch use
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: madhava, Assigned: vingtetun)
References
Details
Attachments
(3 files, 7 obsolete files)
mockups to come. We need to make it finger friendly and not so reliant on context-menus.
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Comment 2•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
This wip still missed a nice graphical way to create a new pref
Assignee: nobody → 21
Attachment #424176 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
This wip is functionnal - I need to clean it up a bit and see if I can find any hidden bugs.
Attachment #424707 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
Fully functional patch.
Attachment #424868 -
Attachment is obsolete: true
Assignee | ||
Comment 8•14 years ago
|
||
(It still miss the wince themes change)
Assignee | ||
Comment 9•14 years ago
|
||
Screenshot of the initial view
Attachment #424177 -
Attachment is obsolete: true
Assignee | ||
Comment 10•14 years ago
|
||
Updated•14 years ago
|
Flags: in-litmus?
Assignee | ||
Comment 11•14 years ago
|
||
This one handle winmo case and correct a small bug with the editing part of a preference
Attachment #424896 -
Attachment is obsolete: true
Attachment #432564 -
Flags: review?(mark.finkle)
Comment 12•14 years ago
|
||
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)
Attachment #432564 -
Flags: review?(mark.finkle) → review-
Comment 13•14 years ago
|
||
(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).
Assignee | ||
Comment 14•14 years ago
|
||
(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)
Attachment #432564 -
Attachment is obsolete: true
Attachment #433989 -
Flags: review?(mark.finkle)
Comment 15•14 years ago
|
||
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?
Assignee | ||
Comment 16•14 years ago
|
||
(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".
Attachment #433989 -
Attachment is obsolete: true
Attachment #434240 -
Flags: review?(mark.finkle)
Attachment #433989 -
Flags: review?(mark.finkle)
Comment 17•14 years ago
|
||
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.
Attachment #434240 -
Flags: review?(mark.finkle) → review+
Comment 18•14 years ago
|
||
pushed: http://hg.mozilla.org/mobile-browser/rev/373513dd886f
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 19•14 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Flags: in-litmus? → in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•