Bug 108053

Summary: Caret is not displayed when trying to focus inside a contenteditable element containing an empty block.
Product: WebKit Reporter: Arpita Bahuguna <arpitabahuguna>
Component: HTML EditingAssignee: Arpita Bahuguna <arpitabahuguna>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, eric, ojan.autocc, ojan, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102359    
Bug Blocks:    
Attachments:
Description Flags
Testpage
none
Patch
none
Testcase with input element
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Arpita Bahuguna 2013-01-27 23:08:22 PST
Created attachment 184941 [details]
Testpage

Steps to reproduce the issue:
Fetch the attached testpage. Try to focus the contenteditable div (by clicking inside the box).
No caret is displayed within the box.

Same is also true for the vertical writing mode.
Comment 1 Arpita Bahuguna 2013-01-28 06:31:17 PST
Created attachment 184977 [details]
Patch
Comment 2 Build Bot 2013-01-28 07:20:09 PST
Comment on attachment 184977 [details]
Patch

Attachment 184977 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16155631

New failing tests:
editing/deleting/4922367.html
Comment 3 WebKit Review Bot 2013-01-28 07:20:12 PST
Comment on attachment 184977 [details]
Patch

Attachment 184977 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16160569

New failing tests:
editing/selection/5076323-2.html
editing/pasteboard/4631972.html
editing/selection/mixed-editability-9.html
editing/selection/editable-non-editable-crash.html
editing/selection/5076323-3.html
editing/deleting/4922367.html
editing/pasteboard/pasting-object.html
editing/selection/table-caret-1.html
editing/pasteboard/4806874.html
editing/inserting/insert-paragraph-01.html
editing/selection/move-by-line-001.html
editing/pasteboard/4944770-1.html
editing/selection/select-element-paragraph-boundary.html
editing/selection/4397952.html
editing/selection/caret-before-select.html
editing/selection/table-caret-3.html
editing/pasteboard/input-field-1.html
editing/selection/5131716-2.html
editing/selection/table-caret-2.html
editing/pasteboard/4641033.html
Comment 4 Build Bot 2013-01-28 07:49:21 PST
Comment on attachment 184977 [details]
Patch

Attachment 184977 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16146855

New failing tests:
editing/deleting/4922367.html
Comment 5 Ryosuke Niwa 2013-01-28 11:19:50 PST
Comment on attachment 184977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184977&action=review

> Source/WebCore/ChangeLog:15
> +        When trying to compute the caret rect for the contenteditable div, the
> +        border and the padding were not considered. Because of this, for the
> +        given test case, which had a border defined on the containing div, the
> +        caret was being painted just atop the border, thereby masking it.

Is the caret before/after input element correctly rendered? For historical reasons, we represent those positions as (input, 0) and (input, 1) respectively.
Comment 6 Ojan Vafai 2013-01-28 12:29:11 PST
These failures are ImageOnlyFailures, so, I suspect that's just the caret drawing in a different (correct?) place and that this change is correct.

