Closed
Bug 875892
Opened 12 years ago
Closed 11 years ago
Defect - The urlbar auto dismisses when it shouldn't after loading a page
Categories
(Firefox for Metro Graveyard :: App Bar, defect, P1)
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)
16.58 KB,
patch
|
Details | Diff | Splinter Review | |
20.44 KB,
patch
|
jwilde
:
review+
|
Details | Diff | Splinter Review |
6.90 KB,
patch
|
Details | Diff | Splinter Review | |
669 bytes,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Bug 835623 introduced this behavior
Reporter | ||
Updated•12 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•12 years ago
|
||
Ccing marco for whiteboard header magic
Reporter | ||
Updated•12 years ago
|
Priority: -- → P1
Reporter | ||
Comment 3•12 years ago
|
||
This is a P1 because it is a major phishing concern.
Updated•12 years ago
|
Blocks: metrov1defect&change
Whiteboard: feature=defect c=main_ui_reorganization u=metro_firefox_user p=0
Reporter | ||
Comment 4•12 years ago
|
||
p=1
Assignee | ||
Comment 5•11 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•11 years ago
|
Blocks: metrov1it10
Assignee | ||
Updated•11 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
Updated•11 years ago
|
No longer blocks: metrov1defect&change
Updated•11 years ago
|
Status: NEW → ASSIGNED
QA Contact: jbecerra
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
Attachment #768318 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #768343 -
Attachment is obsolete: true
Attachment #768465 -
Flags: review?(jwilde)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #768466 -
Flags: review?(jwilde)
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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 13•11 years ago
|
||
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•11 years ago
|
||
updated per comments.
Attachment #768465 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
tests with the debug timeout removed.
Attachment #768466 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #768651 -
Flags: review?(jwilde)
Assignee | ||
Comment 16•11 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•11 years ago
|
||
Comment 18•11 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
Updated•11 years ago
|
Attachment #768651 -
Flags: review?(jwilde) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Assignee | ||
Comment 20•11 years ago
|
||
Missed one dismissAppbar. I'll try to get this in with the other patches when they merge to mc.
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/77a07b4d2d05
https://hg.mozilla.org/mozilla-central/rev/2debe9b0fc31
https://hg.mozilla.org/mozilla-central/rev/b279a6c35aa6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
Comment 26•11 years ago
|
||
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
Comment 27•11 years ago
|
||
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.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•