When you click on <input id="input" type="text" style="text-indent:10px;"> the caret is initially rendered without indentation although typing & deleting any character would move the caret to the correct position. http://simple-rte.rniwa.com/?editor=%3Cinput%20id%3D%22input%22%20type%3D%22text%22%20style%3D%22text-indent%3A10px%3B%22%3E&designmode=false&script=document.getElementById%28%27input%27%29.focus%28%29%3B
http://crbug.com/53466
Created attachment 137151 [details] First try
Comment on attachment 137151 [details] First try View in context: https://bugs.webkit.org/attachment.cgi?id=137151&action=review > Source/WebCore/rendering/RenderBlock.cpp:6483 > + if (currentStyle->isLeftToRightDirection()) > + x += textIndentOffset() / 2; > + else > + x -= textIndentOffset() / 2; Please add a test for this.
Created attachment 137347 [details] add missing test cases
Thanks for the review. Fix it in the latest patch. (In reply to comment #3) > (From update of attachment 137151 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137151&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:6483 > > + if (currentStyle->isLeftToRightDirection()) > > + x += textIndentOffset() / 2; > > + else > > + x -= textIndentOffset() / 2; > > Please add a test for this.
Comment on attachment 137347 [details] add missing test cases View in context: https://bugs.webkit.org/attachment.cgi?id=137347&action=review > Source/WebCore/rendering/RenderBlock.cpp:6495 > + x = min(x, w - (borderRight() + paddingRight()) - caretWidth); This is needed to prevent x from being beyond the edge? Why isn't this needed for the left side as well? Also, why not w - borderRight() - paddingRight() - caretWidth? Same with above. Seems less confusing.
Thanks for reviewing. (In reply to comment #6) > (From update of attachment 137347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137347&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:6495 > > + x = min(x, w - (borderRight() + paddingRight()) - caretWidth); > > This is needed to prevent x from being beyond the edge? Why isn't this needed for the left side as well? I checked firefox & safari and found both webkit & firefox prevent x from being beyond the right edge only. > > Also, why not w - borderRight() - paddingRight() - caretWidth? Same with above. Seems less confusing. I will fix this, thanks :)
Comment on attachment 137347 [details] add missing test cases View in context: https://bugs.webkit.org/attachment.cgi?id=137347&action=review >>> Source/WebCore/rendering/RenderBlock.cpp:6495 >>> + x = min(x, w - (borderRight() + paddingRight()) - caretWidth); >> >> This is needed to prevent x from being beyond the edge? Why isn't this needed for the left side as well? >> >> Also, why not w - borderRight() - paddingRight() - caretWidth? Same with above. Seems less confusing. > > I checked firefox & safari and found both webkit & firefox prevent x from being beyond the right edge only. I'm confused what you mean about WebKit, since this appears to be adding this specifically for WebKit now :p
Sorry for the confusion. WebKit does provide the right caret position for the input element When it has text content -- so, at the moment when the first character entered, I checked the insertion position calculated in RenderText::localCaretRect and found it never beyond right edge. In other words, if we don't check the x value, you will see the caret 'jumps' after inserting first character in following case, "<input id='textIndentTest' type='text' style='text-indent:30px;text-align:right'>". (In reply to comment #8) > (From update of attachment 137347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137347&action=review > > >>> Source/WebCore/rendering/RenderBlock.cpp:6495 > >>> + x = min(x, w - (borderRight() + paddingRight()) - caretWidth); > >> > >> This is needed to prevent x from being beyond the edge? Why isn't this needed for the left side as well? > >> > >> Also, why not w - borderRight() - paddingRight() - caretWidth? Same with above. Seems less confusing. > > > > I checked firefox & safari and found both webkit & firefox prevent x from being beyond the right edge only. > > I'm confused what you mean about WebKit, since this appears to be adding this specifically for WebKit now :p
(In reply to comment #9) > Sorry for the confusion. WebKit does provide the right caret position for the input element When it has text content -- so, at the moment when the first character entered, I checked the insertion position calculated in RenderText::localCaretRect and found it never beyond right edge. > > In other words, if we don't check the x value, you will see the caret 'jumps' after inserting first character in following case, "<input id='textIndentTest' type='text' style='text-indent:30px;text-align:right'>". Ahhh, that explains it. Thanks for clearing that up :)
Created attachment 137597 [details] updated patch based on Levi's review
Comment on attachment 137597 [details] updated patch based on Levi's review View in context: https://bugs.webkit.org/attachment.cgi?id=137597&action=review > Source/WebCore/rendering/RenderBlock.cpp:6501 > switch (alignment) { > case alignLeft: > + if (currentStyle->isLeftToRightDirection()) > + x += textIndentOffset(); > break; > case alignCenter: > x = (x + w - (borderRight() + paddingRight())) / 2; > + if (currentStyle->isLeftToRightDirection()) > + x += textIndentOffset() / 2; > + else > + x -= textIndentOffset() / 2; > break; > case alignRight: > x = w - (borderRight() + paddingRight()) - caretWidth; > + if (!currentStyle->isLeftToRightDirection()) > + x -= textIndentOffset(); > break; > } > + x = min(x, w - borderRight() - paddingRight() - caretWidth); We need to fix this code for vertical writing mode but that's probably a separate bug.
This isn't really an editing bug. It's a rendering bug.
Comment on attachment 137597 [details] updated patch based on Levi's review Clearing flags on attachment: 137597 Committed r114461: <http://trac.webkit.org/changeset/114461>
All reviewed patches have been landed. Closing bug.
This commit, due to its dependency on textInputController, is broken on all WK2 bots. Adding to skipped list, and making a note in bug #42337.