601619 - page title shows up on urlbar when flipping through awesomescreen panes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b7pre) Gecko/20101001 Firefox/4.0b7pre Fennec/4.0b1

Steps to Reproduce:
1. Go to bugzilla.mozilla.org
2. Click on the urlbar to open the awesomescreen
3. Click on "Bookmarks"
4. Click on "History"

Actual Results:
The page title is shown on the urlbar in steps 3 and 4. This is more obvious if you clear out the contents of the urlbar in between steps 2 and 3.

Expected Results:
The url of the currently loaded page should be shown on the urlbar.

Comment 1

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

Madhava - I assume we want to _not_ show the page title? When do we want to show the page title now? Never?

Comment 2

[Madhava Enros [:madhava]]

Yeah - just URL (or user-entered stuff) when on the awesomescreen.

Comment 3

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

Attachment: Patch

Comment 4

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

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

I'm a little worried that this could regress some of the recent, and not so recent, fixes. This code is fragile.

Looks OK though, but please test this a lot before landing.

Comment 5

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

Attachment: Patch v0.2

This patch is a roll-up of this patch and it fixes the regression of the urlbar (bug 603227)

Comment 6

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

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

wrong version of the patch, it misses some changes

Comment 7

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

Attachment: Patch v0.2

Comment 8

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

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


>diff -r a541d6f990e7 chrome/content/browser-ui.js

