Closed Bug 622578 Opened 9 years ago Closed 9 years ago

form helper navigation bar should not obscure web content when only the NEXT / PREV buttons are shown

Categories

(Firefox for Android Graveyard :: General, defect, P1)

defect

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: dietrich, Assigned: vingtetun)

References

Details

(Keywords: ux-minimalism, Whiteboard: [has-patch])

Attachments

(5 files, 13 obsolete files)

38.05 KB, image/png
Details
43.20 KB, image/png
Details
19.23 KB, image/png
Details
7.17 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
70.71 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
it just doesn't help. it's confusing. and it's huge, taking up valuable screen space.
blocking2.0: --- → ?
Severity: normal → enhancement
This is not an enhancement request. The form helper:

* gets in the way visually, sometimes obscuring parts of web content that i'm interacting with.

* distracts the eye, since UI unexpectedly opens in a place not directly visually connected with the UI i'm actually interacting with.
Severity: enhancement → normal
We use the form helper to display form autocomplete and <select> UI too. We could display the <select> UI without the form helper, but autocomplete needs a place to live.

I assume, perhaps incorrectly, that your biggest issue is when the form helper is a long gray bar with "[^][v]" buttons at the end?
(In reply to comment #2)
> I assume, perhaps incorrectly, that your biggest issue is when the form helper
> is a long gray bar with "[^][v]" buttons at the end?

That is correct sir!
(In reply to comment #3)
> (In reply to comment #2)
> > I assume, perhaps incorrectly, that your biggest issue is when the form helper
> > is a long gray bar with "[^][v]" buttons at the end?
> 
> That is correct sir!

I have been playing around with a patch to make the long gray bar shrink to just hold the "[^][v]" buttons. Then, if needed, the bar would grow to hold the autocomplete UI.
(In reply to comment #4)

> I have been playing around with a patch to make the long gray bar shrink to
> just hold the "[^][v]" buttons. Then, if needed, the bar would grow to hold the
> autocomplete UI.

I would like to see this!
I'm not sure why this was nominated blocking 2.0. I've transferred the flag to blocking-fennec. Let me know if this really would be a platform blocker for some reason.
blocking2.0: ? → ---
tracking-fennec: --- → ?
I hit the wrong flag by accident. Sorry.
Updated summary
Summary: form helper should be turned off by default → form helper navigation bar should not obscure web content when only the NEXT / PREV buttons are shown
tracking-fennec: ? → 2.0+
Assignee: nobody → 21
Attached patch wip v0.1 (obsolete) — Splinter Review
Just dumping my wip here to not lost it. I will attach a few screenshots later.
Keywords: ux-minimalism
OS: Android → All
Hardware: ARM → All
Attached patch wip v0.2 (obsolete) — Splinter Review
Still an early wip but with a better handling of suggestions.
Things starts to be usable except optgroup in <select/> list.

The coming screenshots are a very rough try to have something working, I will try to enhance them in the next few days.
Attachment #505118 - Attachment is obsolete: true
Attached patch wip - v0.3 (obsolete) — Splinter Review
The wip still contains a few places where it needs to be fixed and a few clean up but starts to be usable if you don't mind having a permanent popup on your screen once you got a suggestion :)
Attachment #505239 - Attachment is obsolete: true
Attached patch WIP v0.4 (obsolete) — Splinter Review
Except UI styling there is a few places to clean up, I want to confirm I'm going into the right direction.
Attachment #505286 - Attachment is obsolete: true
Attachment #505418 - Flags: feedback?(mark.finkle)
Priority: -- → P1
Attached patch last-wip (obsolete) — Splinter Review
This is the last WIP, the next one will be a patch with more CSS styling.
Attachment #505418 - Attachment is obsolete: true
Attachment #505598 - Flags: feedback?(mark.finkle)
Attachment #505418 - Flags: feedback?(mark.finkle)
Attached patch WIP v0.5 (obsolete) — Splinter Review
Anotehr WIP finally, this one keep the VKB always opened for a better interaction with forms.
Attachment #505598 - Attachment is obsolete: true
Attachment #505598 - Flags: feedback?(mark.finkle)
I've plan to divide this patch in 5 parts:
 * Remove the filtering textbox for select element and add a better support for keys
 * Move the autocomplete feature out of the bottom area
 * Remove all the resizing code of the content-navigator area and use a new style for it
 * Keep the VKB keyboard opened if any while navigating thought the fields
Attachment #508804 - Flags: review?(mark.finkle)
Comment on attachment 508804 [details] [diff] [review]
patch - part 1 - Remove the filter textbox for select elements and enhance keys handling

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

>+    // Save already selected indexes to detect what has changed when a a new
>+    // element is selected
>     this._selectedIndexes = this._getSelectedIndexes();
>+
>     let firstSelected = null;
>-
>     // Using a fragment prevent us to hang on huge list

Don't put a line of code directly on top of a comment. Keep the blank line

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

>+
>+    // Listen key events for fields that are non-editable
>+    let keyModule = new ContentCustomKeySender(window);

Why do we need this? The comment doesn't explain it well enough for me. We already have one of these, why do we need another?

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#202


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


>   _getCaretRect: function _formHelperGetCaretRect() {

>+    if((element.mozIsTextField && element.mozIsTextField(false) ||
>+          element instanceof HTMLTextAreaElement) && focusedElement == element) {

if (

align the second line or just use one line

r- until we figure out the ContentCustomKeySender
Attachment #508804 - Flags: review?(mark.finkle) → review-
(In reply to comment #19)
> Comment on attachment 508804 [details] [diff] [review]
> patch - part 1 - Remove the filter textbox for select elements and enhance keys
> handling
> 
> >diff --git a/chrome/content/SelectHelperUI.js b/chrome/content/SelectHelperUI.js
> 
> >+    // Save already selected indexes to detect what has changed when a a new
> >+    // element is selected
> >     this._selectedIndexes = this._getSelectedIndexes();
> >+
> >     let firstSelected = null;
> >-
> >     // Using a fragment prevent us to hang on huge list
> 
> Don't put a line of code directly on top of a comment. Keep the blank line
> 
> >diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js
> 
> >+
> >+    // Listen key events for fields that are non-editable
> >+    let keyModule = new ContentCustomKeySender(window);
> 
> Why do we need this? The comment doesn't explain it well enough for me. We
> already have one of these, why do we need another?
> 
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#202
> 
> 
> >diff --git a/chrome/content/forms.js b/chrome/content/forms.js
> 
> 
> >   _getCaretRect: function _formHelperGetCaretRect() {
> 
> >+    if((element.mozIsTextField && element.mozIsTextField(false) ||
> >+          element instanceof HTMLTextAreaElement) && focusedElement == element) {
> 
> if (
> 
> align the second line or just use one line
> 
> r- until we figure out the ContentCustomKeySender

To prevent resizing and keep the next/previous buttons always at the same place I want to keep the keyboard opened (in a next patch) even if you are on a button, a select or an input field.

Another benefit of having the keyboard opened is the ability to use keys to navigate thought the <select /> options for example and because I don't want to re-implement the existing algorithm, the ContentCustomKeySender will dispatch keys to the content and let keypress handling happening. Basically this mean we're loosing the filtering option to gain some place _but_ we're allowing keys navigation throught the select options.

I was wondering if I should have expose the existing ContentCustomKeySender as a property of the Browser object instead of creating a new instance of it?
This patch move the autocompletion UI out of the form assistant area and use arrowpanel to have them floating next the current field if needed
Attachment #509127 - Flags: review?(mark.finkle)
This patch move the Select Helper out of the form assistant area and, I think the MenuListHelperUI and SelectHelperUI could be merge soon but this is outside the scope of this bug
Attachment #509128 - Flags: review?(mark.finkle)
The patch removes the needs for resizing the content area and use a new style for both the form assistant and the find assistant.
There is still no pref to show/hide the arrows but I plan to do that on a last patch dealing with VKB
Mark, the patch just remove the filter functionnality and the string associated to it. I think we want that for b5.
Attachment #512165 - Flags: review?(mark.finkle)
Comment on attachment 512165 [details] [diff] [review]
Patch - Remove filter

Looks good for beta 5. We want to drop the string for sure, and the filter functionality, if simple. Make sure we don't regression though.
Attachment #512165 - Flags: review?(mark.finkle) → review+
(In reply to comment #27)
> Comment on attachment 512165 [details] [diff] [review]
> Patch - Remove filter
> 
> Looks good for beta 5. We want to drop the string for sure, and the filter
> functionality, if simple. Make sure we don't regression though.

Pushed: http://hg.mozilla.org/mobile-browser/rev/6e056dad175c
Comment on attachment 509126 [details] [diff] [review]
Patch part 1 v0.2 - Remove select filter and enhance keyboard handling

Is this patch still needed? You probably need to refactor it if it is needed
Attachment #509126 - Flags: review?(mark.finkle) → review-
Comment on attachment 509139 [details] [diff] [review]
509130: Patch part 4 - Remove the needs for resizing the UI by using floating arrows

-moz-border-radius  ->  border-radius
Attachment #509139 - Flags: review?(mark.finkle) → review+
Comment on attachment 509128 [details] [diff] [review]
Patch part 3 - Separate the select helper from the Form assistant area

>+      let image = document.createElement("image");
>+      image.setAttribute("scr", "");
>+      item.appendChild(image);

"scr" -> "src"

>+  reset: function selectHelperReset() {
>+    this._updateControl();
>+    while (this._listbox.hasChildNodes())
>+      this._listbox.removeChild(this._listbox.lastChild);

This could be slow for long lists. Can we use the old way? Do a shallow clone of the listbox

I see a lot of if / else blocks that could be converted to | } else { | in here too

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

>     this._currentElement = {
>       id: aElement.id,
>       name: aElement.name,
>+      title: aElement.label,

I thought I saw | label: xxx |  in a different part of the patch? am I confused?
 
>         // want to show a UI for <select /> element but not managed by
>         // FormHelperUI
>         this.enabled ? this.show(json.current, json.hasPrevious, json.hasNext)
>-                     : SelectHelperUI.show(json.current.choices);
>+                     : SelectHelperUI.show(json.current.choices, jsonc.current.title);

Error: jsonc -> json

>+    if (lastHasChoices && !currentHasChoices)
>       SelectHelperUI.hide();
>-    }
>+    else if (currentHasChoices)
>+      SelectHelperUI.show(aCurrentElement.list, aCurrentElement.title);

How about:

if (currentHasChoices) {
  SelectHelperUI.show(aCurrentElement.list, aCurrentElement.title);
} else {
  if (lastHasChoices)
    SelectHelperUI.hide();
}

r- needs tweaks, but almost done
Attachment #509128 - Flags: review?(mark.finkle) → review-
Comment on attachment 509127 [details] [diff] [review]
Patch part 2 - Move autocompletion out of the form assistant area


>diff --git a/chrome/content/common-ui.js b/chrome/content/common-ui.js
>-  _displaySuggestions: function _formHelperDisplaySuggestions(aSuggestions) {

>+    // the scrollX/scrollY position can change because of the animated zoom so
>+    // delay the suggestions positionning

positioning

>+    if (AnimatedZoom.isZooming()) {
>+      let self = this;
>+      window.addEventListener("AnimatedZoomEnd", function() {
>+        window.removeEventListener("AnimatedZoomEnd", arguments.callee, true);
>+          self._updateSuggestionsFor(aElement);
>+      }, true);
>+      return;
>+    }

The new cool way to write this would be:

window.addEventListener("AnimatedZoomEnd", (function animatedZoomEnd() {
  window.removeEventListener("AnimatedZoomEnd", animatedZoomEnd, true);
    this._updateSuggestionsFor(aElement);
}).bind(this), true);

No self needed and arguments.callee is going away in strict mode

>+    // Create a virtual element to point to
>+    let self = this;
>+    let virtualContentElement = {
>+      getBoundingClientRect: function() {
>+        return virtualContentRect;
>+      }
>+    };

No need for |self| here

r+ with nits
Attachment #509127 - Flags: review?(mark.finkle) → review+
I want a single patch for the next review cycle. Thanks
Attached patch Patch unified (obsolete) — Splinter Review
The patch is a unified version of the previous 4 patches with comments addressed and some bug fixes found by Madhava during testing.

Notice that I have not been cool with AnimatedZoomEnd because the listener wasn't removed in my test, so I have stayed like a old man for the moment.

The patch does not address in terms of look & feel:
 * Madhava want an other style for the navigation buttons
 * The look of select's title is probably not final yet

The patch does not address in terms of feature:
 * The code (pref) to show/hide the navigation buttons
 * The code hide the navigation button on Android and use the keyboard's "Next" key
Attachment #506448 - Attachment is obsolete: true
Attachment #509126 - Attachment is obsolete: true
Attachment #509127 - Attachment is obsolete: true
Attachment #509128 - Attachment is obsolete: true
Attachment #509139 - Attachment is obsolete: true
Attachment #514002 - Flags: review?(mark.finkle)
Comment on attachment 514002 [details] [diff] [review]
Patch unified


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

>-    let keySender = new ContentCustomKeySender(Elements.browsers);
>+    this.keySender = new ContentCustomKeySender(Elements.browsers);
>     let mouseModule = new MouseModule();
>     let gestureModule = new GestureModule(Elements.browsers);
>     let scrollWheelModule = new ScrollwheelModule(Elements.browsers);

I wonder if we shouldn't add all of these to the Browser object. Makes accessing them a bit easier.

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

>+    <vbox id="select-container" class="window-width window-height context-block" top="0" left="0" hidden="true" flex="1">
>+      <vbox id="select-popup" class="dialog-dark">
>+        <hbox pack="center">
>+            <label id="select-title" class="options-title" align="center" crop="center" flex="1"/>
>+            <button label="&selectHelper.done;" oncommand="SelectHelperUI.hide();"/>
>         </hbox>
>+        <richlistbox id="select-commands" onclick="if (event.target != this) SelectHelperUI.selectByIndex(this.selectedIndex);" flex="1"/>

I don't think I like the "Done" button on top of the list.

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

>+  _ensureSuggestionsVisible: function _formHelperEnsureSuggestionsVisible() {

This method is big and a little expensive (for performance) we need to make sure it's never called unless absolutely needed. Just a warning :)

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

>   _getCaretRect: function _formHelperGetCaretRect() {

>+    if((element.mozIsTextField && element.mozIsTextField(false) ||
>+        element instanceof HTMLTextAreaElement) && focusedElement == element) {

if (

>diff --git a/themes/core/forms.css b/themes/core/forms.css

>+#content-navigator > .previous-button {

>+  border: 2px solid white !important;

Use @border_width_???@

>+#content-navigator > .next-button {

>+  border: 2px solid white !important;

Use @border_width_???@

>+#find-helper-textbox {

>+  border: 2px solid white !important;

Use @border_width_???@

>+.option-command > image {
>+  width: 32px;
>+  height: 32px;
>+}
>+
>+.option-command.selected > image {
>+  width: 30px;
>+  height: 30px;
>+  list-style-image: url("chrome://browser/skin/images/check-30.png");

Why use the 32px in .option-command > imag if the selected image is 30px? Should 32px -> 30px ?

----

Overall, this tested very well. The Next/Prev button behavior was great and the CSS style was perfect, IMO. The Find-in-Page UI was great too.
The suggestions UI needs some work. I'm not sure the arrowbox works well enough here. We can talk to Madhava about it though.

 I found one bug and one performance issue:
* Using ESC failed to close an open <select> UI. The UI closed but re-opened in a quick flash. Not sure if Android BACK button would fail the same way.
* The <select> UI was a little slow to open after tapping a combobox or list box (http://people.mozilla.com/~mfinkle/select.html). And this was on Linux desktop.Might be more of a problem on device.

You added a lot of code to use the keyboard while the <select> UI is open, iiuc. Maybe more code and complexity than it's worth. (Just thinking out loud)

Please try to fix the ESC issue and take a look at the speed issue.

There are enough nits here that I'm going to r- and review again.
Attachment #514002 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.3Splinter Review
(In reply to comment #35)
> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> 
> >-    let keySender = new ContentCustomKeySender(Elements.browsers);
> >+    this.keySender = new ContentCustomKeySender(Elements.browsers);
> >     let mouseModule = new MouseModule();
> >     let gestureModule = new GestureModule(Elements.browsers);
> >     let scrollWheelModule = new ScrollwheelModule(Elements.browsers);
> 
> I wonder if we shouldn't add all of these to the Browser object. Makes
> accessing them a bit easier.

I have a kind of mixed feeling about that, I would like to have mouseModule and friends more easily accessible but on the other hand having them "private" prevent bad practices and force to find solution without tweaking those.

Anyway, I think this worth a discussion on IRC but is not require for this bug, IMO.

> * The <select> UI was a little slow to open after tapping a combobox or list
> box (http://people.mozilla.com/~mfinkle/select.html). And this was on Linux
> desktop.Might be more of a problem on device.

I've move from richlistitem to listitem for now but I think the SelectHelperUI.js code and MenuListHelperUI.js code (which still use richlistbox/richlistitem) can be refactored and enhance together in an other bug.
Attachment #514002 - Attachment is obsolete: true
Attachment #514459 - Flags: review?(mark.finkle)
Comment on attachment 514459 [details] [diff] [review]
Patch v0.3

Next steps:
* Get suggestions working in way we like (style and behavior)
* Get the <select> UI style figured out
* Test for regressions. This affects Form Assistant and Find-in-Page
* Test for performance issues:
  * Opening Form Assistant and Find-in-Page
  * Moving between fields
  * Moving between found words
  * Opening the <select> UI
  * Closing the <select> UI
Attachment #514459 - Flags: review?(mark.finkle) → review+
this is ready to land
Whiteboard: [has-patch]
I've filled bug 636339 for doing implementing a better way to interact with the keyboard during a formfill session.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 636463
Depends on: 637215
Verified:
Mozilla/5.0 (Android; Linux armv71; rv2.0b13pre) Gecko/20110317
Firefox/4.0b13pre Fennec/4.0b6pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.