Bug 82688 - Caret is not rendered properly inside an input element with text-indent
Summary: Caret is not rendered properly inside an input element with text-indent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2012-03-29 22:26 PDT by Ryosuke Niwa
Modified: 2012-04-17 22:45 PDT (History)
10 users (show)

See Also:


Attachments
First try (5.79 KB, patch)
2012-04-13 14:33 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
add missing test cases (7.07 KB, patch)
2012-04-16 07:33 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
updated patch based on Levi's review (7.05 KB, patch)
2012-04-17 13:43 PDT, Yi Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-03-29 22:26:45 PDT
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
Comment 1 Ryosuke Niwa 2012-03-29 22:27:00 PDT
http://crbug.com/53466
Comment 2 Yi Shen 2012-04-13 14:33:20 PDT
Created attachment 137151 [details]
First try
Comment 3 Ryosuke Niwa 2012-04-13 14:50:08 PDT
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.
Comment 4 Yi Shen 2012-04-16 07:33:49 PDT
Created attachment 137347 [details]
add missing test cases
Comment 5 Yi Shen 2012-04-16 07:34:27 PDT
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 6 Levi Weintraub 2012-04-16 14:35:45 PDT
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.
Comment 7 Yi Shen 2012-04-16 14:43:49 PDT
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 8 Levi Weintraub 2012-04-16 14:46:42 PDT
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
Comment 9 Yi Shen 2012-04-16 16:09:01 PDT
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
Comment 10 Levi Weintraub 2012-04-16 16:16:34 PDT
(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 :)
Comment 11 Yi Shen 2012-04-17 13:43:35 PDT
Created attachment 137597 [details]
updated patch based on Levi's review
Comment 12 Ryosuke Niwa 2012-04-17 13:50:54 PDT
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.
Comment 13 Ryosuke Niwa 2012-04-17 13:51:17 PDT
This isn't really an editing bug. It's a rendering bug.
Comment 14 WebKit Review Bot 2012-04-17 16:10:38 PDT
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>
Comment 15 WebKit Review Bot 2012-04-17 16:10:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Jer Noble 2012-04-17 22:45:42 PDT
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.