Bug 59731 – REGRESSION(r83522): backspace should not go back on Linux

• Component: UI Events
• Status: RESOLVED FIXED

[Evan Martin]

Linux Chrome and WebKitGtk (not sure about Qt) intentionally do not have the backspace-goes-back behavior.

This regressed in http://trac.webkit.org/changeset/83522 .

[Evan Martin]

Created attachment 91549 [details]
Patch

[Evan Martin]

ap, if you have any advice on how to test this (like a pointer to a test that covers something similar), I'd appreciate it.  It's not clear to me how to handle "back", since it navigates away from the current page.

[Ryosuke Niwa]

(In reply to comment #2)
> ap, if you have any advice on how to test this (like a pointer to a test that covers something similar), I'd appreciate it.  It's not clear to me how to handle "back", since it navigates away from the current page.

You can use alert because it'll be included in expected.txt

[Darin Adler]

(In reply to comment #2)
> ap, if you have any advice on how to test this (like a pointer to a test that covers something similar), I'd appreciate it.  It's not clear to me how to handle "back", since it navigates away from the current page.

There are many tests that go forward and back. Including most of the tests in fast/history and many others like these:

./css/target-fragment-match.html
./dom/DeviceMotion/resources/cached-page-2.html
./dom/DeviceOrientation/resources/cached-page-2.html
./dom/Geolocation/resources/cached-page-2.html
./dom/location-hash.html
./dom/Window/timer-resume-on-navigation-back.html
./dom/Window/window-appendages-cleared.html
./events/pageshow-pagehide-on-back-cached-with-frames.html
./events/pageshow-pagehide-on-back-cached.html
./events/pageshow-pagehide-on-back-uncached.html
./events/resources/pagehide-timeout-go-back.html
./forms/button-state-restore.html
./forms/select-no-name.html
./forms/state-restore-radio-group.html
./forms/state-restore-to-non-autocomplete-form.html
./forms/state-restore-to-non-edited-controls.html
./forms/state-save-of-detached-control.html
./frames/resources/cached-page-2.html
./frames/resources/cached-page-3.html
./frames/sandboxed-iframe-history-denied.html
./harness/resources/cached-page-2.html
./harness/use-page-cache.html
./history/history-back-forward-within-subframe-hash.html
./history/history-back-initial-vs-final-url.html
./history/history-back-within-subframe-hash.html
./history/history-back-within-subframe-url.html
./history/history-length.html
./history/history-subframe-with-name.html
./history/history-traversal-is-asynchronous.html
./history/resources/history-back-within-subframe-hash-2.html
./history/resources/history-back-within-subframe-url-2.html
./history/resources/history-replace-updates-current-item-goback.html
./history/same-document-iframes-changing-fragment.html
./history/same-document-iframes-changing-pushstate.html
./history/saves-state-after-fragment-nav.html
./history/saves-state-after-frame-nav.html
./loader/hashchange-event-properties.html
./loader/input-element-page-cache-crash.html
./loader/resources/empty-document-goes-back.html
./loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html
./loader/stateobjects/document-destroyed-navigate-back.html
./loader/stateobjects/popstate-after-load-complete-addeventlistener.html
./loader/stateobjects/popstate-after-load-complete-body-attribute.html
./loader/stateobjects/popstate-after-load-complete-body-inline-attribute.html
./loader/stateobjects/popstate-after-load-complete-window-attribute.html
./loader/stateobjects/popstate-fires-on-history-traversal.html
./loader/stateobjects/popstate-fires-with-page-cache.html
./loader/stateobjects/pushstate-clears-forward-history.html
./loader/stateobjects/pushstate-object-types.html
./loader/stateobjects/pushstate-then-replacestate.html
./loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html
./loader/stateobjects/pushstate-within-popstate-handler-assert.html
./loader/stateobjects/replacestate-then-pushstate.html
./loader/stateobjects/resources/navigate-back.html
./loader/stateobjects/resources/pushstate-in-iframe-child.html
./media/media-query-list-04.html
./overflow/horizontal-scroll-after-back.html
./viewport/viewport-128.html

[Evan Martin]

Comment on attachment 91549 [details]
Patch

clearing r? because I need a test.

[Early Warning System Bot]

Attachment 91549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8521023

[WebKit Review Bot]

Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8514761

[Alexey Proskuryakov]

Comment on attachment 91549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91549&action=review

Could you please post a rationale for this difference in behavior? I'm not saying that we should discuss whether it's good or bad now, but it would be good to have it for posterity, since it's so surprising.

> Source/WebCore/page/EventHandler.cpp:2908
> +    if (event->ctrlKey() || event->metaKey() || event->altKey() || event->altGraphKey()
> +        || !m_frame->editor()->behavior()->shouldGoBackOnBackspace())
>          return;

My preferred style would be to have a separate if() for the other check, since it's so different in nature. But it's not a big thing.

[Evan Martin]

(In reply to comment #8)
> Could you please post a rationale for this difference in behavior? I'm not saying that we should discuss whether it's good or bad now, but it would be good to have it for posterity, since it's so surprising.

It's a feature that's easy to trigger unintentionally, and on some websites (especially those that construct form controls dynamically), you can lose form state.  There are lots of alternative ways to go back (alt-left, or the 'back' button found on some keyboards or mice) so it's not a huge loss not to have.  It's sorta like how Firefox has '/' as a key to search -- it's cool if you know what it does, but annoying if it starts happening at the wrong times.

It turned out that Linux Firefox disabled backspace-to-go-back (I don't know why) and the other Linux browsers had matched Firefox, so we (Chrome) disabled it too to match other browsers.  Peter (our guy who cares the most about keybindings) said he would've loved to remove it on Windows for the above data-loss reason but people expect it there now.

If it were something like alt-backspace I'd likely have no problem with it.  But I in fact discovered this had regressed when I was typing a long message in some web form (much like this one) and somehow accidentally focused out of the field, hit backspace, and lost my message.  From looking at the timelines involved I'd say I was running a browser with this regression for about a week before I noticed, which means to me that the data loss problem isn't frequent but it's also not rare.


> > Source/WebCore/page/EventHandler.cpp:2908
> > +    if (event->ctrlKey() || event->metaKey() || event->altKey() || event->altGraphKey()
> > +        || !m_frame->editor()->behavior()->shouldGoBackOnBackspace())
> >          return;
> 
> My preferred style would be to have a separate if() for the other check, since it's so different in nature. But it's not a big thing.

Hah, I had the same thought; Rysouke was looking over my shoulder when I wrote this and suggested this way, but I'm gonna combine my vote with yours and change this.

[Ryosuke Niwa]

(In reply to comment #9)
> > > Source/WebCore/page/EventHandler.cpp:2908
> > > +    if (event->ctrlKey() || event->metaKey() || event->altKey() || event->altGraphKey()
> > > +        || !m_frame->editor()->behavior()->shouldGoBackOnBackspace())
> > >          return;
> > 
> > My preferred style would be to have a separate if() for the other check, since it's so different in nature. But it's not a big thing.
> 
> Hah, I had the same thought; Rysouke was looking over my shoulder when I wrote this and suggested this way, but I'm gonna combine my vote with yours and change this.

I'm fine with a separate if statement as well.

[Build Bot]

Attachment 91549 [details] did not build on win:
Build output: http://queues.webkit.org/results/8511895

[WebKit Review Bot]

Attachment 91549 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8516624

[WebKit Review Bot]

Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8521044

[Darin Adler]

To me this seems like a port/browser preference, not a platform editing style quirk.

[Darin Adler]

Many Safari users dislike this feature; we’ve considered removing it many times.

Many other Safari users would be confused if the feature went away. Some use it as their primary way to go back.

[Alexey Proskuryakov]

I actually like the way it's implemented. Although it's a port (or embedder) decision, this is a decision that's made largely based on what dominant browsers on the platform do, and in that sense it's a platform behavior.

We can switch to a more complicated solution if some ports start to disagree, but currently platform aligns with this policy well.

[WebKit Review Bot]

Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8516634

[Evan Martin]

The two main port flavors I had worried about are:
1) ChromeOS -- wants Windows-style editing, also wants backspace-goes-back, so it's fine.
2) Linux-based phones (Qt? Android?), not sure what happens there.

[Ryosuke Niwa]

(In reply to comment #18)
> The two main port flavors I had worried about are:
> 1) ChromeOS -- wants Windows-style editing, also wants backspace-goes-back, so it's fine.
> 2) Linux-based phones (Qt? Android?), not sure what happens there.

Qt folks told me they're fine with this.

[Antonio Gomes]

(In reply to comment #14)
> To me this seems like a port/browser preference, not a platform editing style quirk.

I agree 100%. It has nothing to do with editing quirk, imo.

[Ryosuke Niwa]

(In reply to comment #20)
> (In reply to comment #14)
> > To me this seems like a port/browser preference, not a platform editing style quirk.
> 
> I agree 100%. It has nothing to do with editing quirk, imo.

We can certainly add a setting but adding a new setting/preference seems like an overkill for a tiny feature like this especially because all ports happen to agree on the same behavior.

If anything, this seems like something ports should override default behavior in either EventHandler or in WebKit layer.

[Collabora GTK+ EWS bot]

Attachment 91549 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8516740

[Peter Kasting]

(In reply to comment #16)
> I actually like the way it's implemented. Although it's a port (or embedder) decision, this is a decision that's made largely based on what dominant browsers on the platform do, and in that sense it's a platform behavior.

I don't mind this being a platform behavior.  I do wonder whether tying this to the specific "unix editing behavior" flag is the right way to make it a platform behavior.  This is arguably not "editing" behavior, it's more like "unix other stuff behavior".  I'm not sure we have a notion of that right now.

It seemed like in the pre-r83522 days implementing this was entirely up to the embedder (?), which also seemed fine to me.

[Ryosuke Niwa]

(In reply to comment #23)
> I don't mind this being a platform behavior.  I do wonder whether tying this to the specific "unix editing behavior" flag is the right way to make it a platform behavior.  This is arguably not "editing" behavior, it's more like "unix other stuff behavior".  I'm not sure we have a notion of that right now.

It's editing in the sense of the editing component.  The editing component implements selection, caret, all editing (forms and content-editable region), hit testing, etc...

[Peter Kasting]

(In reply to comment #24)
> It's editing in the sense of the editing component.  The editing component implements selection, caret, all editing (forms and content-editable region), hit testing, etc...

Yeah, but... this is none of the above?  It's just a generic browser navigation shortcut.  Would "editing" be correct for a flag about whether ctrl-shift-t should reopen a closed tab?

I dunno, maybe there isn't anything better we can do here.  Just seems weird to me.

[Evan Martin]

Created attachment 92150 [details]
Patch

[Evan Martin]

I agree that editing behavior is slightly weird, but it is the closest to a logical component for this kind of thing.  It's even adjustable in layout tests, so I was able to verify both behaviors (going back or not going back) in the test I just added.

[Ryosuke Niwa]

Comment on attachment 92150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92150&action=review

> LayoutTests/fast/events/backspace-goes-back.html:1
> +<script>

Missing DOCTYPE.

> LayoutTests/fast/events/backspace-goes-back.html:11
> +function doStep(step, url) {

Nit: this isn't really url, right?  How about "query" or "location"?

> LayoutTests/fast/events/backspace-goes-back.html:37
> +        }, 100);

100 might be too short.

> LayoutTests/fast/events/backspace-goes-back.html:69
> +        sessionStorage.step = 0;

Should we remove "step" when we call notifyDone so that it doesn't affect other tests executed later?

> LayoutTests/fast/events/backspace-goes-back.html:79
> +    }, 0)

Nit: missing a semi-colon.

> Source/WebCore/editing/EditingBehavior.h:64
> +    bool shouldGoBackOnBackspace() const { return m_type != EditingUnixBehavior; }

"GoBack" might be too general.  How about shouldNavigateBackOnBackspace?

[Antonio Gomes]

Comment on attachment 92150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92150&action=review

> LayoutTests/ChangeLog:9
> +        Add a test that verifies that pressing backspace goes back on Mac,
> +        but does not go back on Linux.

Why are not you testing Windows? and the proper terminology would be 'Unix' imo.

[Antonio Gomes]

(In reply to comment #0)
> Linux Chrome and WebKitGtk (not sure about Qt) intentionally do not have the backspace-goes-back behavior.
> 
> This regressed in http://trac.webkit.org/changeset/83522 .

I do not think it was 100% right to change the behavior for all linux platform ports w/out giving some notice in advance.

[Evan Martin]

(In reply to comment #29)
> Why are not you testing Windows?

Windows and Mac are currently the same code path.
I can add a separate test for Windows.

> and the proper terminology would be 'Unix' imo.

If you really want this, I can do it, but I don't think it improves the quality of the code.

[Evan Martin]

(In reply to comment #30)
> > This regressed in http://trac.webkit.org/changeset/83522 .
> 
> I do not think it was 100% right to change the behavior for all linux platform ports w/out giving some notice in advance.

It changed twice: once in the regression linked above, and now I am changing it back to the way it was before.  Which advance notice did you want?

[Antonio Gomes]

> Windows and Mac are currently the same code path.
> I can add a separate test for Windows.

For all platform specific tests, we try to go through all codepaths possible separately. See other related tests ...

> It changed twice: once in the regression linked above, and now I am changing it back to the way it was before.  Which advance notice did you want?

I would prefer people to asked before changing the behavior. QtWebKit 2.2 was tagged recently and it probably has this "regression". 

You are doing the right thing, indeed. I hope you understand my concerns ...

[Alexey Proskuryakov]

> I would prefer people to asked before changing the behavior. QtWebKit 2.2 was tagged recently and it probably has this "regression". 

The regression was due to a mistake. If it were known, it wouldn't have landed that way.

[Antonio Gomes]

(In reply to comment #33)
> > Windows and Mac are currently the same code path.
> > I can add a separate test for Windows.
> 
> For all platform specific tests, we try to go through all codepaths possible separately. See other related tests ...
> 

platform specific *editing* behavior.

> The regression was due to a mistake. If it were known, it wouldn't have landed that way.

Ap: thanks!

[Evan Martin]

Created attachment 92157 [details]
Patch

[Evan Martin]

Created attachment 92159 [details]
Patch

[Evan Martin]

New patch addresses all the comments I haven't responded to already: in particular, I added a Windows step, and I renamed the test and the function to use "navigate" instead of "go" (that is a good catch, Ryosuke).

[Ryosuke Niwa]

Comment on attachment 92159 [details]
Patch

I'd say r+ if nobody objects the idea of making it dependent on editing behavior.

[Antonio Gomes]

Comment on attachment 92159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92159&action=review

LGTM too. I have one question:

> LayoutTests/fast/events/backspace-navigates-back.html:19
> +    setTimeout(function() {
> +        expect(false, 'navigation ' + msg);
> +    }, 500);

Is not half a second too long?

> LayoutTests/fast/events/backspace-navigates-back.html:75
> +        setTimeout(function() {
> +            document.location = '?test-complete';
> +        }, 500);

Ditto.

[Ryosuke Niwa]

Comment on attachment 92159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92159&action=review

>> LayoutTests/fast/events/backspace-navigates-back.html:19
>> +    }, 500);
> 
> Is not half a second too long?

I don't think so.  It was previously 100ms and I think that's too short.  Multiple tests are ran in parallel (NRWT does this), depending on disk cache and all that jazz, it may take a really long time to fetch a page.

[Alexey Proskuryakov]

You may be able to use pagehide events instead of timeouts. I suggest investigating this possibility before landing.

[Evan Martin]

I really dislike the timeout, and welcome a better approach.

There are two things to test:
1) That backspace does cause us to go back for mac/win.  In this case, I set a timeout that will only fire if the test regresses (fails to go back) so I think the timeout is harmless.

2) That backspace does not go back for linux.  This is hard because I'm trying to assert that navigation *doesn't* happen.
In this case, I set a timeout that navigates to a "success" page after waiting a bit to see whether we've gone back.  This timeout is harmful because it's in the normal path of the test.

We discussed ways to fix this latter case but couldn't come up with anything.  The problem is that going back is async; if there was a way to tell whether a back navigation is *pending* then I could just assert that it isn't happening, but we couldn't figure out a combination of show/hide events helping here.  (In a more ideal world I could somehow check which URL we were heading to in the pagehide/unload handler, but since I think that isn't available the other options will be racy.)

The only idea we came up with is perhaps putting a fragment navigation in the history so that we could be more sure that going back happens "quickly", but I think even then we'd still need a timeout.  Advice welcome.

[Ryosuke Niwa]

(In reply to comment #43)
> There are two things to test:
> 1) That backspace does cause us to go back for mac/win.  In this case, I set a timeout that will only fire if the test regresses (fails to go back) so I think the timeout is harmless.

Can we use pageShow/pageHide events for this one?

> 2) That backspace does not go back for linux.  This is hard because I'm trying to assert that navigation *doesn't* happen.
> In this case, I set a timeout that navigates to a "success" page after waiting a bit to see whether we've gone back.  This timeout is harmful because it's in the normal path of the test.

We can add a new method to layoutTestController but I'm not sure if it's worth the effort.

Is there any useful History API?

[Antonio Gomes]

Comment on attachment 92159 [details]
Patch

setTimeout will work, but it does not get it rid of the possibility of being flaky (even with such a high timeout value). I think we try to avoid it, but if there is not choice...

what about this:

to test mac:
- ltc.clearBackForwardList();
- ltc.setEditinbBehavior(mac);
- ltc.queueLoadingScript('eventSender.keyDown("\u0008")'); // will trigger a back navigation on mac
- ltc.queueLoadingScript('something that checks the location here and dumps if the result is right');
- ltc.queueForwardNavigation(1); // get the test ready for the next test.


to test linux and window:
- ltc.clearBackForwardList();
- ltc.setEditinbBehavior({win,unix});
- ltc.queueLoadingScript('eventSender.keyDown("\u0008")'); // will trigger a back navigation on mac
- ltc.queueLoadingScript('something that checks the location here');

[Darin Adler]

Generally speaking, if you start one navigation and then trigger another, the original is canceled.

So perhaps a test could be made that starts a navigation, then tests whether the delete key cancels that one and does a back navigation instead or has allows that first navigation to complete.

[Evan Martin]

Created attachment 92295 [details]
Patch

[Evan Martin]

Darin wins!

 69        // We expect backspace to *not* navigate.
 70        // Start a navigation to the success page; if backspace causes us to go back,
 71        // it will cancel that navigation and navigate us elsewhere, causing the test
 72        // to fail.
 73        document.location = '?test-complete';
 74        eventSender.keyDown('\u0008');

I tested this test by temporarily changing the editing behavior to mac (causing backspace to go back in this branch) and verified that the test then fails.

[Ryosuke Niwa]

Comment on attachment 92295 [details]
Patch

On my second thought, I'm afraid all these states will affect each other and makes it flaky.  Can we have 3 tests with a shared js file instead?

[Evan Martin]

Can you explain more why you think the states will affect each other?  I am reluctant to make this code even more complicated...

[Ryosuke Niwa]

(In reply to comment #50)
> Can you explain more why you think the states will affect each other?  I am reluctant to make this code even more complicated...

To make tests less likely to be flaky, I'm suggesting that we split this test into 3 tests each of which will test win, mac, and unix behavior separately. That way, the result of each test won't affect each other.

[WebKit Review Bot]

Attachment 92295 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8567004
New failing tests:
fast/events/backspace-navigates-back.html

[WebKit Review Bot]

Created attachment 92374 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-22-virtual-x86_64-with-Ubuntu-10.10-maverick

[Evan Martin]

(The bot failure is because I forgot to rebaseline the expected output after the last round of changes.  The failing output is failing just because the success message has the wrong line number.)

I appreciate that you have a lot more experience with this stuff than me, but I still don't understand why you think these tests need to be independent.  If you really insist I guess I can make this into three separate tests.  I think it makes the test more complicated and clutters the tree with 6 additional files without a good reason.

[Alexey Proskuryakov]

+CONSOLE MESSAGE: line 76: PASS: test complete
+This test passes if it prints "PASS: test complete".

"Prints" is a bit confusing in this context. I know I wouldn't guess to check the Web Inspector console. And using the console for navigation tests is fragile, since navigation clears it. A plain alert() might be better.

> clutters the tree with 6 additional files without a good reason

Having three copies of a number of many editing tests doesn't seem so great, so I hesitate to support this as a default choice. On the other hand, testing all code paths in one test makes it harder to debug and to interpret the results.

[Ryosuke Niwa]

(In reply to comment #54)
> I appreciate that you have a lot more experience with this stuff than me, but I still don't understand why you think these tests need to be independent.  If you really insist I guess I can make this into three separate tests.  I think it makes the test more complicated and clutters the tree with 6 additional files without a good reason.

I think it'll simplify the script (since shared js can implement the test logic and each html file can define expected results) and reduce the chance of being flaky (triggering multiple loads/unloads seems to make tests flakier) but I won't insist.

[Ojan Vafai]

Comment on attachment 92295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92295&action=review

Whether it would be better as separate tests is clearly arguable (e.g. I like it as is). Sounds like Ryosuke, would prefer this test split up, but is not opposed to this staying a single test.

> LayoutTests/fast/events/backspace-navigates-back-expected.txt:2
> +This test passes if it prints "PASS: test complete".

s/prints "PASS: test complete"./logs "PASS: test complete" to the console. Just to be more clear (e.g. printing could be printing it to the page.

> LayoutTests/fast/events/backspace-navigates-back.html:86
> +window.onpageshow = function() {

Can you add something like the following:
if (!window.eventSender) {
    document.body.innerHTML += " This test requires eventSender and layoutTest controller. It cannot be run manually.";
    return;
}

[Evan Martin]

Committed r85893: <http://trac.webkit.org/changeset/85893>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85893 might have broken Qt Linux Release
The following tests are not passing:
fast/events/backspace-navigates-back.html

[Evan Martin]

Filed https://bugs.webkit.org/show_bug.cgi?id=60311 about the Qt failure.  Given that we test all platforms in the test, my weak (as in not very good) guess is DumpRenderTree differences.