Closed Bug 875892 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bbondy, Assigned: jimm)

References

Details

(Whiteboard: feature=defect c=main_ui_reorganization u=metro_firefox_user p=5)

Attachments

(4 files, 5 obsolete files)

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.
Bug 835623 introduced this behavior
Summary: Work - The urlbar auto dismisses when it shouldn't after loading a page → Defect - The urlbar auto dismisses when it shouldn't after loading a page
Ccing marco for whiteboard header magic
Priority: -- → P1
This is a P1 because it is a major phishing concern.
Whiteboard: feature=defect c=main_ui_reorganization u=metro_firefox_user p=0
p=1
I'd like to take this one in IT10 and do some work on ContextUI control, which is a bit of a mess currently.
Assignee: nobody → jmathies
Blocks: metrov1it10
Whiteboard: feature=defect c=main_ui_reorganization u=metro_firefox_user p=0 → feature=defect c=main_ui_reorganization u=metro_firefox_user p=5
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Attached patch cleanup contextui control v.1 (obsolete) — Splinter Review
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.
Attached patch cleanup contextui control v.1 (obsolete) — Splinter Review
Attachment #768318 - Attachment is obsolete: true
Attached patch cleanup contextui control v.2 (obsolete) — Splinter Review
passes on try:

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=8b604745d15e
Attachment #768343 - Attachment is obsolete: true
Attachment #768465 - Flags: review?(jwilde)
Attached patch tests update (obsolete) — Splinter Review
Attachment #768466 - Flags: review?(jwilde)
Blocks: 859274
Blocks: 879232
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.
Attachment #768465 - Flags: review?(jwilde) → review+
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.
Attachment #768466 - Flags: review?(jwilde) → review-
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
Attachment #768466 - Flags: review- → review+
updated per comments.
Attachment #768465 - Attachment is obsolete: true
Attached patch tests update v.2 (obsolete) — Splinter Review
tests with the debug timeout removed.
Attachment #768466 - Attachment is obsolete: true
Attachment #768651 - Flags: review?(jwilde)
Blocks: 888091
Comment on attachment 768652 [details] [diff] [review]
tests update v.2

forgot to update the app bar prop.
Attachment #768652 - Attachment is obsolete: true
Attached patch tests update v.2Splinter Review
Comment on attachment 768651 [details] [diff] [review]
cleanup contextui control v.3

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

Everything looks good. :D
Attachment #768651 - Flags: review?(jwilde) → review+
Attached patch bugfixSplinter Review
Missed one dismissAppbar. I'll try to get this in with the other patches when they merge to mc.
Blocks: 887213
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.
Status: RESOLVED → VERIFIED
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.