596614 - focusing the URL field on the awesomescreen should not visibly change categories

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

In this situation:
- tap title to go to awesomescreen
- on awesomescreen, tap on any category other than "All pages" (e.g. Bookmarks)
- focus url field again

currently, the "All Pages" category is visibly chosen.  Even though the resulting set of search items during a search "belongs" to All Pages, we shouldn't visibly switch.  When we do (currently) it means that the screen changes twice (first category switch and then result set showing up) _and_ the screen changes in a way that is not entirely expected.

Instead, in the case of bookmarks, for example, the Bookmarks category should remain selected (and its content on screen) until the user's typing causes a result set to start appearing.

Comment 1

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

I'm a bit afraid that the user will think that he is searching _only_ into his bookmarks or into his history if we do that.

Comment 2

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

Currently, when the user taps the URLBar, we switch to the "All Pages" mode - selecting the "All Pages" button and showing the default list of results.

I kinda agree with Vivien. If we leave the screen in say "Bookmarks", with the bookmark list displayed, the user might think they are searching/filtering the bookmark list.

Beltzner - Thoughts?

Comment 3

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

Attachment: patch - fail with the VKB on Android

The patch is still failing with the VKB on Android, touching the urlbar while you are in an other categories does not bring up the VKB (the field is still readOnly)

Comment 4

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

