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

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

This bug is quite similar to https://codereview.chromium.org/897633002/.
RenderGrid should be also treated as a candidate for visible position
as like RenderFlexibleBox. The only different situation between them is
that RenderGrid has a bug setting "0px" for logicalHeight when it is empty.
RenderGrid should also have a minimum height of a single line if it is editable
as well as RenderFlexibleBox does.
This is the same patch applied to webkit. http://webkit.org/b/141465

BUG=79180
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190936

[changseok, 2015-02-13 15:50:41]

shivamidow@gmail.com changed reviewers:
+ eae@chromium.org, rego@igalia.com, svillar@igalia.com

[changseok, 2015-02-13 15:53:59]

Please have a look.

[Manuel Rego, 2015-02-16 09:18:40]

rego@igalia.com changed reviewers:
+ jchaffraix@chromium.org

[Manuel Rego, 2015-02-16 09:18:40]

Adding @jchaffraix as reviewer, as he's usually reviewing the grid patches.

[changseok, 2015-02-20 08:07:36]

Please review this.  I think this chance is reliable.

[changseok, 2015-02-20 08:08:22]

On 2015/02/20 08:07:36, changseok wrote:
> Please review this.  I think this chance is reliable.
Oops %s/chance/change

[changseok, 2015-02-24 17:00:57]

shivamidow@gmail.com changed reviewers:
+ cbiesinger@chromium.org

[changseok, 2015-02-24 17:00:57]

+cbiesinger as a reviewer.

[cbiesinger, 2015-02-24 17:37:01]

lgtm

but I'm not an OWNER

[rune, 2015-02-25 11:34:52]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2015-02-25 11:34:53]

A general comment about adding height for to elements with contentEditable:

Currently, the mere presence of the contentEditable attribute changes the layout
of an empty block/flex/grid. That doesn't sound right. Shouldn't that just be
applied while editing?

Gecko does not add this empty line unless focused for editing.

https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1158: }
This code is identical to the code in RenderFlexibleBox. How about making this a
method on RenderBlock?

I don't have a good name for it, though.

[changseok, 2015-02-25 17:24:16]

On 2015/02/25 11:34:53, rune wrote:
> A general comment about adding height for to elements with contentEditable:
> 
> Currently, the mere presence of the contentEditable attribute changes the
layout
> of an empty block/flex/grid. That doesn't sound right. Shouldn't that just be
> applied while editing?
> 
> Gecko does not add this empty line unless focused for editing.

Yeah. Gecko doesn't add an empty line for the case, so that it makes users hard
to re-focus the grid box having 0px height after once losing a focus.
Nowhere specifies the contentEditable case in the spec, so each browsers show
different behaviors for the case.
IMHO, why an empty editable flex which has the same issue with grid box has an
empty line is absolutely due to usability, not about spec.

If we decide not to add an empty line for an empty editable grid, we may need to
touch here as well. Because the grid would not have logical height.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
 
>
https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderGrid.cpp (right):
> 
>
https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:1158: }
> This code is identical to the code in RenderFlexibleBox. How about making this
a
> method on RenderBlock?
> 
> I don't have a good name for it, though.

How about addEmptyLineIfNeeded? =)

[rune, 2015-02-25 21:01:35]

On 2015/02/25 17:24:16, changseok wrote:
> On 2015/02/25 11:34:53, rune wrote:
> > A general comment about adding height for to elements with contentEditable:
> > 
> > Currently, the mere presence of the contentEditable attribute changes the
> layout
> > of an empty block/flex/grid. That doesn't sound right. Shouldn't that just
be
> > applied while editing?
> > 
> > Gecko does not add this empty line unless focused for editing.
> 
> Yeah. Gecko doesn't add an empty line for the case, so that it makes users
hard
> to re-focus the grid box having 0px height after once losing a focus.
> Nowhere specifies the contentEditable case in the spec, so each browsers show
> different behaviors for the case.
> IMHO, why an empty editable flex which has the same issue with grid box has an
> empty line is absolutely due to usability, not about spec.
> 
> If we decide not to add an empty line for an empty editable grid, we may need
to
> touch here as well. Because the grid would not have logical height.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I was just mentioning, it's out of scope for this CL anyway.

>
https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderGrid.cpp (right):
> > 
> >

>
https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
> > Source/core/rendering/RenderGrid.cpp:1158: }
> > This code is identical to the code in RenderFlexibleBox. How about making
this
> a
> > method on RenderBlock?
> > 
> > I don't have a good name for it, though.
> 
> How about addEmptyLineIfNeeded? =)

sgtm

[cbiesinger, 2015-02-26 01:12:29]

