Bug 141465 – Div having contentEditable and display:grid cannot be edited if it is empty.

• Component: HTML Editing
• Status: RESOLVED FIXED

[ChangSeok Oh]

This will fix a similar issue with Bug 141218, the only difference from 141218 is that a div has display:grid.

[ChangSeok Oh]

Manuel Rego Casasnovas said :
>> 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.

[Manuel Rego Casasnovas]

Just to give you more information. There're some of these bugs that are both in flex and grid, mostly because of they're not RenderBlockFlow.

In some cases, we need some special methods to check that they're not RenderBlockFlow but they're not buttons either (as RenderButton, like RenderFullScreen) inherit from RenderFlexibleBox.

For example take a look to this method:
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock.cpp#L2927

[ChangSeok Oh]

(In reply to comment #2)
> Just to give you more information. There're some of these bugs that are both
> in flex and grid, mostly because of they're not RenderBlockFlow.
> 
> In some cases, we need some special methods to check that they're not
> RenderBlockFlow but they're not buttons either (as RenderButton, like
> RenderFullScreen) inherit from RenderFlexibleBox.
> 
> For example take a look to this method:
> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock.
> cpp#L2927

Thanks for the comment. But I'm not sure if I understand your intention correctly. Why do we need to check that? As my understanding any element can be editable with the contenteditable attribute. https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable
Actually I checked RenderFullScreen is allowed to edit in firefox.
Could you provide me a sample which should not be allowed to edit even though it has a contenteditable attribute?
I think just adding is<RenderGrid>(*renderer) is enough there? (Of course more touches are required to make grid work properly for this case.)

[ChangSeok Oh]

Created attachment 246469 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #3)
> (In reply to comment #2)
> > Just to give you more information. There're some of these bugs that are both
> > in flex and grid, mostly because of they're not RenderBlockFlow.
> > 
> > In some cases, we need some special methods to check that they're not
> > RenderBlockFlow but they're not buttons either (as RenderButton, like
> > RenderFullScreen) inherit from RenderFlexibleBox.
> > 
> > For example take a look to this method:
> > http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock.
> > cpp#L2927
> 
> Thanks for the comment. But I'm not sure if I understand your intention
> correctly. Why do we need to check that? As my understanding any element can
> be editable with the contenteditable attribute.
> https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable
> Actually I checked RenderFullScreen is allowed to edit in firefox.
> Could you provide me a sample which should not be allowed to edit even
> though it has a contenteditable attribute?
> I think just adding is<RenderGrid>(*renderer) is enough there? (Of course
> more touches are required to make grid work properly for this case.)

Sorry for the misleading comment. I was not thinking about editable or not, just about how we usually detect in other parts of the code that a renderer is a flex or a grid. We usually use that kind of methods, as flex and grid don't inherit from RenderBlockFlow. However, sometimes we need that buttons, full-screen and other that inherit from RenderFlexibleBox behave like a RenderBlockFlow and we've this kind of special method.
Anyway you can forget about my comment, sorry for the noise.

[Sergio Villar Senin]

Comment on attachment 246469 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This bug is qutie similar with webkit.org/b/141218. RenderGrid should be also treated as a candidate

qutie -> quite

similar with -> similar to

> Source/WebCore/ChangeLog:11
> +        a minimum height of a single line if it is editable as well as RenderFlexibleBox does.

Where is this specified?

> LayoutTests/fast/events/key-events-in-editable-gridbox.html:7
> +        display: grid;

No need for the unprefixed version.

[Sergio Villar Senin]

Comment on attachment 246469 [details]
Patch

Setting cq- while we wait for clarifications.

[ChangSeok Oh]

Comment on attachment 246469 [details]
Patch

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

Thanks for the review guys! Let me update the patch soon. =)

>> Source/WebCore/ChangeLog:8
>> +        This bug is qutie similar with webkit.org/b/141218. RenderGrid should be also treated as a candidate
> 
> qutie -> quite
> 
> similar with -> similar to

Oops thanks.

>> Source/WebCore/ChangeLog:11
>> +        a minimum height of a single line if it is editable as well as RenderFlexibleBox does.
> 
> Where is this specified?

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp#L707

>> LayoutTests/fast/events/key-events-in-editable-gridbox.html:7
>> +        display: grid;
> 
> No need for the unprefixed version.

O.K Removed. BTW doesn't webkit have a plan to remove the --webkit- prefix in the future?

[ChangSeok Oh]

Created attachment 246525 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 246525 [details]
Patch

Clearing flags on attachment: 246525

Committed r180050: <http://trac.webkit.org/changeset/180050>

[Csaba Osztrogonác]

(In reply to comment #10)
> Comment on attachment 246525 [details]
> Patch
> 
> Clearing flags on attachment: 246525
> 
> Committed r180050: <http://trac.webkit.org/changeset/180050>

It broke the !ENABLE(CSS_GRID_LAYOUT) build - new bug report: bug141647

[Sergio Villar Senin]

Closing this as it landed some time ago.