Bug 88999

Created attachment 147317 [details] Patch

Comment on attachment 147317 [details] Patch Thanks olilan! Let's give rniwa a chance to comment before landing.

Comment on attachment 147317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147317&action=review I don't think that this is much of an improvement. The changed part of a composition is not necessarily at the end. This bug never says what end user scenario it attempts to fix. From what I can reconstruct by looking at the regression test, this already works fine in Safari. so, the right fix would be to find out what's the difference with your port. > Source/WebCore/editing/Editor.cpp:2310 > + revealSelectionAfterEditingOperation(ScrollAlignment::alignToEdgeIfNeeded, true); Please don't add new boolean arguments - they make call sites like this nearly unreadable. Preferred WebKit style is to either add a new function with an appropriate name, or to add a enum with named value for such arguments.

The scenario is that when using a composition-based IME, if the composition being typed is longer than the input field, the field will not scroll to show the end of the composition. So when typing a long word, the user is left looking at just the beginning of the word. I have been able to reproduce this in Safari, using a very small input field and a composition-based IME such as Cangjie. Once the typed characters become longer than the field, further characters added are not visible. You're right that the start of the composition can change also, but as the editing point is effectively at the end of the composition I think it makes sense to reveal that rather than the start or middle. Where setComposition is being called by a composition-based IME during typing (the most common use of the function, I'd have thought), characters will be being added to the end of the composition. The reason I added the bool parameter is that Editor::revealSelectionAfterEditingOperation just calls FrameSelection::revealSelection, which already has parameters of (const ScrollAlignment& alignment, bool revealExtent). I felt it more consistent to use the same parameters in the Editor function.

> I have been able to reproduce this in Safari, using a very small input field and a composition-based IME such as Cangjie I cannot reproduce with Cangjie (and neither Kotoeri Hiragana) on Lion with Safari 5.1.7. > I felt it more consistent to use the same parameters in the Editor function. We're not updating old code in one large swipe, but new code should use new better idioms.

I can reproduce this on Safari 5.1.7 on Lion with Cangjie. The input has to be very small though, as Cangjie compositions can be only 5 characters long and the issue only manifests when the composition is longer than the input field. Using <input size=1> and typing a 5-character Cangjie composition, I find that only the first two characters are visible, and I can't see the last three characters that I type. Does that repro for you? Regarding the method signature change, happy to resolve. Instead of changing revealSelectionAfterEditingOperation, could just call FrameSelection::revealSelection directly from setIgnoreCompositionSelectionChange with the required parameters. Alternatively, happy to add a new method revealSelectionExtentAfterEditingOperation.

That looks like some kind of a bug indeed. The input tries to scroll to end, but doesn't do that very successfully. If you use a wider input and type some other characters first (so that you start typing near the end), the behavior will be different. Also, this does not happen with Kotoeri.

Created attachment 149188 [details] Patch

> That looks like some kind of a bug indeed. Is there anything else that should be improved about the patch (other than the Boolean parameter issue, which I've corrected in the updated patch)?

Comment on attachment 149188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149188&action=review r=me provided you fix all nits below. > Source/WebCore/ChangeLog:13 > + Reviewed by NOBODY (OOPS!). Nit: This line should appear before the long description but after the bug URL. > Source/WebCore/editing/Editor.cpp:2304 > - m_frame->selection()->revealSelection(alignment); > + m_frame->selection()->revealSelection(alignment, revealExtentOption == RevealExtent); We should also change the type of the second argument of FrameSelection::revealSelection to use RevealExtentOption. > LayoutTests/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). Ditto. > LayoutTests/fast/forms/input-set-composition-scroll.html:1 > +<p>This tests whether an input field scrolls to the end of the new composition when setComposition is called.</p> Nit: No DOCTYPE? > LayoutTests/fast/forms/input-set-composition-scroll.html:7 > + document.getElementById('log').appendChild(document.createTextNode(s+"\n")); Nit: Spaces are missing around +. > LayoutTests/fast/forms/input-set-composition-scroll.html:13 > +var inp = document.getElementById('inp'); Nit: Please don't use abbreviations like inp. Spell out input. You can also use document.querySelector('input'); here. > LayoutTests/fast/forms/input-set-composition-scroll.html:17 > +var TOLERANCE = 5; Nit: I don't see a point in declaring this magic as a variable here. It would have read much better if it were in the condition itself. > LayoutTests/fast/forms/input-set-composition-scroll.html:22 > +if (maxScrollLeft - inp.scrollLeft < TOLERANCE) { > + log("SUCCESS: input has scrolled to the end of the composition"); > +} else { > + log("FAILED: input has not scrolled to the end of the composition. scrollLeft=" + inp.scrollLeft); > +} Nit: No curly brackets around single statements.

(In reply to comment #10) > (From update of attachment 149188 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149188&action=review > > > Source/WebCore/editing/Editor.cpp:2304 > > - m_frame->selection()->revealSelection(alignment); > > + m_frame->selection()->revealSelection(alignment, revealExtentOption == RevealExtent); > > We should also change the type of the second argument of FrameSelection::revealSelection to use RevealExtentOption. If you don't mind, I'd like to do that in a followup patch because it will cause the patch to spider a bit (and modify WebCore.exp.in and friends).

Created attachment 149198 [details] Patch for landing

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 149188 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149188&action=review > > > > > Source/WebCore/editing/Editor.cpp:2304 > > > - m_frame->selection()->revealSelection(alignment); > > > + m_frame->selection()->revealSelection(alignment, revealExtentOption == RevealExtent); > > > > We should also change the type of the second argument of FrameSelection::revealSelection to use RevealExtentOption. > > If you don't mind, I'd like to do that in a followup patch because it will cause the patch to spider a bit (and modify WebCore.exp.in and friends). The follow up patch will be in Bug 89833.

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 149188 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149188&action=review > > > > > Source/WebCore/editing/Editor.cpp:2304 > > > - m_frame->selection()->revealSelection(alignment); > > > + m_frame->selection()->revealSelection(alignment, revealExtentOption == RevealExtent); > > > > We should also change the type of the second argument of FrameSelection::revealSelection to use RevealExtentOption. > > If you don't mind, I'd like to do that in a followup patch because it will cause the patch to spider a bit (and modify WebCore.exp.in and friends). Yeah, that's totally fine.

Comment on attachment 149198 [details] Patch for landing Clearing flags on attachment: 149198 Committed r121121: <http://trac.webkit.org/changeset/121121>

All reviewed patches have been landed. Closing bug.

The test included fails on mac bots. Changelist http://trac.webkit.org/changeset/121171 skips the test, and b89845 tracks the failure.

(In reply to comment #9) > > That looks like some kind of a bug indeed. > > Is there anything else that should be improved about the patch (other than the Boolean parameter issue, which I've corrected in the updated patch)? Unsure, it doesn't look like this issue has been fully investigated. Why does this sometimes, but not always? The fix doesn't reflect any such subtleties. Sorry for a late response, I'm on vacation now.