Closed Bug 897131 Opened 7 years ago Closed 7 years ago

Defect - URL field should display the current page URL after auto-complete is dismissed

Categories

(Firefox for Metro Graveyard :: General, defect, P2)

x86
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: ywang, Assigned: jimm)

References

Details

(Whiteboard: feature=defect c=tbd u=tbd p=1)

Attachments

(1 file)

Steps to reproduce: 
1. Open a webpage on Firefox Metro: mozilla.org, for example
2. Tap on the URL field
3. Type in search keywords: "Firefox". The auto-complete screen slides in and shows search results
4. Tap out to dismiss auto-complete screen
5. Edge swipe or right-click to show nav bar and tab strip.

Expected result:
The URL field should display the current page URL: mozilla.org

Actual result: 
The URL field displays search input: Firefox
Whiteboard: feature=defect c=tbd u=tbd p=0
Priority: -- → P2
Assignee: nobody → jmathies
OS: Mac OS X → Windows 8 Metro
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=1
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Attached patch fix plus testSplinter Review
Attachment #781010 - Flags: review?(jwilde)
Status: NEW → ASSIGNED
QA Contact: jbecerra
http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fmetro%2F&string=updateURI

Currently we only update in the web progress and when selecting a new tab.
Comment on attachment 781010 [details] [diff] [review]
fix plus test

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

Will change the r flag once I get my clobber build finished and can check this. Otherwise, looks good.

::: browser/metro/base/content/ContextUI.js
@@ +97,5 @@
>    displayNavUI: function () {
>      let shown = false;
>  
>      if (!this.navbarVisible) {
> +      BrowserUI.updateURI();

Any reason why we're putting this here rather than in displayNavbar?

::: browser/metro/base/tests/mochitest/browser_urlbar.js
@@ +243,5 @@
> +
> +    EventUtils.sendString("about:blank", window);
> +    let opened = yield waitForCondition(() => gEdit.popup.popupOpen);
> +    ok(opened, "type in urlbar: popup opens");
> +    

Nit: whitespace.

@@ +248,5 @@
> +    sendElementTap(window, tab.browser);
> +
> +    let closed = yield waitForCondition(() => !gEdit.popup.popupOpen);
> +    ok(closed, "autocomplete closed after tap on content");
> +    ok(!ContextUI.navbarVisible, "navbar closed"); 

Nit: whitespace.

@@ +254,5 @@
> +    let event = document.createEvent("Events");
> +    event.initEvent("MozEdgeUICompleted", true, false);
> +    window.dispatchEvent(event);
> +
> +    ok(ContextUI.navbarVisible, "navbar visible"); 

Nit: whitespace.
(In reply to Jonathan Wilde [:jwilde] from comment #3)
> Comment on attachment 781010 [details] [diff] [review]
> fix plus test
> 
> Review of attachment 781010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Will change the r flag once I get my clobber build finished and can check
> this. Otherwise, looks good.
> 
> ::: browser/metro/base/content/ContextUI.js
> @@ +97,5 @@
> >    displayNavUI: function () {
> >      let shown = false;
> >  
> >      if (!this.navbarVisible) {
> > +      BrowserUI.updateURI();
> 
> Any reason why we're putting this here rather than in displayNavbar?

That felt more like a direct control method vs something that has a lot of functionality embedded in it.
(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Jonathan Wilde [:jwilde] from comment #3)
> > Comment on attachment 781010 [details] [diff] [review]
> > fix plus test
> > 
> > Review of attachment 781010 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Will change the r flag once I get my clobber build finished and can check
> > this. Otherwise, looks good.
> > 
> > ::: browser/metro/base/content/ContextUI.js
> > @@ +97,5 @@
> > >    displayNavUI: function () {
> > >      let shown = false;
> > >  
> > >      if (!this.navbarVisible) {
> > > +      BrowserUI.updateURI();
> > 
> > Any reason why we're putting this here rather than in displayNavbar?
> 
> That felt more like a direct control method vs something that has a lot of
> functionality embedded in it.

Okay, makes sense. :)
Attachment #781010 - Flags: review?(jwilde) → review+
https://hg.mozilla.org/mozilla-central/rev/062d97d84821
https://hg.mozilla.org/mozilla-central/rev/b855e94a55fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130805030205

WFM
Tested on Windows 8 for iteration-11 using latest nightly built from http://hg.mozilla.org/mozilla-central/rev/482b9d04974a

I followed steps provided in comment0 and got expected result
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.