Bug 25533

Created attachment 29970 [details] aforementioned test case

I'm curious to know how this works in the Mac port... I mean, does it work like firefox (which I understand it's the expected behavior as well for GTK) or is it just ok in Mac that down/up arrowing moves to the next/previous cell in the same row for a table? Asking this because, as far as I can see through my early tests, to make this work as it's described for the GTK port would require changes in deep WebCore code, and probably platform dependant. (Adding Chris Fleizach to CC)

(In reply to comment #2) > I'm curious to know how this works in the Mac port... I mean, does it work like firefox (which I > understand it's the expected behavior as well for GTK) or is it just ok in Mac that down/up > arrowing moves to the next/previous cell in the same row for a table? > > Asking this because, as far as I can see through my early tests, to make this work as it's > described for the GTK port would require changes in deep WebCore code, and probably > platform dependant. I plan to work (and hopefully cook a patch) on this bug during this week, at the WebKitGTK hackfest, so any input regarding to my questions above would be really helpful and appreciated O:-) I would even consider it as a nice Christmas present!

Created attachment 76044 [details] Patch proposal + New layout test I'm attaching a patch to fix this bug (along with a new layout test) I've written this week during the WebKitGTK hackfest, making the following assumptions: - It will affect the GTK port only. Other ports will keep working as they used to do and the new functionality introduced is just irrelevant for them. - Following Martin Robinson's suggestion to avoid introducing and JS API breakage with the "MoveDown" and "MoveUp" editing commands, I've created new commands for moving up and down (with or without extending a selection) *specifically designed to be used when in caret browsing mode only* which, so far should only affect the GTK port, as the rest of the ports would keep using the "MoveDown"/"MoveUp" commands. - In the layout test I'm not only checking the behaviour of this patch when in caret browsing mode but also that the "MoveDown" and "MoveUp" commands (in GTK used only when NOT in caret mode) keep being the same ones than they used to be, to confirm no JS API breakage happened here. I understand this could be not an easy patch to get accepted, as it introduced four new editing commands that are only needed by the GTK port, but after thinking for a long time and getting a lot of support from Martin, we thought that perhaps this could be the best option to avoid risking the JS API, whenever the patch also provided a good enough layout test for it. Eager to get some feedback. Thanks!

Adding Kent Tamura and Darin Adler to CC (as per svn blame output)

Removing [GTK] prefix from bug's title, as its resolution happens mostly in WebCore text editing code

Created attachment 77809 [details] Patch proposal + New layout test Attaching new version of the patch as the previous one is now obsolete, due to latest changes in EditorClientGtk and SelectionController (EDirection type removed)

Ping?

Mario, when I try this fix with certain pages, caret navigation seems to skip lines that it didn't used to skip. For instance, try navigating from top to bottom on: http://www.ubuntu.com/project/about-ubuntu

Created attachment 80187 [details] Additional test case Mario, when re-testing your change for this bug with the test case I mentioned in #a11y, I discovered that your patch impacts it. Therefore maybe this (and my previous comment about skipping lines on Ubuntu) is the same issue after all. To use this test case, enable caret navigation. Then Tab/Shift+Tab into focus is on the 'Start here' link at the top of the page. Start Down Arrowing. Without your patch, I get stuck on every instance of the 'bla' link. I cannot Down Arrow off of it. If I Right Arrow once to move off of the link and then Down Arrow, I move to the end of the line. With your patch, I only get stuck on the first instance of the 'bla' link. I still cannot Down Arrow off of it. But if I Right Arrow once to move off of the link and then Down Arrow, I jump to the very end of the document.

Damm it... sounds like this patch is not correct then, after all :-( Clearing r? flags for now... will come back to it asap (but work is piling up quickly so can't tell at the moment)

Comment on attachment 77809 [details] Patch proposal + New layout test This is not valid, as per last Joanmarie's comment

Created attachment 84236 [details] Patch proposal + Layout test Attaching a new patch that doesn't present the problems stated by Joanie, although we still need to fix another bug I found while working on this one that is causing trouble with other issues related to vertical movement of the caret (see below)... (In reply to comment #10) > [...] > Mario, when re-testing your change for this bug with the test case I > mentioned in #a11y, I discovered that your patch impacts it. Therefore > maybe this (and my previous comment about skipping lines on Ubuntu) is > the same issue after all. Yep, it certainly was :-) > To use this test case, enable caret navigation. Then Tab/Shift+Tab into > focus is on the 'Start here' link at the top of the page. Start Down > Arrowing. > > Without your patch, I get stuck on every instance of the 'bla' link. > I cannot Down Arrow off of it. If I Right Arrow once to move off of the > link and then Down Arrow, I move to the end of the line. > > With your patch, I only get stuck on the first instance of the 'bla' > link. I still cannot Down Arrow off of it. But if I Right Arrow once > to move off of the link and then Down Arrow, I jump to the very end > of the document. You were right, I found there was a "different wrong behaviour" with my patch that was not happening without it (still wrong, though), but I found it to be a different bug I'll be reporting in some minutes, right after posting this comment. Also (and that's why I mention this here), I'll be making that new bug blocking this one, since it definitely impacts vertical caret navigation as well.

Comment on attachment 84236 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=84236&action=review > Source/WebCore/ChangeLog:28 > + (WebCore::createCommandMap): Add the new commands needed for the > + GTK port, related with up/down movement in caret browsing mode. I don't think it's correct to fix just GTK port. This is a bug that needs to be fixed on all ports regardless of whether the caret navigation is enabled or not. To demonstrate the bug on other ports, simply enable the design mode on the attached test case. > Source/WebCore/editing/EditorCommand.cpp:620 > +static bool isVerticallyCoincident(VisiblePosition origin, VisiblePosition candidate, int originX, SelectionDirection direction) > +{ This is definitely not right. We shouldn't be adding these selection related code in EditorCommand.cpp, which is supposed to be delegating all the work to the rest of editing code.

(In reply to comment #14) > (From update of attachment 84236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84236&action=review > > > Source/WebCore/ChangeLog:28 > > + (WebCore::createCommandMap): Add the new commands needed for the > > + GTK port, related with up/down movement in caret browsing mode. > > I don't think it's correct to fix just GTK port. Actually the code I've introduced is common to all the platforms, it's just that the GTK port is the only one that would use it at the moment, since it's the only one, afaik, with caret browsing mode. I created those 4 new commands precisely because modifying the behavior of the already present MoveUp/MoveDown commands would have an impact in the current API, and that seemed to me like a bad idea. > This is a bug that needs to be fixed on all ports regardless of whether the > caret navigation is enabled or not. I checked what Chrome and Safari does when moving across a table (not in caret browsing mode, but selecting a single character and then extending the selection with shift+arrows) and I found they traverse tables in the same (wrong?) way: cell by cell in a row by row basis. Obviously, my current patch wouldn't fix the behavior for those other ports since I'm defining four new commands precisely not to touch current up/down behavior, but your comment makes me think perhaps I should actually be modifying those current commands, instead of creating new ones. Could you please clarify? > To demonstrate the bug on other ports, simply enable the design mode on the > attached test case. What do you mean by "enable the design mode"? > > Source/WebCore/editing/EditorCommand.cpp:620 > > +static bool isVerticallyCoincident(VisiblePosition origin, VisiblePosition candidate, int originX, SelectionDirection direction) > > +{ > > This is definitely not right. We shouldn't be adding these selection related > code in EditorCommand.cpp, which is supposed to be delegating all the work > to the rest of editing code. Could you provide some guidance, even roughly, about how this could be done then? I'm not fond on this so deep issues in editing stuff and probably I'm missing something :-) Thanks for the great feedback

(In reply to comment #15) > > This is a bug that needs to be fixed on all ports regardless of whether the > > caret navigation is enabled or not. > > I checked what Chrome and Safari does when moving across a table (not in caret browsing mode, but selecting a single character and then extending the selection with shift+arrows) and I found they traverse tables in the same (wrong?) way: cell by cell in a row by row basis. > > Obviously, my current patch wouldn't fix the behavior for those other ports since I'm defining four new commands precisely not to touch current up/down behavior, but your comment makes me think perhaps I should actually be modifying those current commands, instead of creating new ones. Yes. You should be changing the behavior of up/down on all platforms because the current behavior is inconsistent with TextEdit, Firefox, etc... > > To demonstrate the bug on other ports, simply enable the design mode on the > > attached test case. > > What do you mean by "enable the design mode"? document.designMode = 'on'; > > > Source/WebCore/editing/EditorCommand.cpp:620 > > > +static bool isVerticallyCoincident(VisiblePosition origin, VisiblePosition candidate, int originX, SelectionDirection direction) > > > +{ > > > > This is definitely not right. We shouldn't be adding these selection related > > code in EditorCommand.cpp, which is supposed to be delegating all the work > > to the rest of editing code. > > Could you provide some guidance, even roughly, about how this could be done then? I'm not fond on this so deep issues in editing stuff and probably I'm missing something :-) Given my comment above, I'd guess that you need to modify SelectionController.cpp and visible_units.cpp but not EditorCommand.cpp.

(In reply to comment #16) > [...] > > Obviously, my current patch wouldn't fix the behavior for those other ports since I'm defining four new commands precisely not to touch current up/down behavior, but your comment makes me think perhaps I should actually be modifying those current commands, instead of creating new ones. > > Yes. You should be changing the behavior of up/down on all platforms because the current behavior is inconsistent with TextEdit, Firefox, etc... Gotcha. So no more MoveCaretUp/Down command, let's fix MoveUp/Down instead. > > > To demonstrate the bug on other ports, simply enable the design mode on the > > > attached test case. > > > > What do you mean by "enable the design mode"? > > document.designMode = 'on'; Very useful tip indeed. Thanks! I was using the select-1-char + extend with shift/arrow trick to workaround that in chrome and safari. Much better this way! > > > > Source/WebCore/editing/EditorCommand.cpp:620 > > > > +static bool isVerticallyCoincident(VisiblePosition origin, VisiblePosition candidate, int originX, SelectionDirection direction) > > > > +{ > > > > > > This is definitely not right. We shouldn't be adding these selection related > > > code in EditorCommand.cpp, which is supposed to be delegating all the work > > > to the rest of editing code. > > > > Could you provide some guidance, even roughly, about how this could be done then? I'm not fond on this so deep issues in editing stuff and probably I'm missing something :-) > > Given my comment above, I'd guess that you need to modify SelectionController.cpp and visible_units.cpp but not EditorCommand.cpp. Now I understood why I shouldn't be creating new commands, this makes a lot of sense to me as well. Still fix for bug 55481 keeps being a dependency for this so I'll base my work on that one, hoping it will be accepted at some point :-) Thanks twice!

Created attachment 84953 [details] WIP: (incomplete) patch for fixing the bug I've been working lately on this bug but couldn't find a reliable patch for it yet, that is, I have a patch that seems to improve the situation but still makes some other tests to fail, which means is not correct yet and need further work. However, due to other constraints currently out of my control, I need to temporarily stop working on this bug and move on to another task, so I'm attaching the patch with the current status of the patch I have, hoping I can resume work on this stuff soon at some point in the next months, or simply for the sake of sharing it in case someone else wished to step up finishing it during the meantime. Hence, not asking for review over the attached patch. (In reply to comment #17) [...] > > Yes. You should be changing the behavior of up/down on all platforms because the current behavior is inconsistent with TextEdit, Firefox, etc... > > Gotcha. So no more MoveCaretUp/Down command, let's fix MoveUp/Down instead. > Did it that way. Now I just need to fix those failing tests :-) [...] > > > Could you provide some guidance, even roughly, about how this could be done then? I'm not fond on this so deep issues in editing stuff and probably I'm missing something :-) > > > > Given my comment above, I'd guess that you need to modify SelectionController.cpp and visible_units.cpp but not EditorCommand.cpp. > > Now I understood why I shouldn't be creating new commands, this makes a lot of sense to me as well. > > Still fix for bug 55481 keeps being a dependency for this so I'll base my work on that one, hoping it will be accepted at some point :-) As I said here, I used patch for 55481 as a dependency for this one, as it's crucial not to get stuck on objects with non-zero padding/border when navigating vertically.