875892 - Defect - The urlbar auto dismisses when it shouldn't after loading a page

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

05-13 worked correctly but builds after this landed no longer works. The appbar navigation hides itself after page loads, but it should keep it open.

Comment 1

[Brian R. Bondy [:bbondy]]

Bug 835623 introduced this behavior

Comment 2

[Brian R. Bondy [:bbondy]]

Ccing marco for whiteboard header magic

Comment 3

[Brian R. Bondy [:bbondy]]

This is a P1 because it is a major phishing concern.

Comment 4

[Brian R. Bondy [:bbondy]]

p=1

Comment 5

[Jim Mathies [:jimm]]

I'd like to take this one in IT10 and do some work on ContextUI control, which is a bit of a mess currently.

Comment 6

[Jim Mathies [:jimm]]

Attachment: split out ContextUI from browser-ui.js

Comment 7

[Jim Mathies [:jimm]]

Attachment: cleanup contextui control v.1

I still have some work to do - cleanup of some of the props on context ui, rework the existing contextui tests, and add some more tests to it.

Comment 8

[Jim Mathies [:jimm]]

Attachment: cleanup contextui control v.1

Comment 9

[Jim Mathies [:jimm]]

Attachment: cleanup contextui control v.2

passes on try:

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=8b604745d15e

Comment 10

[Jim Mathies [:jimm]]

Attachment: tests update

Comment 11

[Jonathan Wilde [:jwilde]]

Comment on attachment 768465 [details] [diff] [review]
cleanup contextui control v.2

Review of attachment 768465 [details] [diff] [review]:
-----------------------------------------------------------------

Just two tweaks and this patch is ready to land. :D

::: browser/metro/base/content/ContextUI.js
@@ +57,2 @@
>    get isVisible() {
> +    return this.navbarVisible || this.tabbarVisible || this.appbarVisible;

In other parts of the app, we use "appbar" to mean "any sort of contextual toolbar that comes up from the bottom of the screen" and have code for both the navbar and contextual appbar in appbar.js. Calling the contextual appbar just "appbar" could get really confusing.

Can we change appbarVisible to contextAppbarVisible, displayAppbar to displayContextAppbar, etc?

--

Also, should we include the findbar visibility in this somehow? It's technically ContextUI... I'm pretty sure it uses appbar.xml and will fire a lot of the appbar events, too. Including the findbar visiblity can wait to a future patch, though.

::: browser/metro/base/content/appbar.js
@@ +16,4 @@
>      Elements.contextappbar.addEventListener('MozAppbarShowing', this, false);
>      Elements.contextappbar.addEventListener('MozAppbarDismissing', this, false);
> +
> +    // fired when a context sensitive item (bookmarks) changes state 

Nit: space at end of line.

Comment 12

[Jonathan Wilde [:jwilde]]

Comment on attachment 768466 [details] [diff] [review]
tests update

Review of attachment 768466 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/tests/mochitest/browser_context_ui.js
@@ +12,5 @@
>  gTests.push({
>    desc: "Context UI on about:start",
>    run: function testAboutStart() {
>      yield addTab("about:start");
> +    yield waitForMs(5000);

This is a really long wait. What changes would have to be made elsewhere to be able to remove this? If not, we need to comment the reason for inclusion.

::: browser/metro/base/tests/mochitest/head.js
@@ +183,5 @@
>          yield promise;
>        }
>      }
>  
> +    if (ContextUI.appbarVisible) {

As noted in the previous patch, we should rename this to contextAppbarVisible or the like.

Comment 13

[Jonathan Wilde [:jwilde]]

Comment on attachment 768466 [details] [diff] [review]
tests update

Review of attachment 768466 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/tests/mochitest/browser_context_ui.js
@@ +12,5 @@
>  gTests.push({
>    desc: "Context UI on about:start",
>    run: function testAboutStart() {
>      yield addTab("about:start");
> +    yield waitForMs(5000);

Talked with jimm. This was temporary debugging code that'll get dropped before landing and not a signal of any issues elsewhere that should get resolved. :D

Comment 14

[Jim Mathies [:jimm]]

Attachment: cleanup contextui control v.3

updated per comments.

Comment 15

[Jim Mathies [:jimm]]

Attachment: tests update v.2

tests with the debug timeout removed.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 768652 [details] [diff] [review]
tests update v.2

forgot to update the app bar prop.

Comment 17

[Jim Mathies [:jimm]]

Attachment: tests update v.2

Comment 18

[Jonathan Wilde [:jwilde]]

Comment on attachment 768651 [details] [diff] [review]
cleanup contextui control v.3

Review of attachment 768651 [details] [diff] [review]:
-----------------------------------------------------------------

Everything looks good. :D

Comment 19

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77a07b4d2d05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2debe9b0fc31
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b279a6c35aa6

Comment 20

[Jim Mathies [:jimm]]

Attachment: bugfix

Missed one dismissAppbar. I'll try to get this in with the other patches when they merge to mc.

Comment 21

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e146a60c58c

Comment 22

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa716235af8f

Bah!

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77a07b4d2d05
https://hg.mozilla.org/mozilla-central/rev/2debe9b0fc31
https://hg.mozilla.org/mozilla-central/rev/b279a6c35aa6

Comment 24

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/9df20557f30b
https://hg.mozilla.org/mozilla-central/rev/942686767e5e

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e146a60c58c
https://hg.mozilla.org/mozilla-central/rev/aa716235af8f

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20130715 Firefox/25.0

Verified this fix on latest Nightly. 
The urlbar now remains shown after loading a page.

Comment 27

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130816030205
Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0

WFM
Tested on windows 8 using latest nightly for iteration-12. The urlbar now remains shown after loading a page.