Steps to reproduce: 1. Enter some text into a textarea, including blank lines 2. Tap on various lines in the text area. 3. Repeat using a mouse. I expect tapping on a position to have essentially the same behavior as clicking on it. Instead, touch adjustment is tweaking the position to the nearest text. This makes it almost impossible to position the cursor on an empty line using touch gestures.
Created attachment 165640 [details] Patch
I am confused why adjustment happens in this case. Is any of the text 'clickable', or are they just responding to tap because the textarea is focusable? In any case I don't like making a special exception for textareas, because the same problem is likely to manifest in other forms or content-editable elements as well.
(In reply to comment #2) > I am confused why adjustment happens in this case. Is any of the text 'clickable', or are they just responding to tap because the textarea is focusable? > > In any case I don't like making a special exception for textareas, because the same problem is likely to manifest in other forms or content-editable elements as well. Yes, it sees that the text area is focusable as it bubbles up through the parents of the text looking for responders. This results in the text nodes being candidates for touch adjustment. In most cases, we do want touch adjustment to apply to children of a form element, such as the search cancel button, media controls, date picker, etc. Cases were we want to suppress touch adjustment on child nodes that pass the filter check may be rare. Though not excited about adding an explicit exception in touch adjustment, I'm also reluctant to generalize until we see a pattern emerge. There is a danger of winding up with an over-designed or awkward API. Possible that we can bail early in the parent traversal for a responder on a text nodes. This would allow us to catch text nodes that are links and prevent adjustment on text nodes in other cases. Effectively makes link the exception rather than textarea.
(In reply to comment #3) > In most cases, we do want touch adjustment to apply to children of a form element, such as the search cancel button, media controls, date picker, etc. Cases were we want to suppress touch adjustment on child nodes that pass the filter check may be rare. > Well, in those cases the children would themselves be handling a tap event. While in this case it is only their form parent handling it. > Though not excited about adding an explicit exception in touch adjustment, I'm also reluctant to generalize until we see a pattern emerge. There is a danger of winding up with an over-designed or awkward API. > To me this seems like an extension of the idea we now use of not adjusting the point if it is already inside the target. In this case the point is also inside the target ultimately handling the tap-event, but the touch-adjustment logic is comparing the touch point to all the subtarget areas of the text children instead of the big area of the textarea. I will see if I can figure out a nice safe way to avoid adjusting the point if it is already inside the responder of the target.
Created attachment 165766 [details] Patch Proposal This patch is a wildly different approach. It changes what targets we adjust to, by adjusting to the actual responding nodes instead of trying to adjust the best child of them. Hopefully this should help detect the cases where we do not need to adjust the point at all because it is already contained in the selected target. A few test-cases needed to be updated though since they relied on touch adjustment to non-responding nodes.
Attachment 165766 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/touc..." exit_code: 1 Source/WebCore/page/TouchAdjustment.cpp:265: Missing space before ( in for( [whitespace/parens] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #4) > (In reply to comment #3) > > In most cases, we do want touch adjustment to apply to children of a form element, such as the search cancel button, media controls, date picker, etc. Cases were we want to suppress touch adjustment on child nodes that pass the filter check may be rare. > > > Well, in those cases the children would themselves be handling a tap event. While in this case it is only their form parent handling it. > > > Though not excited about adding an explicit exception in touch adjustment, I'm also reluctant to generalize until we see a pattern emerge. There is a danger of winding up with an over-designed or awkward API. > > > To me this seems like an extension of the idea we now use of not adjusting the point if it is already inside the target. In this case the point is also inside the target ultimately handling the tap-event, but the touch-adjustment logic is comparing the touch point to all the subtarget areas of the text children instead of the big area of the textarea. > > I will see if I can figure out a nice safe way to avoid adjusting the point if it is already inside the responder of the target. Appears to be straightforward enough to implement, though the change would have farther reaching impact. One common strategy is to consolidate mouse handlers to improve performance (notably for pages that must work with older versions of IE). If a mouse handler is added to a toolbar rather than each of its buttons (relying on event.getTarget() to delegate to the correct button), the proposed fix would prevent touch adjustment to the toolbar buttons. This is just one example where the current behavior of snapping to children is actually a good thing. Overall, snapping to finer grained content seems worthwhile, and text area (or possibly non-link text content in general) seems like an exception because of the issue with blank lines.
(In reply to comment #7) > Appears to be straightforward enough to implement, though the change would have farther reaching impact. One common strategy is to consolidate mouse handlers to improve performance (notably for pages that must work with older versions of IE). If a mouse handler is added to a toolbar rather than each of its buttons (relying on event.getTarget() to delegate to the correct button), the proposed fix would prevent touch adjustment to the toolbar buttons. This is just one example where the current behavior of snapping to children is actually a good thing. Overall, snapping to finer grained content seems worthwhile, and text area (or possibly non-link text content in general) seems like an exception because of the issue with blank lines. That is true. I knew there had to be a reason I originally wrote the algorithm that way. I think the issue is actually more that the blank lines does not have appropiate sub targets. Since they can be used to set the caret, they are obviously "active" targets, and should have corresponding sub-target areas that touch adjustment can use.
Allan, I don't believe your patch proposal addresses the problem since willRespondToMouseClickEvents returns true for editable content, which in turns means that nodeRespondsToTapGesture will return true for nodes inside a text area. Updating the tap gesture filter to skip text nodes that are content editable introduces a test regression in disabled-formelements.html. Think we are opening a can of worms trying to generalize a fix required for textarea.
(In reply to comment #9) > Allan, I don't believe your patch proposal addresses the problem since willRespondToMouseClickEvents returns true for editable content, which in turns means that nodeRespondsToTapGesture will return true for nodes inside a text area. Updating the tap gesture filter to skip text nodes that are content editable introduces a test regression in disabled-formelements.html. Think we are opening a can of worms trying to generalize a fix required for textarea. Some regressions are caused by the tests not being perfectly constructed. If you only get one, you should investigate it and figure out of it is important.
(In reply to comment #10) > (In reply to comment #9) > > Allan, I don't believe your patch proposal addresses the problem since willRespondToMouseClickEvents returns true for editable content, which in turns means that nodeRespondsToTapGesture will return true for nodes inside a text area. Updating the tap gesture filter to skip text nodes that are content editable introduces a test regression in disabled-formelements.html. Think we are opening a can of worms trying to generalize a fix required for textarea. > > Some regressions are caused by the tests not being perfectly constructed. If you only get one, you should investigate it and figure out of it is important. The one regression is symptomatic of a larger problem. Here are the issues: - If nodeRespondsToTapGestures returns false for editable text nodes we still the problem of finding a responder on an ancestor, which causes the candidate to be included. - If we further block the propagation through parents (only for editable text), we don't find inputs, which is why disabled-formelements.html is failing. - If we allow the propagation, but use the responders bounds (and make editable text a non-responder) rather than the child bounds, we reduce the usefulness of touch adjustment in cases where the mouse handler code is consolidated like in the toolbar example discussed previously. Things get more interesting when we consider links within editable content, which probably should get touch adjustment. What about other nodes within editable divs?
(In reply to comment #11) > The one regression is symptomatic of a larger problem. Here are the issues: > > - If nodeRespondsToTapGestures returns false for editable text nodes we still the problem of finding a responder on an ancestor, which causes the candidate to be included. > - If we further block the propagation through parents (only for editable text), we don't find inputs, which is why disabled-formelements.html is failing. > - If we allow the propagation, but use the responders bounds (and make editable text a non-responder) rather than the child bounds, we reduce the usefulness of touch adjustment in cases where the mouse handler code is consolidated like in the toolbar example discussed previously. > > Things get more interesting when we consider links within editable content, which probably should get touch adjustment. What about other nodes within editable divs? Yeah, it is not easy. With all these issues it seems the only real solution is to make targets for editable empty lines, because that is the real cause of the problem, that empty lines does not get represented as targetable areas, when they are actually valid targets. Now actually doing that, is not so easy, and will probably require messing with absolute quads and line boxes. So on the short term, it looks like the original patch is the only simple solution, but it will need some comments to explain that is an specialized solution to one issue of a bigger problem.
Will gladly add a TODO. :)
Created attachment 165864 [details] Patch
Comment on attachment 165864 [details] Patch As per discussion, this is an acceptable work-around for now.
(In reply to comment #15) > (From update of attachment 165864 [details]) > As per discussion, this is an acceptable work-around for now. Tonikitoo, can you please take a look.
Comment on attachment 165864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165864&action=review > Source/WebCore/page/TouchAdjustment.cpp:234 > + // Do not adjust within a <textarea> to enable targeting a blank line between long lines of text. do you have the same problem with <p contenteditable="true">This is an editable paragraph, please manually add an empty line here</p> ?
Comment on attachment 165864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165864&action=review >> Source/WebCore/page/TouchAdjustment.cpp:234 >> + // Do not adjust within a <textarea> to enable targeting a blank line between long lines of text. > > do you have the same problem with <p contenteditable="true">This is an editable paragraph, please manually add an empty line here</p> ? Explored using content editable and determined that things are not very clear cut. If you had a link within a content editable paragraph, should adjustment be used? The input field for search cancel is editable, which could result in biasing toward the cancel button if we avoid all touch adjustment on editable content. It certainly deserves a closer look, but this particular use case is quite pressing.
Comment on attachment 165864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165864&action=review >>> Source/WebCore/page/TouchAdjustment.cpp:234 >>> + // Do not adjust within a <textarea> to enable targeting a blank line between long lines of text. >> >> do you have the same problem with <p contenteditable="true">This is an editable paragraph, please manually add an empty line here</p> ? > > Explored using content editable and determined that things are not very clear cut. If you had a link within a content editable paragraph, should adjustment be used? The input field for search cancel is editable, which could result in biasing toward the cancel button if we avoid all touch adjustment on editable content. It certainly deserves a closer look, but this particular use case is quite pressing. In answer to your original question, yes any multi-line editable content is potentially problematic for touch adjustment.
> In answer to your original question, yes any multi-line editable content is potentially problematic for touch adjustment. a multi line textarea is essentially the same as a <div contentEditable=true>, since the former has an "made of" a shadow dom div in content editable. That said, can not we generalize at least to all content editable? (not 'X' in search fields or whatever else)? ps: I read the discussion above.
I think it could be possible to make an alternative version of RenderBlock::absoluteQuads and use that for RenderBlocks with inline children. The current one just returns its own border-box. It could instead or additionally return the line boxes, where the lines top and bottom are read from RootInlineBox::selectionTop and RootInlineBox::selectionBottom, but the width from RenderBlock::contentWidth. Though this could potentially interfere with floats.
Special handling for render blocks with inline children sounds a bit risky. Would this not break touch adjustment for multiline links? Expect to get back to this patch soon. Have a few options to explore. If we want a common solution for all editable content, one possible solution is to post-process the candidate list to amalgamate children that are within a common editable ancestor. Need to do a bit of further investigation to ensure that this doesn't break input fields that contain decorators such as cancel or speech buttons.
(In reply to comment #22) > Special handling for render blocks with inline children sounds a bit risky. Would this not break touch adjustment for multiline links? > It shouldn't. The lines compete on equal ground with other elements, and they are only generated for editable paragraphs (blocks with inline content).
Created attachment 169478 [details] Patch
Comment on attachment 169478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169478&action=review > Source/WebCore/page/TouchAdjustment.cpp:290 > + if (candidate->isContentEditable()) { > + Node* replacement = candidate; > + Node* parent = candidate->parentOrHostNode(); > + while (parent && parent->isContentEditable()) { > + replacement = parent; > + if (editableAncestors.contains(replacement)) { > + replacement = 0; > + break; > + } > + editableAncestors.add(replacement); > + parent = parent->parentOrHostNode(); > + } > + candidate = replacement; > + } Would it be possible to only consolidate nodes sharing the same responder? Thereby leaving links inside the editable content unchanged? > Source/WebCore/page/TouchAdjustment.cpp:342 > + float targetArea = max(rect.size().area(), 1); Can you explain what this change achieves?
Comment on attachment 169478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169478&action=review >> Source/WebCore/page/TouchAdjustment.cpp:342 >> + float targetArea = max(rect.size().area(), 1); > > Can you explain what this change achieves? The change guards against division by zero. Was seeing some nan scores when clicking near the beginning of a blank line without the change.
Comment on attachment 169478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169478&action=review >> Source/WebCore/page/TouchAdjustment.cpp:290 >> + } > > Would it be possible to only consolidate nodes sharing the same responder? Thereby leaving links inside the editable content unchanged? Each text segment is its own responder simply by virtue of being editable.
Tonikitoo, can you please take a look at the latest patch.
Comment on attachment 169478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169478&action=review Looks good >>> Source/WebCore/page/TouchAdjustment.cpp:342 >>> + float targetArea = max(rect.size().area(), 1); >> >> Can you explain what this change achieves? > > The change guards against division by zero. Was seeing some nan scores when clicking near the beginning of a blank line without the change. was ´area´ zero or clamped to zero?
Comment on attachment 169478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169478&action=review >>>> Source/WebCore/page/TouchAdjustment.cpp:342 >>>> + float targetArea = max(rect.size().area(), 1); >>> >>> Can you explain what this change achieves? >> >> The change guards against division by zero. Was seeing some nan scores when clicking near the beginning of a blank line without the change. > > was ´area´ zero or clamped to zero? Area was zero for blank line element. Presumably element's width was zero.
Comment on attachment 169478 [details] Patch Clearing flags on attachment: 169478 Committed r132488: <http://trac.webkit.org/changeset/132488>
All reviewed patches have been landed. Closing bug.