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

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

it just doesn't help. it's confusing. and it's huge, taking up valuable screen space.

Comment 1

[Dietrich Ayala (:dietrich)]

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.

Comment 2

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

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?

Comment 3

[Dietrich Ayala (:dietrich)]

(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!

Comment 4

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

(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.

Comment 5

[Madhava Enros [:madhava]]

(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!

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Dietrich Ayala (:dietrich)]

I hit the wrong flag by accident. Sorry.

Comment 8

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

Updated summary

Comment 9

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

Attachment: wip v0.1

Just dumping my wip here to not lost it. I will attach a few screenshots later.

Comment 10

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

Attachment: wip v0.2

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.

Comment 11

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

Attachment: Simple form navigation

Comment 12

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

Attachment: List navigation

Comment 13

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

Attachment: Autocomplete suggestions

Comment 14

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

Attachment: wip - v0.3

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 :)

Comment 15

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

Attachment: WIP v0.4

Except UI styling there is a few places to clean up, I want to confirm I'm going into the right direction.

Comment 16

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

Attachment: last-wip

This is the last WIP, the next one will be a patch with more CSS styling.

Comment 17

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

Attachment: WIP v0.5

Anotehr WIP finally, this one keep the VKB always opened for a better interaction with forms.

Comment 18

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

Attachment: patch - part 1 - Remove the filter textbox for select elements and enhance keys handling

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

Comment 19

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

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

Comment 20

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

(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?

Comment 21

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

Attachment: Patch part 1 v0.2 - Remove select filter and enhance keyboard handling

Adressed comments

Comment 22

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

Attachment: Patch part 2 - Move autocompletion out of the form assistant area

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

Comment 23

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

Attachment: Patch part 3 - Separate the select helper from the Form assistant area

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

Comment 24

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

Attachment: Patch part 4 - Remove the needs for resizing the UI by using floating arrows

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

Comment 25

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

Attachment: 509130: Patch part 4 - Remove the needs for resizing the UI by using floating arrows

Comment 26

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

Attachment: Patch - Remove filter

Mark, the patch just remove the filter functionnality and the string associated to it. I think we want that for b5.

Comment 27

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

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.

Comment 28

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

(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 29

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

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

Comment 30

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

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

Comment 31

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

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

Comment 32

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

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

Comment 33

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

I want a single patch for the next review cycle. Thanks

Comment 34

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

Attachment: Patch unified

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

Comment 35

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

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.

Comment 36

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

Attachment: Patch v0.3

(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.

Comment 37

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

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

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

this is ready to land

Comment 39

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

pushed: http://hg.mozilla.org/mobile-browser/rev/956ae84e7459

Comment 40

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

I've filled bug 636339 for doing implementing a better way to interact with the keyboard during a formfill session.

Comment 41

[Ayan Shah[:ashah]]

Verified:
Mozilla/5.0 (Android; Linux armv71; rv2.0b13pre) Gecko/20110317
Firefox/4.0b13pre Fennec/4.0b6pre