Closed Bug 875975 Opened 6 years ago Closed 6 years ago

Change - It should be possible to stop navigation by pressing the 'escape' key while a page is loading

Categories

(Firefox for Metro Graveyard :: Browser, defect, P3)

All
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 25

People

(Reporter: TimAbraldes, Assigned: almasry.mina)

References

Details

(Whiteboard: [shovel-ready] feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1)

Attachments

(1 file, 4 obsolete files)

In desktop Firefox, it is possible to stop navigation by pressing the 'escape' key while a page loads.  It is not currently possible to do this in Windows 8 Style Firefox.
Summary: Defect - It should be possible to stop navigation by pressing the 'escape' key while a page is loading → Change - It should be possible to stop navigation by pressing the 'escape' key while a page is loading
Whiteboard: feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
QA Contact: jbecerra
Priority: -- → P3
QA Contact: jbecerra
Whiteboard: feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → [shovel-ready] feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
Assignee: nobody → msamuel
Hi Marina, saw that you assigned yourself to this Change Story so I wanted to know if you are planning on working on this during IT#9 or at a future iteration?
Flags: needinfo?(msamuel)
Hi Marco, probably a future iteration. I'm focusing on downloads stuff for now.
Assignee: msamuel → nobody
Flags: needinfo?(msamuel)
Hi Marina, sounds good.  Thanks for the update.
I'd like to take a shot at this. I'm pretty new here, bbondy (or anyone willing) can you point out where I start with this, and how do I go about writing a test for it?
Flags: needinfo?(netzen)
(In reply to Mina Almasry from comment #5)
> I'd like to take a shot at this. I'm pretty new here, bbondy (or anyone
> willing) can you point out where I start with this, and how do I go about
> writing a test for it?

Hi Mina, you can already stop the navigation but you have to press escape twice.
Here is the line of code that does stop the current page from loading:
https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#1049

If we wanted to change that, I think we'd not return early from a couple lines above:

> if (ContextUI.dismiss()) {
>   return;
> }

You can find instructions for building the project locally here:
https://wiki.mozilla.org/Firefox/Windows_8_Integration#Building_Locally

After you make the change you should only need to build inside 
browser/metro by the way for the changes to come into effect.

You'll need to make the build you're using the default browser to use it in Metro.
Flags: needinfo?(netzen)
Before starting on this let's ask a front end team member if they want this change or not. If not then Mina, please feel free to pick another bug and we can help out there too.
Flags: needinfo?(mbrubeck)
Yes, we should do this.  In general we want to match desktop Firefox keyboard shortcuts (except in cases where they don't make sense for Metro), because doing otherwise leads to frustration for users switching between desktop and Metro.

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> If we wanted to change that, I think we'd not return early from a couple
> lines above:
> 
> > if (ContextUI.dismiss()) {
> >   return;
> > }

I think we still want to return early to avoid falling through to some of the later cases, but we can add a "stop page load" in both the early-return code path and the fall-through code path.
Flags: needinfo?(mbrubeck)
Attached patch patch (obsolete) — Splinter Review
Well that was pretty simple. Now hitting escape while a page is loading both dismisses the context UI and stops the page from loading. Do you need a test for this? I've been struggling writing one but I could get help on the IRC if it's needed.
(In reply to Matt Brubeck (:mbrubeck) [away until July 3] from comment #8)
> I think we still want to return early to avoid falling through to some of
> the later cases, but we can add a "stop page load" in both the early-return
> code path and the fall-through code path.

There are no later cases, but maybe you mean cases that will be added in the future.  It's basically the same thing but I think you suggested some code duplication.
Attachment #771516 - Flags: review?(mbrubeck)
Comment on attachment 771516 [details] [diff] [review]
patch

Or maybe we should just move the "stop()" block above the "ContextUI.dismiss()" block, so the first keypress would stop the pageload and the second keypress would hide the toolbar.  That would mean an extra keystroke if you want to do both things, but potentially less confusion from having two things happening at once...

Anyway, I'm fine with this patch for now; I think it's an improvement over what we have currently.  If you think my new suggestion above is worth trying, feel free to post a revised patch in this bug, or land this now and file a follow-up bug later.
Attachment #771516 - Flags: review?(mbrubeck) → review+
Attached patch patch with no code duplication (obsolete) — Splinter Review
I followed instructions but I too was afraid it's code duplication. I have made another patch without the code duplication. This patch also builds, accomplishes what we want, and passes all the existing mochitest-metros
Attachment #771748 - Flags: review?(mbrubeck)
I have made another patch with Matt's recommendations in comment #11: while a page is loading pressing esc once with stop loading, pressig another time will dismiss the context UI. I have to say I agree with Matt, it seemed pretty strange to have pressing esc do two things, and I think this behavior is better.
Attachment #771751 - Flags: review?(mbrubeck)
I like the idea of page load stop only for the first esc and then dismiss for the second.
Comment on attachment 771751 [details] [diff] [review]
patch: one esc -> stop loading, two esc -> dismiss ui

Yes, let's use this one.  Thanks!
Attachment #771751 - Flags: review?(mbrubeck) → review+
Assignee: nobody → netzen
Blocks: metrov1it10
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: [shovel-ready] feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → [shovel-ready] feature=change c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Attached patch patch (obsolete) — Splinter Review
I don't know if you've noticed yet, but I've been editing my patches in a way that breaks them. They don't look right in your review tool and hg probably won't accept them.

I've attached a correct patch this time, and marked all the others as obsolete. I've also added the commit message I was missing. Again, the behaviour is the same: one esc stops loading, the second dismisses the UI. Sorry about that.
Attachment #771516 - Attachment is obsolete: true
Attachment #771748 - Attachment is obsolete: true
Attachment #771751 - Attachment is obsolete: true
Attachment #771748 - Flags: review?(mbrubeck)
Attachment #771953 - Flags: review?(mbrueck)
Comment on attachment 771953 [details] [diff] [review]
patch

>-    if (ContextUI.dismiss()) {
>-      return;
>-    }
>-
>     if (Browser.selectedTab.isLoading()) {
>       Browser.selectedBrowser.stop();
>       return;
>     }
>+
>+    ContextUI.dismiss();
>+

Two nits: Please keep the "if" and "return" statements, since it will make it easier to correctly add more code below here in the future.  And please remove the blank line at the end of the function body.  (No need to post a new patch; if you'd like one of us to check in this patch for you, we can just fix those nits before pushing it.)  Thanks again!
Attachment #771953 - Flags: review?(mbrueck) → review+
Attached patch patchSplinter Review
No worries. Here is a patch with the edits.
Attachment #771953 - Attachment is obsolete: true
Attachment #772415 - Flags: review?(mbrubeck)
Attachment #772415 - Flags: review?(mbrubeck) → review+
Assignee: netzen → almasry.mina
Keywords: checkin-needed
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/7461c5ee0915
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20130715 Firefox/25.0
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130715 Firefox/25.0

Verified this fix on the latest nightly build.
At the first "ESC" key press, the page stops loading. 
If pressing "ESC" for the second time the UI is dismissed (a blank <white> page is shown).
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. At the first "ESC" key press, the page stops loading.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.