(In reply to comment #2)
> I kinda agree with Vivien. If we leave the screen in say "Bookmarks", with the
> bookmark list displayed, the user might think they are searching/filtering the
> bookmark list.
> 

As I've said on IRC I've start to change my mind when playing with the code. I'm currently hesitant between the two options, I don't want to confuse the users but not switching the current screen give a much better feeling when you use it.

Comment 5

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

Attachment: wip - v0..2

This patch works with the VKB and correct bug 591955.
It still needs to be improved:
 * there is a bug dismissing the first letter typed in sometimes
 * the whole text should be selected when going from a panel to the "All Pages" panel

Comment 6

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

Attachment: Patch

This one works fine, it still fix bug 591955 and also select the urlbar text the right way when clicking (double click select all seems to be broken, even without this patch, I will check that into an other bug)

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Sorry to take so long here; wanted to check with Madhava to understand the original design intent before I made a decision.

The intent of the design is that the moment the user begins *typing*, the list of categories disappears, and the only thing shown is the list of filtered items. As this bug points out, though, this leads to an interim state where just selecting the URL field causes there to be a mode switch, which is jittery and potentially confusing.

I agree with comment 0; yes, there's a chance that the user will think that they're filtering entirely on the selected category, but it's the cleanest way to execute the interaction with the current design.

Comment 8

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

Comment on attachment 478916 [details] [diff] [review]
Patch


>       <handler event="keypress" phase="capturing">
>         <![CDATA[
>           if (event.keyCode == event.DOM_VK_RETURN)
>             setTimeout(function() { BrowserUI.activePanel = null }, 0);
>-          else if (this.readOnly)
>-            this.readOnly = false;
>+          else if (BrowserUI.activePanel != AllPagesList)
>+            BrowserUI.activePanel = AllPagesList;

Someday we should consider removing the explicit calls to BrowserUI and use events or callbacks to handle communicating to the main app.

>+      <handler event="keypress" phase="bubbling">
>+        <![CDATA[
>+          // Disable the read-only state in landscape if there is a keypress
>+          // because this means there is a kind of hardware keyboard
>+          this.readOnly = false;

How do we know it's landscape now? How do we know it's a hardware keyboard?


>diff -r 8447b3f08dff chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js	Fri Sep 24 19:19:50 2010 -0700
>+++ b/chrome/content/browser-ui.js	Tue Sep 28 01:49:29 2010 +0200
>@@ -165,18 +165,23 @@ var BrowserUI = {
>     this._updateButtons(browser);
>     this._updateIcon(browser.mIconURL);
>     this.updateStar();
>   },
> 
>   showToolbar: function showToolbar(aEdit) {
>     this.hidePanel();
>     this._editURI(aEdit);
>-    if (aEdit)
>+    if (aEdit) {
>       this.showAutoComplete();
>+
>+      // Selecting the text is done after showing the autocomplete because
>+      // pushDialog remove the selection if it is done before
>+      this._edit.select();

Why not put this in the showAutoComplete call? Do we need to obey any preferences when selecting?

>       // URL textbox events
>       case "click":
>-        this.doCommand("cmd_openLocation");
>+        // Don't call the command if there is currently a panel opened, this
>+        // way the current panel is not visibly changed until the user starts
>+        // to type (bug 596614)
>+        if (this.activePanel) {
>+          if (this._edit.readOnly) {
>+            this._edit.readOnly = false;
>+            this._edit.blur();
>+            gFocusManager.setFocus(this._edit, Ci.nsIFocusManager.FLAG_NOSCROLL);
>+
>+            if (this._edit.clickSelectsAll)
>+              this._edit.select();

I don't like having logic for the URLBar in some many locations in the code

>+        else
>+          this.doCommand("cmd_openLocation");

Use {}

Comment 9

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

Comment on attachment 478916 [details] [diff] [review]
Patch

waiting for new patch

Comment 10

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

Attachment: Patch v0.2

Completely revamped patch:

 * no more BrowserUI calls inside bindings.xml
 * the blur/focus logic for the IME state is now attached to the readOnly property of the field which prevent browser-ui.js to care about it
 * We finally respect the clickSelectsAll property of textbox.xml instead of always selecting all the text
 * the code related to panel switching into browser-ui.js is done into function closely related to panel
 * all tests stays green!

Comment 11

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

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

>diff -r 5cc57f96e2f3 chrome/content/bindings.xml

>       <method name="invalidate">

>           let controller = this.input.controller;
>           let searchString = controller.searchString;
>           let items = this._items;
>-
>           // Need to iterate over all our existing entries at a minimum, to make
>           // sure they're either updated or cleared out. We might also have to
>           // add extra items.

Keep the blank line


>diff -r 5cc57f96e2f3 chrome/content/browser-ui.js

> 
>+    this._edit.readOnly = !(aPanel == AllPagesList && Util.isPortrait());
>+#ifndef ANDROID
>+    // On devices (and desktop) where it can exists harware keyboard, leave the
>+    // focus on the field to allow direct input
>+    if (this._edit.readOnly && aPanel != AllPagesList)
>+#endif
>+#ifdef ANDROID
>+    if (this._edit.readOnly)
>+#endif
>+      this.blurFocusedElement();

I don't think we can hardcode that Android does not have a hard keyboard (and that Meego does have a hard keyboard). The T-mobile G2 (android) has a hard keyboard.

>     // Give the new page lots of room
>     Browser.hideSidebars();
>     this.closeAutoComplete(true);
>+    this.activePanel = null;

Shouldn't we put the | activePanel = null | in the closeAutoComplete method? Just asking

(just looked - it already is in the closeAutoComplete method so remove it from here)

r- for the hardcoded Android code. We need to avoid that

Comment 12

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

Attachment: Patch v0.3

I've replaced the #ifdef by a keydown listener on the window

Comment 13

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

Comment on attachment 481254 [details] [diff] [review]
Patch v0.3

When testing this, I found one issue with the home page (about:home). When tapping "Tabs from your other computers" the awesomescreen opens, but no selector buttons (all pages, bookmarks, history, desktop) are shown.

Looks like we just need to tweak the about:home code.
http://mxr.mozilla.org/mobile-browser/source/chrome/content/aboutHome.xhtml#254

r+ with that fixed

Comment 14

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

(In reply to comment #13)
> Comment on attachment 481254 [details] [diff] [review]
> Patch v0.3
> 
> When testing this, I found one issue with the home page (about:home). When
> tapping "Tabs from your other computers" the awesomescreen opens, but no
> selector buttons (all pages, bookmarks, history, desktop) are shown.
> 
> Looks like we just need to tweak the about:home code.
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/aboutHome.xhtml#254
> 
> r+ with that fixed

I am currently trying to fix a annoying bug discovered while using a custom build with this patch checked. Sometimes the software keyboard does not appear once the awesome bar is opened.

Actually my steps to reproduce are:
 * launch fennec without any network connection
 * go to the awesome bar and click on a remote page
 * go to the awesome bar and click on a remote page (yes, another time)
 * go to the awesome bar

Actual result:
 * No software keyboard

Expected result:
 * A software keyboard

I'm not sure of what's happening here, it sounds like a race condition

Comment 15

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

Attachment: Patch v0.4

The vkb has been hidden/displayed until my device crashed! Yeah!
(I really need to find a way to debug the VKB on desktop)

Comment 16

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

Attachment: Patch v0.4

fix a little bug

Comment 17

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

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

Looks like we could use some tests for this too :)

Comment 18

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

(In reply to comment #17)
> Comment on attachment 481915 [details] [diff] [review]
> Patch v0.4
> 
> Looks like we could use some tests for this too :)

I could add test into some of its dependencies, my main problem is much of the pain was for having the virtual keyboard working on device but i'm really not sure how to test that?

http://hg.mozilla.org/mobile-browser/rev/835d44a205d5

Comment 20

[Aakash Desai [:aakashd]]

This isn't fixed in builds when reproducing the steps in comment #0 :

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101011 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101011 Namoroka/4.0b8pre Fennec/4.0b2pre

Comment 21

[Ayan Shah[:ashah]]

As per discussion with aakashd on IRC, this is actually fixed.
Verified on:

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101011 Firefox/4.0b8pre
Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101011
Firefox/4.0b8pre Fennec/4.0b2pre