606595 - tapping on, but releasing off, titlebutton should not bring up awesomescreen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Right now:

- I tap on the titlebutton (the part of the titlebar with the title in it)
   - it visually changes to a field
- I realize that I don't want to go to the awesomescreen, so I drag off of the button and lift my finger
- I still go to the awesomescreen anyway -- the button-press is not canceled

This should cancel the button press.

Comment 1

[Matt Brubeck (:mbrubeck)]

This is a consequence of bug 574116.  This could be fixed by using a <label> or <button> element for the titlebar, instead of a <textbox>.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch adds a <label> that only displays the title.  In "edit mode", we hide the <label> and show the <textbox>, which now only displays the URL.

This fixes the bug described here.  It also fixes a bug where dragging on the titlebar selects the title.  It should preserve all other current behavior, including bug 596614 and bug 593456.

The patch also cleans up some old code:
* Remove the "ignoreDrag" property, no longer needed.
* Remove the selection hacks from bug 605303, no longer needed.
* Remove some unused attributes from the #urlbar-edit textbox.
* Crops button-bg.png from 800px to 64px (to save 1KB) and uses repeat-x.

Comment 3

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

Comment on attachment 486734 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
>       <handler event="focus" phase="bubbling">
>         <![CDATA[
>-          if (this.clickSelectsAll && !this.readOnly)
>+          if (this.clickSelectsAll)
>             this.select();
>         ]]>
>       </handler>

Any reason for removing the readOnly check here, it is to prevent calling blur/focus unexpectingly (which are called when the readOnly state changed to refresh the IME state)

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


>   updateAwesomeHeader: function updateAwesomeHeader(aString) {

>-    if (aPanel) {
>+    if (aPanel)
>       aPanel.open();
>-      if (this._edit.value == "")
>-        this._showURI();
>-    }

This line is here to ensure we are showing the URL when we're opening an other panel than the AllPagesList and the input text is empty for some reasons (user has deleted text for example)
Can you ensure we're not regressing?

>       // URL textbox events
>       case "click":
>-        // if there is an already opened panel, keep it active (bug 596614).
>-        if (this.activePanel && this._edit.readOnly)
>+        if (this._edit.readOnly)
>           this._edit.readOnly = false;
>-        else if (!this.activePanel)
>-          AllPagesList.doCommand();

Does this change means if someone clicks on the urlbar while it is not readonly, (e.g when it has already the focus) we would bring the AllPagesList?
If yes, this is not what we want.

>@@ -854,28 +843,34 @@ var BrowserUI = {
>         }
>         break;
>       // Favicon events
>       case "error":
>         this._favicon.src = "";
>         break;
>       // Awesome popup event
>       case "NavigationPanelShown":
>-        this._edit.setAttribute("open", "true");
>+        this._edit.collapsed = false;
>+        this._title.collapsed = true;
>+
>+        if (!this._edit.readOnly)
>+          this._edit.focus();

Could you nest the focus call inside the binding?

> 
>         // Disabled the search button if no search engines are available
>         let button = document.getElementById("urlbar-icons");
>         if (BrowserSearch.engines.length)
>           button.removeAttribute("disabled");
>         else
>           button.setAttribute("disabled", "true");
> 
>         break;
>       case "NavigationPanelHidden":
>-        this._edit.removeAttribute("open");
>+        this._edit.collapsed = true;
>+        this._title.collapsed = false;
>+

Since this._edit is the only things used for those events, I think we could move them safely to the binding.
This will prevent to spread this._edit everywhere.

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul
>                     <textbox id="urlbar-edit"
>                              type="autocomplete"
>-                             class="uri-element"

This is needed for RTL to force the display of this box to LTR which is the classical behavior for a textbox containing URL.

Does browser_awesomescreen.js tests are still working or do you think or any tests we can add?

f- because I'm scared of those changes, I think I would like to see some new tests and to be sure all the existing one are working

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to comment #3)
> >-          if (this.clickSelectsAll && !this.readOnly)
> >+          if (this.clickSelectsAll)
> >             this.select();
> 
> Any reason for removing the readOnly check here, it is to prevent calling
> blur/focus unexpectingly (which are called when the readOnly state changed to
> refresh the IME state)

This was an attempt to fix a bug where the URL is not highlighted when the edit field first appears in readOnly mode - but it didn't work.  I can restore this line, but it doesn't seem to make a difference either way.  (What's a good test case for this?)

> >     if (aPanel) {
> >       aPanel.open();
> >-      if (this._edit.value == "")
> >-        this._showURI();
> >     }
> 
> This line is here to ensure we are showing the URL when we're opening an other
> panel than the AllPagesList and the input text is empty for some reasons (user
> has deleted text for example)
> Can you ensure we're not regressing?

This did regress.  I'll restore that line.

> >       // URL textbox events
> >       case "click":
> >-        // if there is an already opened panel, keep it active (bug 596614).
> >-        if (this.activePanel && this._edit.readOnly)
> >+        if (this._edit.readOnly)
> >           this._edit.readOnly = false;
> >-        else if (!this.activePanel)
> >-          AllPagesList.doCommand();
> 
> Does this change means if someone clicks on the urlbar while it is not
> readonly, (e.g when it has already the focus) we would bring the AllPagesList?
> If yes, this is not what we want.

No.  The reason for this change is that the "else if" case is now handled by the command attribute on the #urlbar-title element.  The #urlbar-edit element is no longer visible while there is no active panel, so it can't be clicked.  Clicking on the urlbar while it's already focused does not change the panel (no change to existing behavior).

> >       case "NavigationPanelShown":
> >+        this._edit.collapsed = false;
> >+        this._title.collapsed = true;
> >+
> >+        if (!this._edit.readOnly)
> >+          this._edit.focus();
> 
> Could you nest the focus call inside the binding?
> 
> >       case "NavigationPanelHidden":
> >+        this._edit.collapsed = true;
> >+        this._title.collapsed = false;
> 
> Since this._edit is the only things used for those events, I think we could
> move them safely to the binding.

I'll try this.

> >-                             class="uri-element"
> 
> This is needed for RTL to force the display of this box to LTR which is the
> classical behavior for a textbox containing URL.

Oops!  Restored.  Looks like I should add this to the new uri-title <label> too.

> Does browser_awesomescreen.js tests are still working or do you think or any
> tests we can add?

Looks like I broke some tests.  I'll make sure to at least fix those and post a new patch.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

This addresses all of Vivien's comments, except for moving NavigationPanel handlers to the autocomplete binding.  (I tried that and it didn't seem any simpler to me; if you want you can do it in a separate bug.)

All tests are passing with this patch, some new checks are added for the new title element, and some previously-disabled tests are now enabled.

Comment 6

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

Comment on attachment 487608 [details] [diff] [review]
patch v2

I guess we can move the NavigationPanel[Show/Hide] events into another buf if needed.
Thanks for adressing comments!

Comment 7

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

Comment on attachment 487608 [details] [diff] [review]
patch v2


>   _showURI: function _showURI() {
>     // Replace the web page title by the url of the page
>     let urlString = this.getDisplayURI(Browser.selectedBrowser);
>     if (Util.isURLEmpty(urlString))
>       urlString = "";
> 
>     this._edit.value = urlString;
>-  },
>+   },

