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

Status: VERIFIED FIXED

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

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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.

Comment 1

[Brian R. Bondy [:bbondy]]

p=1

Comment 2

[Marco Mucci [:MarcoM]]

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?

Comment 3

[Marina Samuel [:emtwo]]

Hi Marco, probably a future iteration. I'm focusing on downloads stuff for now.

Comment 4

[Marco Mucci [:MarcoM]]

Hi Marina, sounds good.  Thanks for the update.

Comment 5

[Mina Almasry]

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?

Comment 6

[Brian R. Bondy [:bbondy]]

(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.

Comment 7

[Brian R. Bondy [:bbondy]]

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.

Comment 8

[Matt Brubeck (: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.

Comment 9

[Mina Almasry]

Attachment: patch

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.

Comment 10

[Brian R. Bondy [:bbondy]]

(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.

Comment 11

[Matt Brubeck (: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.

Comment 12

[Mina Almasry]

Attachment: patch with no code duplication

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

Comment 13

[Mina Almasry]

Attachment: patch: one esc -> stop loading, two esc -> dismiss ui

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.

Comment 14

[Brian R. Bondy [:bbondy]]

I like the idea of page load stop only for the first esc and then dismiss for the second.

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 771751 [details] [diff] [review]
patch: one esc -> stop loading, two esc -> dismiss ui

Yes, let's use this one.  Thanks!

Comment 16

[Mina Almasry]

Attachment: patch

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.

Comment 17

[Matt Brubeck (:mbrubeck)]

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!

Comment 18

[Mina Almasry]

Attachment: patch

No worries. Here is a patch with the edits.

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7461c5ee0915

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7461c5ee0915

Comment 21

[Cornel Ionce [:noni] [Hubs QA]]

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).

Comment 22

[Samvedana (:Samvedana)]

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.