Caret is not rendered properly inside an input element with text-indent

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Ryosuke Niwa]

http://crbug.com/53466

[Yi Shen]

Created attachment 137151 [details]
First try

[Ryosuke Niwa]

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.

[Yi Shen]

Created attachment 137347 [details]
add missing test cases

[Yi Shen]

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.

[Levi Weintraub]

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.

[Yi Shen]

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 :)

[Levi Weintraub]

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

[Yi Shen]

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

[Levi Weintraub]

(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 :)

[Yi Shen]

Created attachment 137597 [details]
updated patch based on Levi's review

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

This isn't really an editing bug. It's a rendering bug.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

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.