Bug 59731

Summary: REGRESSION(r83522): backspace should not go back on Linux
Product: WebKit Reporter: Evan Martin <evan>
Component: UI EventsAssignee: Evan Martin <evan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, benjamin, buildbot, darin, dglazkov, eric, gns, gustavo.noronha, kling, pkasting, rniwa, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ojan: review+
Archive of layout-test-results from ec2-cr-linux-02 none

Description Evan Martin 2011-04-28 12:50:39 PDT
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 .
Comment 1 Evan Martin 2011-04-28 13:55:19 PDT
Created attachment 91549 [details]
Patch
Comment 2 Evan Martin 2011-04-28 13:56:00 PDT
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.
Comment 3 Ryosuke Niwa 2011-04-28 13:59:49 PDT
(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
Comment 4 Darin Adler 2011-04-28 14:00:44 PDT
(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
Comment 5 Evan Martin 2011-04-28 14:08:06 PDT
Comment on attachment 91549 [details]
Patch

clearing r? because I need a test.
Comment 6 Early Warning System Bot 2011-04-28 14:10:33 PDT
Attachment 91549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8521023
Comment 7 WebKit Review Bot 2011-04-28 14:19:16 PDT
Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8514761
Comment 8 Alexey Proskuryakov 2011-04-28 14:21:40 PDT
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.
Comment 9 Evan Martin 2011-04-28 14:34:00 PDT
(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.
Comment 10 Ryosuke Niwa 2011-04-28 14:42:31 PDT
(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.
Comment 11 Build Bot 2011-04-28 14:51:05 PDT
Attachment 91549 [details] did not build on win:
Build output: http://queues.webkit.org/results/8511895
Comment 12 WebKit Review Bot 2011-04-28 15:22:09 PDT
Attachment 91549 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8516624
Comment 13 WebKit Review Bot 2011-04-28 15:22:43 PDT
Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8521044
Comment 14 Darin Adler 2011-04-28 16:16:20 PDT
To me this seems like a port/browser preference, not a platform editing style quirk.
Comment 15 Darin Adler 2011-04-28 16:17:25 PDT
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.
Comment 16 Alexey Proskuryakov 2011-04-28 16:24:01 PDT
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.
Comment 17 WebKit Review Bot 2011-04-28 16:25:49 PDT
Attachment 91549 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8516634
Comment 18 Evan Martin 2011-04-28 16:40:46 PDT
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.
Comment 19 Ryosuke Niwa 2011-04-28 17:17:59 PDT
(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.
Comment 20 Antonio Gomes 2011-04-28 19:13:28 PDT
(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.
Comment 21 Ryosuke Niwa 2011-04-28 19:54:53 PDT
(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.
Comment 22 Collabora GTK+ EWS bot 2011-04-28 21:50:26 PDT
Attachment 91549 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8516740
Comment 23 Peter Kasting 2011-04-29 09:39:44 PDT
(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.
Comment 24 Ryosuke Niwa 2011-04-29 10:34:57 PDT
(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...
Comment 25 Peter Kasting 2011-05-02 10:24:50 PDT
(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.
Comment 26 Evan Martin 2011-05-03 15:40:23 PDT
Created attachment 92150 [details]
Patch
Comment 27 Evan Martin 2011-05-03 15:42:00 PDT
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.
Comment 28 Ryosuke Niwa 2011-05-03 15:52:03 PDT
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?
Comment 29 Antonio Gomes 2011-05-03 16:01:18 PDT
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.
Comment 30 Antonio Gomes 2011-05-03 16:03:14 PDT
(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.
Comment 31 Evan Martin 2011-05-03 16:04:45 PDT
(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.
Comment 32 Evan Martin 2011-05-03 16:05:57 PDT
(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?
Comment 33 Antonio Gomes 2011-05-03 16:14:11 PDT
> 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 ...
Comment 34 Alexey Proskuryakov 2011-05-03 16:16:16 PDT
> 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.
Comment 35 Antonio Gomes 2011-05-03 16:17:31 PDT
(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!
Comment 36 Evan Martin 2011-05-03 16:21:50 PDT
Created attachment 92157 [details]
Patch
Comment 37 Evan Martin 2011-05-03 16:22:27 PDT
Created attachment 92159 [details]
Patch
Comment 38 Evan Martin 2011-05-03 16:23:33 PDT
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).
Comment 39 Ryosuke Niwa 2011-05-03 16:29:39 PDT
Comment on attachment 92159 [details]
Patch

I'd say r+ if nobody objects the idea of making it dependent on editing behavior.
Comment 40 Antonio Gomes 2011-05-03 17:20:33 PDT
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.
Comment 41 Ryosuke Niwa 2011-05-03 17:24:24 PDT
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.
Comment 42 Alexey Proskuryakov 2011-05-03 17:35:36 PDT
You may be able to use pagehide events instead of timeouts. I suggest investigating this possibility before landing.
Comment 43 Evan Martin 2011-05-04 10:58:42 PDT
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.
Comment 44 Ryosuke Niwa 2011-05-04 11:14:18 PDT
(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?
Comment 45 Antonio Gomes 2011-05-04 11:28:51 PDT
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');
Comment 46 Darin Adler 2011-05-04 11:51:15 PDT
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.
Comment 47 Evan Martin 2011-05-04 12:10:35 PDT
Created attachment 92295 [details]
Patch
Comment 48 Evan Martin 2011-05-04 12:10:48 PDT
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.
Comment 49 Ryosuke Niwa 2011-05-04 12:16:06 PDT
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?
Comment 50 Evan Martin 2011-05-04 13:54:58 PDT
Can you explain more why you think the states will affect each other?  I am reluctant to make this code even more complicated...
Comment 51 Ryosuke Niwa 2011-05-04 13:57:33 PDT
(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.
Comment 52 WebKit Review Bot 2011-05-04 22:37:05 PDT
Attachment 92295 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8567004
New failing tests:
fast/events/backspace-navigates-back.html
Comment 53 WebKit Review Bot 2011-05-04 22:37:15 PDT
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
Comment 54 Evan Martin 2011-05-05 10:27:47 PDT
(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.
Comment 55 Alexey Proskuryakov 2011-05-05 10:44:40 PDT
+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.
Comment 56 Ryosuke Niwa 2011-05-05 10:51:05 PDT
(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.
Comment 57 Ojan Vafai 2011-05-05 11:35:50 PDT
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;
}
Comment 58 Evan Martin 2011-05-05 15:23:31 PDT
Committed r85893: <http://trac.webkit.org/changeset/85893>
Comment 59 WebKit Review Bot 2011-05-05 16:19:11 PDT
http://trac.webkit.org/changeset/85893 might have broken Qt Linux Release
The following tests are not passing:
fast/events/backspace-navigates-back.html
Comment 60 Evan Martin 2011-05-05 16:21:28 PDT
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.