Bug 108053

Created attachment 184977 [details] Patch

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

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

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

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

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

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

Created attachment 185228 [details] Testcase with input element Testcase to verify the placement of the caret before/after an input element.

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

Created attachment 188274 [details] Patch

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.

Created attachment 188528 [details] Patch

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

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.

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

Created attachment 188829 [details] Patch

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.

Created attachment 188856 [details] Patch

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

Created attachment 189023 [details] Patch

Comment on attachment 189023 [details] Patch Clearing flags on attachment: 189023 Committed r143313: <http://trac.webkit.org/changeset/143313>

All reviewed patches have been landed. Closing bug.