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

VERIFIED FIXED

Status

defect
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: bbondy, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Bug 835623 introduced this behavior
(Reporter)

Updated

6 years ago
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
(Reporter)

Comment 2

6 years ago
Ccing marco for whiteboard header magic
(Reporter)

Updated

6 years ago
Priority: -- → P1
(Reporter)

Comment 3

6 years ago
This is a P1 because it is a major phishing concern.
Whiteboard: feature=defect c=main_ui_reorganization u=metro_firefox_user p=0
(Reporter)

Comment 4

6 years ago
p=1
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: metrov1it10
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 7

6 years ago
Posted 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.
(Assignee)

Comment 8

6 years ago
Posted patch cleanup contextui control v.1 (obsolete) — Splinter Review
Attachment #768318 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Posted 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)
(Assignee)

Comment 10

6 years ago
Posted patch tests update (obsolete) — Splinter Review
Attachment #768466 - Flags: review?(jwilde)
(Assignee)

Updated

6 years ago
Blocks: 859274
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 14

6 years ago
updated per comments.
Attachment #768465 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Posted patch tests update v.2 (obsolete) — Splinter Review
tests with the debug timeout removed.
Attachment #768466 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #768651 - Flags: review?(jwilde)
(Assignee)

Updated

6 years ago
Blocks: 888091
(Assignee)

Comment 16

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

forgot to update the app bar prop.
Attachment #768652 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
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+
(Assignee)

Comment 20

6 years ago
Posted patch bugfixSplinter Review
Missed one dismissAppbar. I'll try to get this in with the other patches when they merge to mc.
(Assignee)

Updated

6 years ago
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.