>   set activePanel(aPanel) {

>     this._activePanel = aPanel;
>+    if (willHidePanel || willShowPanel) {
>+      // XXX
>+      let event = document.createEvent("UIEvents");
>+      event.initUIEvent("AwesomePanel", true, true, window, willShowPanel);
>+      window.dispatchEvent(event);
>+    }

Remove the "// XXX" or add a real comment
Also, lets use "NavigationPanel" for the event name.

>+      case "pagehide":
>+        // XXXvn
>+        // On a pagehide the content can dismiss the VKB, we're trying to avoid
>+        // that by adding it back again. We should really fix the real bug
>+        let utils = Util.getWindowUtils(window);
>+        if (this.activePanel && !this._edit.readOnly && browser.currentURI.spec != "about:blank" && utils.IMEStatus == 0) {
>+          this._edit.readOnly = !this._edit.readOnly;
>+          this._edit.readOnly = !this._edit.readOnly;
>+        }

Make sure a bug is filed for this problem. I want this code to be removed ASAP


>diff -r a541d6f990e7 chrome/content/content.js

>     switch (aMessage.name) {
>-      case "Browser:Blur":
>-        gFocusManager.clearFocus(content);
>+      case "Browser:Blur": {
>+        // We want to clear the focus _only_ is the VKB is displayed for an
>+        // element within this particular window
>+        let utils = Util.getWindowUtils(content);
>+        if (utils.IMEStatus != 0 && gFocusManager.activeWindow && gFocusManager.focusedElement &&
>+            gFocusManager.focusedElement.mozIsTextField && gFocusManager.focusedElement.mozIsTextFied(true))
>+          gFocusManager.clearFocus(content);

This is a huge if check. How about:

try {
  if (utils.IMEStatus != 0 && gFocusManager.activeWindow && gFocusManager.focusedElement.mozIsTextField(true))
    gFocusManager.clearFocus(content)
} catch(e) {}

note that mozIsTextFied -> mozIsTextField

>diff -r a541d6f990e7 chrome/tests/browser_awesomescreen.js

>+    window.addEventListener("AwesomePanel", function(aEvent) {
>+      window.removeEventListener(aEvent.type, arguments.callee, true);
>+      gCurrentTest.onPopupReady();
>+    }, true);

Should you be checking that the aEvent.detail is true (for open)? (and in other places too)


>diff -r a541d6f990e7 chrome/tests/browser_sessionstore.js

>     // Need to wait until the page is loaded
>     messageManager.addMessageListener("pageshow",
>     function(aMessage) {
>       if (gCurrentTest._currentTab.browser.currentURI.spec != "about:blank") {
>         messageManager.removeMessageListener(aMessage.name, arguments.callee);
>-        gCurrentTest.onPageReady();
>+        //XXX why do we need to put this load into a timeout?
>+        setTimeout(function() { gCurrentTest.onPageReady(); }, 400);

The session store is also using "pageshow" to attach to the tab. There could be a race condition. Do you need to use 400? Can you just use 0?

Comment 9

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

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

r- until you tell me if the changes are going to work or not

Comment 10

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

Attachment: Patch v0.3

The patch address the comments you made, it is also based on a platform patch that prevent content to dismiss the IME it is already opened on chrome and the current focused element is an editable element.

I think the platform patch still need some work since it probably need to check for the IMEStatus also to have a guess if the user has asked to close the VKB or not.

Comment 11

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

Attachment: Patch v0.4

Here what I think about this bug and how we should handle it.

If we want to have this patch for b2 I think we should land it as if and remove the //XXX bug 604192 once the platform patch will land since we're unsure it will make b2. We should also probably block on bug 604192 for the final release.

Comment 12

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

(In reply to comment #8)

> >-        gCurrentTest.onPageReady();
> >+        //XXX why do we need to put this load into a timeout?
> >+        setTimeout(function() { gCurrentTest.onPageReady(); }, 400);
> 
> The session store is also using "pageshow" to attach to the tab. There could be
> a race condition. Do you need to use 400? Can you just use 0?

I'm have tried with 0 and it doesn't work, something else happened. But I think we should track the session_store case into an other bug since it has other issues, like not working the same way depending if it is running alone or in the full testsuite.

Comment 13

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

Attachment: Patch v0.4

Remove an unused let HTMLInputElement = Ci.nsIDOMHTMLInputElement

Comment 14

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

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


>+      case "pagehide":
>+        // XXX bug 60419, when a content web page is close the content sometimes

bug 604192
>+        // dismiss the VKB, we're trying to avoid that by adding it back again.
>+        let utils = Util.getWindowUtils(window);
>+        if (this.activePanel && !this._edit.readOnly && browser.currentURI.spec != "about:blank" && utils.IMEStatus == 0) {

Can we use the real constant here?  utils.IMEStatus == utils.IME_STATUS_DISABLED

>+      case "Browser:Blur": {

>+        let utils = Util.getWindowUtils(content);
>+        let focusedElement = gFocusManager.focusedElement;
>+        if (utils.IMEStatus != 0 && (focusedElement && focusedElement.mozIsTextField && focusedElement.mozIsTextFied(false)))

utils.IMEStatus != utils.IME_STATUS_DISABLED

focusedElement.mozIsTextFied(false) -> focusedElement.mozIsTextField(false)

(how broken was this with the function name spelled wrong?)

>diff -r 2a0e63ecb5b2 chrome/tests/browser_sessionstore.js

>     // Need to wait until the page is loaded
>     messageManager.addMessageListener("pageshow",
>     function(aMessage) {
>       if (gCurrentTest._currentTab.browser.currentURI.spec != "about:blank") {
>         messageManager.removeMessageListener(aMessage.name, arguments.callee);
>-        gCurrentTest.onPageReady();
>+        //XXX why do we need to put this load into a timeout?
>+        setTimeout(function() { gCurrentTest.onPageReady(); }, 400);

Remove this and let's fix in a new bug. I don't want this to be "fixed" and forgotten

r+ with changes

Comment 15

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

http://hg.mozilla.org/mobile-browser/rev/95b66089dfc3

Comment 16

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

I've filled bug 604353 for the browser_viewport.js tests

Comment 17

[Daniel Holbert [:dholbert]]

The dev-tree-management bot says this caused a 5.75% Txul regression on n900:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d5998ae036159346
Is that expected/acceptable?

Comment 18

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

(In reply to comment #17)
> The dev-tree-management bot says this caused a 5.75% Txul regression on n900:
> http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/d5998ae036159346
> Is that expected/acceptable?

It is acceptable to me. The bugs this fixes are more important than the 40ms we lost. This patch also has some hacks in it that will go away when bug 604192 lands. We can check to see if the hacks, especially the "pagehide" code is the cause of the regression.

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101015 Namoroka/4.0b8pre Fennec/4.0b2pre

and

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

Comment 20

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15192