Did you mean to mark this patch for review? If so, you need to mark it r?.
Comment 7 Ryosuke Niwa 2013-01-28 18:37:16 PST
I'll emphasize that we can't land this patch unless the caret before/after input element is rendered correctly.
Comment 8 Ojan Vafai 2013-01-28 18:39:43 PST
(In reply to comment #7)
> I'll emphasize that we can't land this patch unless the caret before/after input element is rendered correctly.

What do you mean? Since this patch only changes the x/y of the rect we use to paint the caret how would Positions have anything to do with this?
Comment 9 Ryosuke Niwa 2013-01-28 18:46:04 PST
(In reply to comment #8)
> (In reply to comment #7)
> > I'll emphasize that we can't land this patch unless the caret before/after input element is rendered correctly.
> 
> What do you mean? Since this patch only changes the x/y of the rect we use to paint the caret how would Positions have anything to do with this?

The last time I checked, the caret before and after an input element were represented as inside the input element (input, 0) and (input, 1) respectively. But because we had this bug, they looked as if they were placed outside of the input element (before and after the element). I'm not sure if things have changed since then but I couldn't fix this bug when I tried before because of that.
Comment 10 Arpita Bahuguna 2013-01-29 05:49:03 PST
(In reply to comment #5)
> (From update of attachment 184977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184977&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        When trying to compute the caret rect for the contenteditable div, the
> > +        border and the padding were not considered. Because of this, for the
> > +        given test case, which had a border defined on the containing div, the
> > +        caret was being painted just atop the border, thereby masking it.
> 
> Is the caret before/after input element correctly rendered? For historical reasons, we represent those positions as (input, 0) and (input, 1) respectively.

Hi rniwa, from what I could see on the outset, there is indeed a displacement of the caret before/after the input element (with this fix).
I shall look into this issue further and update.

Also, on Firefox the carets are displaced from their expected position both before and after the input element (as would be the case with this fix) whereas Opera behaves as WebKit does currently.


I do agree that with this fix the placement of the caret before/after input element is not proper.
Currently the caret before/after the input element is placed atop the border of the input element. Is that the expected behavior? I suppose it should be placed taking the element's border/padding into account (i.e. displaced either to the left or the right of the element's border). Would really appreciate your thoughts on the same.
Comment 11 Arpita Bahuguna 2013-01-29 05:51:58 PST
Created attachment 185228 [details]
Testcase with input element

Testcase to verify the placement of the caret before/after an input element.
Comment 12 Ryosuke Niwa 2013-01-29 12:21:28 PST
(In reply to comment #10)
> I do agree that with this fix the placement of the caret before/after input element is not proper.
> Currently the caret before/after the input element is placed atop the border of the input element. Is that the expected behavior? I suppose it should be placed taking the element's border/padding into account (i.e. displaced either to the left or the right of the element's border). Would really appreciate your thoughts on the same.

I think it'll be okay (or even desirable to an extent) if there was a space between the input element's border/margin and the caret when the caret is placed before or after the input element.
Comment 13 Arpita Bahuguna 2013-02-14 00:09:01 PST
Created attachment 188274 [details]
Patch
Comment 14 Ryosuke Niwa 2013-02-14 11:58:45 PST
Comment on attachment 188274 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188274&action=review

> Source/WebCore/rendering/RenderBox.cpp:3877
> +    if (node()->isRootEditableElement()) {

I don't think this makes sense. If anything, we should just work-around the bug of input element by explicitly checking that condition instead.
In particular, we want to keep the work-around when node->canContainRangeEndPoint() returns false.
Comment 15 Arpita Bahuguna 2013-02-15 03:45:28 PST
Created attachment 188528 [details]
Patch
Comment 16 Arpita Bahuguna 2013-02-15 03:53:05 PST
(In reply to comment #14)
> (From update of attachment 188274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188274&action=review
> 
Thanks for the review Rniwa.

> > Source/WebCore/rendering/RenderBox.cpp:3877
> > +    if (node()->isRootEditableElement()) {
> 
> I don't think this makes sense. If anything, we should just work-around the bug of input element by explicitly checking that condition instead.
> In particular, we want to keep the work-around when node->canContainRangeEndPoint() returns false.
I had previously tried a check against input elements but the issue still persisted for elements such as <select> and <textarea>. This lead me to use a more generic approach of applying the border/padding only for the root editable element.
But I see your point here. Have thus modified to now check against form control elements. So border/padding will not be applied for a form control element. This successfully covers the <select> and <textarea> cases as well.
Comment 17 WebKit Review Bot 2013-02-15 04:54:41 PST
Comment on attachment 188528 [details]
Patch

Attachment 188528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16581815

New failing tests:
editing/pasteboard/4631972.html
editing/selection/mixed-editability-9.html
editing/selection/5076323-3.html
editing/deleting/4922367.html
editing/pasteboard/pasting-object.html
editing/selection/table-caret-1.html
editing/selection/5076323-2.html
editing/selection/move-by-line-001.html
editing/inserting/insert-paragraph-01.html
editing/selection/editable-non-editable-crash.html
editing/selection/table-caret-3.html
editing/selection/table-caret-2.html
editing/selection/5131716-2.html
Comment 18 Build Bot 2013-02-15 05:28:36 PST
Comment on attachment 188528 [details]
Patch

Attachment 188528 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16592085

New failing tests:
editing/deleting/4922367.html
Comment 19 WebKit Review Bot 2013-02-15 05:29:34 PST
Comment on attachment 188528 [details]
Patch

Attachment 188528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16580816

New failing tests:
editing/pasteboard/4631972.html
editing/selection/mixed-editability-9.html
editing/selection/5076323-3.html
editing/deleting/4922367.html
editing/pasteboard/pasting-object.html
editing/selection/table-caret-1.html
editing/selection/5076323-2.html
editing/selection/move-by-line-001.html
editing/inserting/insert-paragraph-01.html
editing/selection/editable-non-editable-crash.html
editing/selection/table-caret-3.html
editing/selection/table-caret-2.html
editing/selection/5131716-2.html
Comment 20 Arpita Bahuguna 2013-02-15 06:27:11 PST
Hi rniwa,

The tests fail because of the caret being drawn displaced by the border/padding for <table>, <object> and <iframe> elements placed inside the contenteditable container.

How should we handle these elements (or any other element apart from input/form control elements) that might occur within the contenteditable region?

Should we rather base the check against node->canContainRangeEndPoint()? (Even this check fails for table element).

Your thoughts on the same would be really appreciated.
Comment 21 Ryosuke Niwa 2013-02-15 10:18:47 PST
(In reply to comment #20)
> The tests fail because of the caret being drawn displaced by the border/padding for <table>, <object> and <iframe> elements placed inside the contenteditable container.

That's why I suggested to use canContainRangeEndPoint(). We may need to special case table though because (table, 0) and (table, 1) are valid range positions but we abuse it to mean before/after table.

> How should we handle these elements (or any other element apart from input/form control elements) that might occur within the contenteditable region?
> 
> Should we rather base the check against node->canContainRangeEndPoint()? (Even this check fails for table element).

Yes. Checking against canContainRangeEndPoint() will be the right thing to do (or even editingIgnoresContent). I don't remember if iframe's canContainRangeEndPoint returns false or not. We may need to modify it.
Comment 22 Arpita Bahuguna 2013-02-18 02:07:47 PST
Created attachment 188829 [details]
Patch
Comment 23 Ryosuke Niwa 2013-02-18 04:09:57 PST
Comment on attachment 188829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188829&action=review

> Source/WebCore/rendering/RenderBox.cpp:3871
> +    if (!(editingIgnoresContent(node()) || isTableElement(node()))) {

You should perhaps add a FIXME stating why this condition is necessary because really, we should always be adding border/padding.
The fact we don't do that for some element is a work-around for the bug that we use positions like (table, 0) and (table, 1) to mean before/after table.
Comment 24 Arpita Bahuguna 2013-02-18 05:04:31 PST
Created attachment 188856 [details]
Patch
Comment 25 Arpita Bahuguna 2013-02-18 05:05:38 PST
(In reply to comment #23)
> (From update of attachment 188829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188829&action=review
> 
Thank-you for the r+ rniwa! I have added a FIXME along with the condition check. Hope it is appropriate.
> > Source/WebCore/rendering/RenderBox.cpp:3871
> > +    if (!(editingIgnoresContent(node()) || isTableElement(node()))) {
> 
> You should perhaps add a FIXME stating why this condition is necessary because really, we should always be adding border/padding.
> The fact we don't do that for some element is a work-around for the bug that we use positions like (table, 0) and (table, 1) to mean before/after table.
Comment 26 Arpita Bahuguna 2013-02-19 01:35:27 PST
Created attachment 189023 [details]
Patch
Comment 27 WebKit Review Bot 2013-02-19 02:15:48 PST
Comment on attachment 189023 [details]
Patch

Clearing flags on attachment: 189023

Committed r143313: <http://trac.webkit.org/changeset/143313>
Comment 28 WebKit Review Bot 2013-02-19 02:15:53 PST
All reviewed patches have been landed.  Closing bug.