Bug 108053 – Caret is not displayed when trying to focus inside a contenteditable element containing an empty block.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Arpita Bahuguna]

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.

[Arpita Bahuguna]

Created attachment 184977 [details]
Patch

[Build Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Ojan Vafai]

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?.

[Ryosuke Niwa]

I'll emphasize that we can't land this patch unless the caret before/after input element is rendered correctly.

[Ojan Vafai]

(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?

[Ryosuke Niwa]

(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.

[Arpita Bahuguna]

(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.

[Arpita Bahuguna]

Created attachment 185228 [details]
Testcase with input element

Testcase to verify the placement of the caret before/after an input element.

[Ryosuke Niwa]

(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.

[Arpita Bahuguna]

Created attachment 188274 [details]
Patch

[Ryosuke Niwa]

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.

[Arpita Bahuguna]

Created attachment 188528 [details]
Patch

[Arpita Bahuguna]

(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.

[WebKit Review Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

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.

[Ryosuke Niwa]

(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.

[Arpita Bahuguna]

Created attachment 188829 [details]
Patch

[Ryosuke Niwa]

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.

[Arpita Bahuguna]

Created attachment 188856 [details]
Patch

[Arpita Bahuguna]

(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.

[Arpita Bahuguna]

Created attachment 189023 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189023 [details]
Patch

Clearing flags on attachment: 189023

Committed r143313: <http://trac.webkit.org/changeset/143313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.