Closed Bug 596614 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: madhava, Assigned: vingtetun)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
blocking2.0: --- → ?
Summary: focusing the URL field on the awesomescreen should not visible change categories → focusing the URL field on the awesomescreen should not visibly change categories
blocking2.0: ? → ---
tracking-fennec: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
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.
tracking-fennec: ? → 2.0b2+
Assignee: nobody → 21
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?
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)
(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.
Attached patch wip - v0..2 (obsolete) — Splinter Review
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
Attachment #478289 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #478812 - Attachment is obsolete: true
Attachment #478916 - Flags: review?(mark.finkle)
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 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 on attachment 478916 [details] [diff] [review]
Patch

waiting for new patch
Attachment #478916 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.2 (obsolete) — Splinter Review
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!
Attachment #478916 - Attachment is obsolete: true
Attachment #481217 - Flags: review?(mark.finkle)
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
Attachment #481217 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.3Splinter Review
I've replaced the #ifdef by a keydown listener on the window
Attachment #481217 - Attachment is obsolete: true
Attachment #481254 - Flags: review?(mark.finkle)
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
Attachment #481254 - Flags: review?(mark.finkle) → review+
(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
Attached patch Patch v0.4 (obsolete) — Splinter Review
The vkb has been hidden/displayed until my device crashed! Yeah!
(I really need to find a way to debug the VKB on desktop)
Attachment #481872 - Flags: review?(mark.finkle)
Attached patch Patch v0.4Splinter Review
fix a little bug
Attachment #481872 - Attachment is obsolete: true
Attachment #481915 - Flags: review?(mark.finkle)
Attachment #481872 - Flags: review?(mark.finkle)
Comment on attachment 481915 [details] [diff] [review]
Patch v0.4

Looks like we could use some tests for this too :)
Attachment #481915 - Flags: review?(mark.finkle) → review+
(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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 603227
Duplicate of this bug: 603331
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
No longer blocks: 601619
You need to log in before you can comment on or make changes to this bug.