Bug 141218

Summary: Div having contentEditable and display:flex cannot be edited if it is empty.
Product: WebKit Reporter: ChangSeok Oh <changseok>
Component: HTML EditingAssignee: ChangSeok Oh <changseok>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, enrica, esprehn+autocc, hyatt, kangil.han, kling, koivisto, ossy, rego, rniwa
Priority: P2    
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

Description ChangSeok Oh 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
Comment 1 ChangSeok Oh 2015-02-03 19:42:53 PST 
Created attachment 246008 [details]
Patch
Comment 2 ChangSeok Oh 2015-02-03 22:00:43 PST 
Created attachment 246016 [details]
Patch
Comment 3 ChangSeok Oh 2015-02-03 22:15:42 PST 
Created attachment 246017 [details]
Patch
Comment 4 Ryosuke Niwa 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.
Comment 5 Brian Burg 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 ""
Comment 6 Manuel Rego Casasnovas 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. :-)
Comment 7 Manuel Rego Casasnovas 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.
Comment 8 ChangSeok Oh 2015-02-11 00:06:06 PST 
Created attachment 246378 [details]
Patch
Comment 9 ChangSeok Oh 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.
Comment 10 ChangSeok Oh 2015-02-11 00:45:45 PST 
Comment on attachment 246378 [details]
Patch

Got r+ from rniwa, so I land the patch.
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2015-02-11 00:52:05 PST 
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 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.
Comment 14 WebKit Commit Bot 2015-02-11 07:23:59 PST 
Re-opened since this is blocked by bug 141473
Comment 15 ChangSeok Oh 2015-02-11 11:04:08 PST 
Created attachment 246398 [details]
Patch
Comment 16 ChangSeok Oh 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.
Comment 17 WebKit Commit Bot 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>
Comment 18 WebKit Commit Bot 2015-02-11 12:13:25 PST 
All reviewed patches have been landed.  Closing bug.