Bug 153747 – REGRESSION(r195949): [GTK] Test /webkit2/WebKitWebView/insert/link is failing since r195949

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

/webkit2/WebKitWebView/insert/link:                                  **

ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:445:void testWebViewEditorCreateLink(EditorTest*, gconstpointer): assertion failed: (javascriptResult)

FAIL

I haven't had time to investigate this yet, just noticed it in the bots.

[Carlos Garcia Campos]

Ok, what r195949 broke is MoveToEndOfLine and MoveToBeginningOfLine editing command when there's only one word and it's selected. It can be reproduced using any input field:

1. Write a single world
2. Select it entirely from left to right to ensure the cursor is at the end of the word. Double clicking should also work.
3. Press End key

The selection should be cleared, but it's not. Same happens if you select the word from right to left and press the Start key. So, my guess is that when the cursor is already at the desired position, it does nothing. I'm not sure if this happens in mac too, or depends on the editor behavior, so I'm leaving this as GTK specific for now, could someone confirm?

[Darin Adler]

On Mac it is Control-Right-Arrow, not End. The “End key moves cursor to the end of line” behavior comes from Windows; on Mac the End key scrolls to the end of the document and does not move the cursor. Also, this particular keyboard command is overridden by Mission Control by default in recent versions of Mac, so you have to turn off Mission Control’s "Move right a space" key binding in System Preferences Keyboard Shortcuts to test it.

I tried to reproduce this with the local copy of Safari with the latest WebKit that I built and I could not reproduce the bug on Mac; it seemed to work fine. Might have done the testing wrong.

[Carlos Garcia Campos]

Created attachment 270981 [details]
Test

Thanks for looking at this, Darin. I've written a test that fails with current trunk and passes with r195949 reverted, at least for the GTK+ port. It should be copied to LayoutTests/editing/selection.

[Adrien Plazas]

Created attachment 271455 [details]
Makes the test pass

DISCLAIMER: This is very probably not the right thing to do, it makes the test pass though.

[Adrien Plazas]

The problem seems to be caused by FrameSelection::modify() returning too early (in the added section) and hence not performing the switch (alter) {...} bloc.

[Adrien Plazas]

I'm not sure how to fix this properly though, any help is welcome.

[Doug Russell]

In the event that shouldNotify is true and the early return would kick in, I'd expect the position to be isNull() == true and the code path would have bailed early on the follow isNull() check, but if you have a case where that's not true, we should be able to adjust accordingly. Probably by moving the if (shouldNotify...) farther down when position isn't null.

[Michael Catanzaro]

Comment on attachment 271455 [details]
Makes the test pass

I think this requires further investigation.

[Carlos Garcia Campos]

The thing is why we are returning early in case of notifying the AT about the selection change. I guess we are assuming that reaching a boundary means that there won't be a selection change. Also, why aren't we also notifying when reaching a boundary in case of character granularity?

[Carlos Garcia Campos]

Created attachment 271916 [details]
Test case as a patch

Just to check Mac behavior in EWS.

[Carlos Garcia Campos]

Ok, so it seems to pass in Mac, I don't understand why, though.

[Carlos Garcia Campos]

Created attachment 271920 [details]
Patch

This patch fixes the problem for GTK+, and hopefully doesn't break Mac.

[Doug Russell]

This change looks sensible and as long it doesn't break the mac tests I'm on board.

[Michael Catanzaro]

Comment on attachment 271920 [details]
Patch

OK then, I was waiting for Doug to comment. :)

Thanks for the investigation, Adrian!

[Carlos Garcia Campos]

Committed r197024: <http://trac.webkit.org/changeset/197024>