Bug 141218

Summary:

Div having contentEditable and display:flex cannot be edited if it is empty.

Product:

WebKit

Reporter:

ChangSeok Oh <changseok>

Component:

HTML Editing

Assignee:

ChangSeok Oh <changseok>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, enrica, esprehn+autocc, hyatt, kangil.han, kling, koivisto, ossy, rego, rniwa

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=226926

Bug Depends on:

141473

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

ChangSeok Oh

Reported 2015-02-03 16:32:28 PST

This bug is reported in chromium[1], but it happens on webkit as well. I proposed a patch for blink[2], I'd like to apply it to webkit, too. [1] http://crbug.com/450617 [2] https://codereview.chromium.org/897633002

Attachments
Patch (5.17 KB, patch) 2015-02-03 19:42 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (5.35 KB, patch) 2015-02-03 22:00 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (5.35 KB, patch) 2015-02-03 22:15 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (4.99 KB, patch) 2015-02-11 00:06 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Patch (5.11 KB, patch) 2015-02-11 11:04 PST, ChangSeok Oh	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

ChangSeok Oh

Comment 1 2015-02-03 19:42:53 PST

Created attachment 246008 [details] Patch

ChangSeok Oh

Comment 2 2015-02-03 22:00:43 PST

Created attachment 246016 [details] Patch

ChangSeok Oh

Comment 3 2015-02-03 22:15:42 PST

Created attachment 246017 [details] Patch

Ryosuke Niwa

Comment 4 2015-02-10 10:30:09 PST

Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > LayoutTests/fast/events/key-events-in-editable-flexbox.html:1 > +<html> Missing DOCTYPE. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:24 > + var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1; > + if (onMacPlatform) > + eventSender.keyDown("rightArrow", ["metaKey"]); > + else > + eventSender.keyDown("end"); Use getSelection().modify('move', 'forward', 'lineboundary') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:29 > + eventSender.keyDown('E'); > + eventSender.keyDown('S'); > + eventSender.keyDown('T'); > + shouldBeEqualToString("targetDiv.innerText", "TEST"); Use document.execCommand('insertText') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:36 > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); // Remove '\n' > + shouldBeEmptyString("targetDiv.innerText"); Use document.execCommand('delete') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:42 > + eventSender.keyDown('T'); > + eventSender.keyDown('E'); > + eventSender.keyDown('S'); > + eventSender.keyDown('T'); > + shouldBeEqualToString("targetDiv.innerText", "TEST"); Ditto. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:50 > + <body onload="test()"> > + <div id="target" contentEditable>T</div> > + </body> I don't think we normally indent test contents. Also your indentation is inconsistent. Sometimes it's 4 spaces and sometimes 2 spaces. Please at least be consistent.

Brian Burg

Comment 5 2015-02-10 10:52:26 PST

Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > Source/WebCore/ChangeLog:13 > + having âdisplay : flexâ goes weird and VisibleSelection is empty accordingly. The quote marks here do not seem to render in the review tool. Please use these ASCII marks ""

Manuel Rego Casasnovas

Comment 6 2015-02-10 11:30:01 PST

Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > Source/WebCore/dom/Position.cpp:939 > + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { I think this is going to be failing for grid too. Maybe you could use here !renderer->isRenderBlockFlow() to cover both cases. Also adding a test case for grid would be nice. :-)

Manuel Rego Casasnovas

Comment 7 2015-02-10 14:40:35 PST

(In reply to comment #6) > Comment on attachment 246017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246017&action=review > > > Source/WebCore/dom/Position.cpp:939 > > + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { > > I think this is going to be failing for grid too. Maybe you could use here > !renderer->isRenderBlockFlow() to cover both cases. Mmmm, I've just realized that this won't make any sense. I'm wondering if we should just add is<RenderGrid> or if this "if" is actually needed for something else. Anyway, this is failing in grid too.

ChangSeok Oh

Comment 8 2015-02-11 00:06:06 PST

Created attachment 246378 [details] Patch

ChangSeok Oh

Comment 9 2015-02-11 00:18:07 PST

Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review Thanks for all comments. >> Source/WebCore/ChangeLog:13 >> + having âdisplay : flexâ goes weird and VisibleSelection is empty accordingly. > > The quote marks here do not seem to render in the review tool. Please use these ASCII marks "" Done. >>> Source/WebCore/dom/Position.cpp:939 >>> + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { >> >> I think this is going to be failing for grid too. Maybe you could use here !renderer->isRenderBlockFlow() to cover both cases. >> >> Also adding a test case for grid would be nice. :-) > > Mmmm, I've just realized that this won't make any sense. I'm wondering if we should just add is<RenderGrid> or if this "if" is actually needed for something else. > > Anyway, this is failing in grid too. Hrm.. It sounds getting complicated. Let's cover the grid issue in a new bug. I opened http://webkit.org/b/141465 >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:1 >> +<html> > > Missing DOCTYPE. Done >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:24 >> + eventSender.keyDown("end"); > > Use getSelection().modify('move', 'forward', 'lineboundary') instead. Nice improvement. Good to know. thanks. =) >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:29 >> + shouldBeEqualToString("targetDiv.innerText", "TEST"); > > Use document.execCommand('insertText') instead. ditto >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:36 >> + shouldBeEmptyString("targetDiv.innerText"); > > Use document.execCommand('delete') instead. ditto. >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:42 >> + shouldBeEqualToString("targetDiv.innerText", "TEST"); > > Ditto. O.K >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:50 >> + </body> > > I don't think we normally indent test contents. Also your indentation is inconsistent. > Sometimes it's 4 spaces and sometimes 2 spaces. Please at least be consistent. Done.

ChangSeok Oh

Comment 10 2015-02-11 00:45:45 PST

Comment on attachment 246378 [details] Patch Got r+ from rniwa, so I land the patch.

WebKit Commit Bot

Comment 11 2015-02-11 00:52:00 PST

Comment on attachment 246378 [details] Patch Clearing flags on attachment: 246378 Committed r179921: <http://trac.webkit.org/changeset/179921>

WebKit Commit Bot

Comment 12 2015-02-11 00:52:05 PST

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 13 2015-02-11 06:33:36 PST

(In reply to comment #11) > Comment on attachment 246378 [details] > Patch > > Clearing flags on attachment: 246378 > > Committed r179921: <http://trac.webkit.org/changeset/179921> It made tests crash/assert on the debug bots.

WebKit Commit Bot

Comment 14 2015-02-11 07:23:59 PST

Re-opened since this is blocked by bug 141473

ChangSeok Oh

Comment 15 2015-02-11 11:04:08 PST

Created attachment 246398 [details] Patch

ChangSeok Oh

Comment 16 2015-02-11 11:05:48 PST

(In reply to comment #13) > (In reply to comment #11) > > Comment on attachment 246378 [details] > > Patch > > > > Clearing flags on attachment: 246378 > > > > Committed r179921: <http://trac.webkit.org/changeset/179921> > > It made tests crash/assert on the debug bots. Wrong downcasting caused the assertion failure. Updated patch should fix it.

WebKit Commit Bot

Comment 17 2015-02-11 12:13:19 PST

Comment on attachment 246398 [details] Patch Clearing flags on attachment: 246398 Committed r179944: <http://trac.webkit.org/changeset/179944>

WebKit Commit Bot

Comment 18 2015-02-11 12:13:25 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.