On 2015/02/25 21:01:35, rune wrote:
> On 2015/02/25 17:24:16, changseok wrote:
> >
>
https://codereview.chromium.org/870013003/diff/20001/Source/core/rendering/Re...
> > > Source/core/rendering/RenderGrid.cpp:1158: }
> > > This code is identical to the code in RenderFlexibleBox. How about making
> this
> > a
> > > method on RenderBlock?
> > > 
> > > I don't have a good name for it, though.
> > 
> > How about addEmptyLineIfNeeded? =)
> 
> sgtm

I was wondering, should the function add the empty line or maybe it would be
more generally useful if it was a const function that just calculated the height
of an empty line and the caller will set it? Could be called emptyLineHeight()
or heightOfEmptyLine()

[changseok, 2015-02-26 06:11:07]

On 2015/02/26 01:12:29, cbiesinger wrote:
> I was wondering, should the function add the empty line or maybe it would be
> more generally useful if it was a const function that just calculated the
height
> of an empty line and the caller will set it? Could be called emptyLineHeight()
> or heightOfEmptyLine()
Hrm.. not sure if I understand correctly. That's what the if (hasLineIfEmpty())
block does?
You might want..
if (hasLineIfEmpty() {
    LayoutUnit emptyLineHeight = emptyLineHeight();
    if (size().height() < minHeight)
        setLogicalHeight(emptyLineHeight);
}
But it looks not much different from original code? :P

[changseok, 2015-02-26 06:17:15]

On 2015/02/26 06:11:07, changseok wrote:
> if (hasLineIfEmpty() {
>     LayoutUnit emptyLineHeight = emptyLineHeight();
>     if (size().height() < minHeight)
if (size().height() < emptyLineHeight) is correct. sorry.

[changseok, 2015-02-26 09:30:25]

>> How about addEmptyLineIfNeeded? =)
>sgtm
I rethought the name of addEmptyLineIfNeeded though, it's not enough good to
explain what it is.
I think keeping hasLineIfEmpty() here is better because it's more readable.
Instread, we can make those lines simpe by adding
minimumLogicalHeightForEmptyLIne.
Please see again.

[rune, 2015-02-26 10:38:50]

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/key-events-in-editable-gridbox.html (right):

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/key-events-in-editable-gridbox.html:3: <head>
html, head, and body should be left out when not necessary.

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/key-events-in-editable-gridbox.html:32: }
You shouldn't need to wait for an onload here? Couldn't this test be run
directly if you move the div#target element above the script?

https://codereview.chromium.org/870013003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/870013003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:115: }
You should now use this method in RenderFlexibleBox::layoutFlexItems as well.

[changseok, 2015-02-26 11:28:54]

Thanks for the comments. Updated patch is coming.

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/key-events-in-editable-gridbox.html (right):

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/key-events-in-editable-gridbox.html:3: <head>
On 2015/02/26 10:38:50, rune wrote:
> html, head, and body should be left out when not necessary.

Done.

https://codereview.chromium.org/870013003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/key-events-in-editable-gridbox.html:32: }
On 2015/02/26 10:38:50, rune wrote:
> You shouldn't need to wait for an onload here? Couldn't this test be run
> directly if you move the div#target element above the script?

You're right. Done.

https://codereview.chromium.org/870013003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/870013003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:115: }
On 2015/02/26 10:38:50, rune wrote:
> You should now use this method in RenderFlexibleBox::layoutFlexItems as well.

O.K

[rune, 2015-02-26 11:42:34]

lgtm if you re-add <!DOCTYPE html> to the test.

[changseok, 2015-02-26 15:30:43]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-02-26 15:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com
Link to the patchset: https://codereview.chromium.org/870013003/#ps80001 (title:
"DoctypeAdded")

[commit-bot: I haz the power, 2015-02-26 15:31:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870013003/80001

[commit-bot: I haz the power, 2015-02-26 15:36:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-26 15:36:06]

Try jobs failed on following builders:
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[changseok, 2015-02-26 16:05:18]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-02-26 16:05:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com
Link to the patchset: https://codereview.chromium.org/870013003/#ps100001
(title: "RebasedPatch")

[commit-bot: I haz the power, 2015-02-26 16:05:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870013003/100001

[commit-bot: I haz the power, 2015-02-26 16:10:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-26 16:10:45]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[changseok, 2015-02-26 16:22:03]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-02-26 16:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com
Link to the patchset: https://codereview.chromium.org/870013003/#ps120001
(title: "ResolveConflict")

[commit-bot: I haz the power, 2015-02-26 16:23:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870013003/120001

[commit-bot: I haz the power, 2015-02-26 17:34:45]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190936