Added a space

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

>+                    <label id="urlbar-title" class="uri-element" crop="end" flex="1"
>+                           command="cmd_openLocation" onclick="this.doCommand();"
>+                           placeholder="&urlbar.emptytext;"/>
>                     <textbox id="urlbar-edit"

>-                             enablehistory="false"
>-                             maxrows="6"

Why remove these?

Also, I wonder if we should try to move the label inside the textbox and move more of the label / textbox state toggling into the binding. Maybe in the future.
Could be less clutter in front-end JS

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

> /* URL bar ----------------------------------------------------------------- */
> #urlbar-container {
>   color: #000;
>   border-radius: 8px;
>   margin: 8px;
>-  background-image: url("chrome://browser/skin/images/button-bg.png");
>+  background: url("chrome://browser/skin/images/button-bg.png") bottom left repeat-x;

Watch Ts on this change. Using the repeat with a smaller background image could be slower

r+ with nits addressed

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to comment #7)
> >-                             enablehistory="false"
> >-                             maxrows="6"
> 
> Why remove these?

enablehistory="false" is the default.

maxrows="6" seems to be ignored, since we provide our own rows and there are 24 of them.

> Also, I wonder if we should try to move the label inside the textbox and move
> more of the label / textbox state toggling into the binding. Maybe in the
> future.

I'll file a followup for this; it would probably go well with Vivien's event handling suggestion.

Comment 9

[Matt Brubeck (:mbrubeck)]

Pushed with whitespace fix: http://hg.mozilla.org/mobile-browser/rev/14a1648e0302

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101103 Firefox/4.0b8pre Fennec/4.0b3pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101103 Firefox/4.0b8pre Fennec/4.0b3pre