Bug 36627

Just from sniffing around a bit, it seems that Qt and GTK+ have different editing behaviors, so getting things to work in a Qt-ish way and a GTK+-ish may require more granularity than just "Linux", "Mac" and "Windows."

(In reply to comment #1) > Just from sniffing around a bit, it seems that Qt and GTK+ have different > editing behaviors, so getting things to work in a Qt-ish way and a GTK+-ish may > require more granularity than just "Linux", "Mac" and "Windows." Can you give an example of a difference between Qt and GTK+ editing behaviors?

On further investigation it seems the difference in behavior that I've seen so far is mostly due to: https://bugzilla.gnome.org/show_bug.cgi?id=605466 . GTK+ (except GTKTextView), Qt a. double-click a word b. modify selection via shift+right arrow ---> selection becomes one character longer GTK+ (GTKTextView) a. double-click a word b. modify selection via shift+right arrow ---> selection becomes one character shorter For the sake of simplicity, I think it makes sense for both Qt and GTK+ to follow the same behavior here. Perhaps this isn't as complicated as I thought.

Wow, yeah, I think the GtkTextView behavior has to be a bug! I had to try it myself to believe it.

I didn't realize double-click selection in GtkTextView is the reverse of GtkEntry. I think it should definitely be consistent. I will try to find some GTK+ core hackers to ask about making this conistent I would for now suggest to implement GtkEntry/ Qt behaviour in WebKit "Linux". Maybe a better name would be "Freedesktop" since that is the common denominator shared by KDE, GNOME, XFCE and LXDE?

I am working in some initial steps on this.

Once we have a third behavior or fourth, I think we need some functions that translate the overall behavior policy into specific behaviors rather than directly saying "behavior == Mac" or "behavior == Windows". Helper functions that expression the different rules in plain language. I think that should be straightforward and we can do that refactoring before adding a new behavior value.

(In reply to comment #7) > Once we have a third behavior or fourth, I think we need some functions that translate the overall behavior policy into specific behaviors rather than directly saying "behavior == Mac" or "behavior == Windows". Helper functions that expression the different rules in plain language. > > I think that should be straightforward and we can do that refactoring before adding a new behavior value. Do you mean something like isMacEditingBehavior(), isWindowsEditingBehaviour(), etc?

(In reply to comment #8) > Do you mean something like isMacEditingBehavior(), isWindowsEditingBehaviour(), etc? No, things like: shouldDoubleClickSelectSpaceAfterWord Although maybe not specifically that one; I’d have to look at the differences. Generally speaking I’d like the editing behavior policies clearly stated in some central place and this is one good way to clearly state them.

> > Do you mean something like isMacEditingBehavior(), isWindowsEditingBehaviour(), etc? > > No, things like: > > shouldDoubleClickSelectSpaceAfterWord > > Although maybe not specifically that one; I’d have to look at the differences. Generally speaking I’d like the editing behavior policies clearly stated in some central place and this is one good way to clearly state them. Hum, I see you point now. I am worried if we could end up with too many methods like this, e.g.: - shouldSelectWordUnderContextMenu <- right click on top of text; - shouldExtendSelectionWorkAsOnWindows, shouldExtendSelectionWorkAsOnLinux, etc; - shouldClickingBelowTheLastLineOfTextboxGoToEnd - etc. (sorry for the bad fake names) My point is if different editing behavior are generally plaform specific ones, should we really have all those methods?

(In reply to comment #10) > My point is if different editing behavior are generally plaform specific ones, should we really have all those methods? I think we should. They can be inline functions. The best way to document things like this is with code. The actual compiled code will be identical to what otherwise would be spread at the different call sites. By putting these functions in a single header we can make it clear what these different editing "behaviors" mean.

(In reply to comment #11) > (In reply to comment #10) > > My point is if different editing behavior are generally plaform specific ones, should we really have all those methods? > > I think we should. They can be inline functions. The best way to document things like this is with code. The actual compiled code will be identical to what otherwise would be spread at the different call sites. > > By putting these functions in a single header we can make it clear what these different editing "behaviors" mean. I think this is a great idea. It will make the code much more readable. Also, it will make it very easy for people new to the editing code to figure out what the behaviors of each platform are. Finally, if we ever were to add a new platform, there'd only be a single file we'd need to modify.

Ok. We can go that direction. As a start, grep'ed WebCore/ looking for the current call sites of Settings::editingBehavior(), and tried to understand each situation. Summary follows: - RenderBlock::positionForPointWithInlineChildren(...) It handlers where to put the caret (by returning a VisiblePosition) if clicking under the last line of a editable area (e.g. <textarea>). Current difference: Windows moves caret to the last line, but obeying the X position of the click. On Linux and Mac, caret goes to the end of the last line. - static bool executeToggleStyle(...), at EditorCommand.cpp Method toggles on/off a given style (for example, italic or bold) of a selected text in a editable area. On Mac, it considers the selection as already styled id the start of the selection is styled. On non EditingMacBeviour case, current selection is only considered as already styled if the whole selection is styled. Based on that, the method toggles on or off the style. - EventHandler::handleMousePressEventSingleClick(...) It handles how a selection is extended by a click when SHIFT is being held. On Mac, it calculates if the clicked position is closer to the start or to the end of the current selected text and moves selection towards the closer. Windows and Linux work rather differently. __________________ I've also seen some related stuff that are likely platform specific editing behavior as well but are controlled by Settings::editingBehavior, including: - right clicking on a text opens the context sensitive menu and selects the word under the mouse point on anything but Gtk and Chromium. It is defined in EventHandler::sendContextMenuEvent (see GTK and CHROMIUM guards there). - "Pushing the down arrow key on the last line puts the caret at the end of the last line on Mac and Linux, but does nothing on Windows. A similar case exists on the top line." as described in Settings.h, landed by Darin. @darin, ojan: should all these cases become the "helper functions that expression the different rules in plain language" you are agreeing with?

(In reply to comment #13) correcting a few typos: > On Mac, it considers the selection as already styled id the start of the s/id/if > __________________ > > I've also seen some related stuff that are likely platform specific editing behavior as well but are controlled by Settings::editingBehavior, including: s/are controlled/are NOT controlled/

We should probably put this in a new file. EditingBehavior.h? (In reply to comment #13) > - RenderBlock::positionForPointWithInlineChildren(...) > > It handlers where to put the caret (by returning a VisiblePosition) if clicking under the last line of a editable area (e.g. <textarea>). Current difference: > Windows moves caret to the last line, but obeying the X position of the click. On Linux and Mac, caret goes to the end of the last line. Almost correct. It's also for clicking above a line. On Mac/Linux it should move to the beginning of the line. ON Windows, it obeys the X position. Also, it's not just for clicking, it also applies to keyboard movement (i.e. hitting up in the middle of the top line or down in the middle of the bottom line). > - static bool executeToggleStyle(...), at EditorCommand.cpp > > Method toggles on/off a given style (for example, italic or bold) of a selected text in a editable area. > > On Mac, it considers the selection as already styled id the start of the selection is styled. On non EditingMacBeviour case, current selection is only considered as already styled if the whole selection is styled. Based on that, the method toggles on or off the style. > > - EventHandler::handleMousePressEventSingleClick(...) > > It handles how a selection is extended by a click when SHIFT is being held. On Mac, it calculates if the clicked position is closer to the start or to the end of the current selected text and moves selection towards the closer. Windows and Linux work rather differently. Specifically, Win/Linux always move the extent end of the selection. > I've also seen some related stuff that are likely platform specific editing behavior as well but are controlled by Settings::editingBehavior, including: > > - right clicking on a text opens the context sensitive menu and selects the word under the mouse point on anything but Gtk and Chromium. It is defined in EventHandler::sendContextMenuEvent (see GTK and CHROMIUM guards there). This one should be using editingBehavior, but it's not as straightforward as the other ones. Currently, all Chromium is avoided. Really, this behavior should only happen for Mac. I see the word get selected in Chromium-mac though, so it must be doing so down a different code path. > - "Pushing the down arrow key on the last line puts the caret at the end of the last line on Mac and Linux, but does nothing on Windows. A similar case exists on the top line." as described in Settings.h, landed by Darin. Isn't this the usage in RenderBlock::positionForPointWithInlineChildren? > @darin, ojan: should all these cases become the "helper functions that expression the different rules in plain language" you are agreeing with? We should replace any usages of editingBehavior outside of EditingSettings.h. In fact, I think we should remove the getter entirely if possible. There's a couple cases you missed as well. http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.cpp#L167 Whether the revealing the selection top-aligns or center-aligns the selection. http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.cpp#L247 Whether mouse-based selections are directional http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.cpp#L302 Whether keyboard-based selections move the extent for some horizontal navigations. I would guess that there are other cases where we want to change to use this style (e.g. selecting the space after a word on double-click, whether to smartInsertDelete, etc.). But, I suggest leaving those to later. So you can: 1. Move all uses of editingBehavior to EditingBehavior.h. 2. Add EditingLinuxBehavior 3. Move the remaining things that should be in EditingBehavior

That all sounds pretty good. The trick here is to carefully chose the names for the functions so they are as clear as possible.

Great input Darin and Ojan. I am on it ... will probably will need help with namings when I come up with the first version of the patch. Also this change has morphed from the original goal of this but. We are now on a preparation change. Should we file another bug and block this one?

(In reply to comment #17) > Also this change has morphed from the original goal of this but. We are now on a preparation change. Should we file another bug and block this one? Yes.

(In reply to comment #18) > (In reply to comment #17) > > Also this change has morphed from the original goal of this but. We are now on a preparation change. Should we file another bug and block this one? > Yes. s/but/bug Filed bug 39854 (by clone of this one)

Back working on this.

Created attachment 71715 [details] Patch v0.1 - WIP - (real patch coming...)

Not to bikeshed too quickly, but any chance of using something more generic like "XDG" or "FDO" instead of "Linux"? I'm not sure what the most appropriate term is here.

EditingUnixBehavior? I know that technically, mac is unix too, but I don't expect that to be as confusing as terms that many people (e.g. me) wouldn't know. For example I don't know what XDG and FDO are. Otherwise, patch looks great. Happy to see this get in!

martin, also pointed out 'EditingFreetypeBehavior' on IRC a while ago, iirc. We have to make sure all affected ports were running freetype. (In reply to comment #23) > EditingUnixBehavior? I know that technically, mac is unix too, but I don't expect that to be as confusing as terms that many people (e.g. me) wouldn't know. For example I don't know what XDG and FDO are. Mac is not listed as a unix-based system (below), so I would not worry much about it. 434 /* OS(UNIX) - Any Unix-like system */ 435 #if OS(AIX) \ 436 || OS(ANDROID) \ 437 || OS(DARWIN) \ 438 || OS(FREEBSD) \ 439 || OS(HAIKU) \ 440 || OS(LINUX) \ 441 || OS(NETBSD) \ 442 || OS(OPENBSD) \ 443 || OS(QNX) \ 444 || OS(SOLARIS) \ 445 || OS(SYMBIAN) \ 446 || defined(unix) \ 447 || defined(__unix) \ 448 || defined(__unix__) 449 #define WTF_OS_UNIX 1

ah, it is: DARWIN > Mac is not listed as a unix-based system (below), so I would not worry much about it. > > 434 /* OS(UNIX) - Any Unix-like system */ > 435 #if OS(AIX) \ > 436 || OS(ANDROID) \ > 437 || OS(DARWIN) \ > 438 || OS(FREEBSD) \

(In reply to comment #23) > EditingUnixBehavior? I know that technically, mac is unix too, but I don't expect that to be as confusing as terms that many people (e.g. me) wouldn't know. For example I don't know what XDG and FDO are. quick reference: * XGD: http://www.freedesktop.org/wiki/Software/xdg-user-dirs * FDO = freedesktop(?)

(In reply to comment #23) > EditingUnixBehavior? I know that technically, mac is unix too, but I don't expect that to be as confusing as terms that many people (e.g. me) wouldn't know. For example I don't know what XDG and FDO are. Fair enough. There's always EditingFreedesktopOrgBehavior, EditingFreedesktopBehavior, or EditingXWindowsBehavior, I reckon.

Definitely not freetype! That has nothing to do with user interface.

err, s/freetype/freedesktop. Not sure where freetype came from =/ sorry about the confusion. I will come up with a patch later today, but first I have some questions about two things I do not know if should come together in this patch: 1) the WIP patch I uploaded (attachment 71715 [details]) not only adds a "linux" behavior, but also changes the default behavior in Settings.cpp: +static EditingBehaviorType editingBehaviorTypeForPlatform() +{ + return +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && OS(DARWIN)) || (PLATFORM(QT) && defined(Q_WS_MAC)) + // (PLATFORM(MAC) is always false in Chromium and Qt, hence the extra condition. + EditingMacBehavior +#elif OS(WINDOWS) || (PLATFORM(QT) && defined(Q_WS_WIN)) + EditingWindowsBehavior +#elif OS(UNIX) + EditingLinuxBehavior +#else + // Fallback + EditingMacBehavior +#endif + ; +} Basically - WebKitGtk that currently uses Mac editing behavior would be switching to "linux"; - QtWebKit that uses Windows editing behavior unconditionally would be switching to {mac, win,"linux"}, depending on the platform it is build on; - Ditto for Chromium. So there are big changes happening here ... Please let me know the best way you think to proceed. Should it be a big patch changing it all, or we go incrementally? 2) Also I have to add LayoutTestController::setEditingMode("linux") to all editing tests using this API today.

(In reply to comment #29) > - WebKitGtk that currently uses Mac editing behavior would be switching to "linux"; At least for WebKitGTK+, this is highly desirable. When building the GTK+ port on Windows or Mac, we'd probably want to align with GTK+ on those platforms as well.

(In reply to comment #29) > - Ditto for Chromium. This change seems fine for Chromium. We're just adding Linux since it was already using mac behavior on OS(DARWIN). Can we simplify the #if's to just be #if OS(DARWIN) EditingMacBehavior #elif OS(WINDOWS) EditingWindowsBehavior #elif OS(UNIX) EditingLinuxBehavior #else // Fallback EditingMacBehavior #endif ? I'm not sure if this would work for QT or not.

(In reply to comment #29) > Please let me know the best way you think to proceed. Should it be a big patch changing it all, or we go incrementally? Incrementally will almost certainly be easier to avoid needing to rollback. There will be lots of tests affected by this. It'll likely be easier to make sense of everything if you do one platform at a time. Either way is fine with me though as long as you get the tree green. :)

Created attachment 72294 [details] (committed r70975, r=ojan) patch v1 Added EditingUnixBehavior. Patch also: * updates all corresponding port specific APIs, including Mac, Win, Gtk and Chromium. * update Qt,Gtk,Mac,Win and Chromium LayoutTestController::setEditingBehavior methods to support the newly added editing behavior type. Patch updating all tests that go through platform specific code paths will be in a follow up, so we keep this one clean.

Attachment 72294 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/win/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebKit/gtk/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebCore/page/Settings.cpp:85: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebKit/mac/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #34) > Attachment 72294 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > WebKit/win/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] > WebKit/gtk/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] > WebCore/page/Settings.cpp:85: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] > WebKit/mac/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] > Total errors found: 4 in 21 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Fixed locally.

Comment on attachment 72294 [details] (committed r70975, r=ojan) patch v1 Clearing review flags on attachment: 72294 Committed r70975: <http://trac.webkit.org/changeset/70975>