Make flexibleBox visible position cadidate only where it is editable.

Make flexibleBox visible position cadidate only where it is editable.

FlexibleBox has been a candidate for VisiblePosition after r189482. But it was enough
only when it has editable style. Why we needed r189482 was flexibleBox could not be focusable
when it became an empty box in desing mode since it was not a candidate for VisiblePosition.
We gave it too much power. It causes useless selection region while dragging even though
it was not in desing mode. Unwanted selection region could be watched conditionally,
this is because font rendering depends on platforms. Unfortunately Mac paints font
relatively smaller than others do. I suspect a floating point truncation error though,
we don't need to go too far.
In anycase we can avoid the unwanted selection region radically with this way.

BUG=468745

[changseok, 2015-04-02 16:27:10]

shivamidow@gmail.com changed reviewers:
+ eae@chromium.org, pdr@chromium.org

[changseok, 2015-04-02 16:27:11]

PTAL

[pdr., 2015-04-03 03:21:24]

pdr@chromium.org changed reviewers:
+ cbiesinger@chromium.org, leviw@chromium.org
- pdr@chromium.org

[pdr., 2015-04-03 03:21:44]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2015-04-03 03:21:44]

+cc leviw and cbiesinger

[cbiesinger, 2015-04-03 17:58:41]

Does't this apply to the grid part of the condition as well?

[changseok, 2015-04-03 18:20:06]

On 2015/04/03 17:58:41, cbiesinger wrote:
> Does't this apply to the grid part of the condition as well?

I thought like you. But I could not see such regression for grid. Even I could
not create a test case for that.
As my testing, grid also passes LayoutFlexisbleBox into Position::isCandidate as
well while dragging. So the change for LayoutFlexibleBox also covered the
LayoutGrid case.
Thus.. no. we don't now.
 
If you concern that, let me know. 
I also think adding the editable check for grid is more reasonable and it would
not harmful.

[cbiesinger, 2015-04-03 18:27:04]

On 2015/04/03 18:20:06, changseok wrote:
> As my testing, grid also passes LayoutFlexisbleBox into Position::isCandidate
as
> well while dragging.

Hmm that seems weird? Grid does not use LayoutFlexibleBox...

[changseok, 2015-04-03 19:01:52]

On 2015/04/03 18:27:04, cbiesinger wrote:
> On 2015/04/03 18:20:06, changseok wrote:
> > As my testing, grid also passes LayoutFlexisbleBox into
Position::isCandidate
> as
> > well while dragging.
> 
> Hmm that seems weird? Grid does not use LayoutFlexibleBox...

Yeah, but it's true. Here is my fiddle. You can try.
http://jsfiddle.net/2u6hvt51/7/
What I can guess is floating point related something. 
This issue is involved with many layout elements. button, font size, zoom etc so
that it's quite hard to reproduce the bug rather than fixing it. :P

[pdr., 2015-04-06 18:25:47]

This bug is currently marked as releaseblockstable and P1. WDYT about rolling
out the original patch while we work on this review?

https://codereview.chromium.org/1057953002/diff/20001/Source/core/dom/Positio...
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/1057953002/diff/20001/Source/core/dom/Positio...
Source/core/dom/Position.cpp:894: if (renderer->isLayoutBlockFlow() ||
(renderer->isFlexibleBox() && m_anchorNode->hasEditableStyle()) ||
renderer->isLayoutGrid()) {
I'm not an expert in this area but this fix looks too local. Why don't we need
the hasEditableStyle check for grid or layoutblockflow?

[changseok, 2015-04-07 06:24:41]

I got where the FlexibleBox came from. It's LayoutButton which is a subclass of
LayoutFlexibleBox.
So dragging over button causes isFlexibleBox is true even though the button is
placed in grid.
I think grid itself can not be a candidate for selection if not in design mode
as well as flexiblebox.

On 2015/04/06 18:25:47, pdr wrote:
>
https://codereview.chromium.org/1057953002/diff/20001/Source/core/dom/Positio...
> Source/core/dom/Position.cpp:894: if (renderer->isLayoutBlockFlow() ||
> (renderer->isFlexibleBox() && m_anchorNode->hasEditableStyle()) ||
> renderer->isLayoutGrid()) {
> I'm not an expert in this area but this fix looks too local. Why don't we need
> the hasEditableStyle check for grid or layoutblockflow?

Because we haven't faced such issues for LayoutBlockFlow and LayoutGrid =)
I think adding hasEditableStyle() for LayoutBlockFlow and LayoutGrid is
reasonable as well, but no issue happened related with them and also I could not
create test cases to prove such regressions.
In my opinion, we have two options to choose.
1. Fix this bug for LayoutFlexibleBox only. And let LayoutBlockFlow and
LayoutGrid go until we see a bug regarding them.
2. Add hasEditableStyle to LayoutBlockFlow and LayoutGrid cases as well without
test cases for them.

BTW are the cases for LayoutBlockFlow and LayoutGrid out of scope of this bug?

[leviw_travelin_and_unemployed, 2015-04-07 17:19:52]

On 2015/04/06 at 18:25:47, pdr wrote:
> This bug is currently marked as releaseblockstable and P1. WDYT about rolling
out the original patch while we work on this review?

FYI, it looks like a revert of the original issue was landed:
https://codereview.chromium.org/1060113002/

[changseok, 2015-04-07 17:41:25]

On 2015/04/07 17:19:52, leviw wrote:
> On 2015/04/06 at 18:25:47, pdr wrote:
> > This bug is currently marked as releaseblockstable and P1. WDYT about
rolling
> out the original patch while we work on this review?
> 
> FYI, it looks like a revert of the original issue was landed:
> https://codereview.chromium.org/1060113002/

Yeah I know. As noted in 1060113002, we can re-land it when we're ready here.

[leviw_travelin_and_unemployed, 2015-04-07 17:48:22]

FWIW: it seems to me that the only case where we'd want to allow FlexBox and
Grid to be a candidate is when they're empty (no children with height),
editable, and have height. Otherwise the position should be rooted inside one of
their flex items or their descendants.

What does it mean to have a position between two flex items? If the user adds
content there, what do you get?

[changseok, 2015-04-07 18:42:33]

On 2015/04/07 17:48:22, leviw wrote:
> What does it mean to have a position between two flex items? If the user adds
> content there, what do you get?

Maybe this is what you want? https://jsfiddle.net/j8tyba0m/5/

[leviw_travelin_and_unemployed, 2015-04-07 20:24:30]

On 2015/04/07 at 18:42:33, shivamidow wrote:
> On 2015/04/07 17:48:22, leviw wrote:
> > What does it mean to have a position between two flex items? If the user
adds
> > content there, what do you get?
> 
> Maybe this is what you want? https://jsfiddle.net/j8tyba0m/5/

That's not exactly what I mean. Internally, Flexbox and Grid coerce their
children into flex items. In your test case, we'll wrap your text in the middle
in an anonymous block that acts as that flex item.

I specifically mean if you have this:

<div "display: flex">
   <div>foo</div>
   <div>bar</div>
</div>

And you programmatically place the selection between the two children of that
flex box, there's a question of whether or not we:
A) append the content to 'foo'
B) prepend the content to 'bar'
C) create a text node in the middle and wrap it in an anonymous block

What do other browsers do?

[changseok, 2015-04-08 09:26:12]

On 2015/04/07 20:24:30, leviw wrote:
> On 2015/04/07 at 18:42:33, shivamidow wrote:
> > On 2015/04/07 17:48:22, leviw wrote:
> > > What does it mean to have a position between two flex items? If the user
> adds
> > > content there, what do you get?
> > 
> > Maybe this is what you want? https://jsfiddle.net/j8tyba0m/5/
> 
> That's not exactly what I mean. Internally, Flexbox and Grid coerce their
> children into flex items. In your test case, we'll wrap your text in the
middle
> in an anonymous block that acts as that flex item.
> 
> I specifically mean if you have this:
> 
> <div "display: flex">
>    <div>foo</div>
>    <div>bar</div>
> </div>
> 
> And you programmatically place the selection between the two children of that
> flex box, there's a question of whether or not we:
> A) append the content to 'foo'
> B) prepend the content to 'bar'
> C) create a text node in the middle and wrap it in an anonymous block
> 
> What do other browsers do?

Not sure of I wrote a test as you said. see https://jsfiddle.net/s6f4fkf3/4/
There is difference of handling focus between gecko and blink/webkit, I could
not compare browsers with a single code path.
In short, B for blink/webkit. And C for gecko. (I don't know how IE works since
no window machine I have)

BTW is this related with this bug?

[changseok, 2015-04-08 16:08:08]

We can not leave this behind. crbug.com/450617 which was fixed by the reverted
patch happens again.
So.. what is owners' thought?
IMHO, we don't need to consider too many things here. 
The issue is that unwanted blue line happens after the reverted patch
https://codereview.chromium.org/1060113002/
and it can be fixed by my last proposed patch.
grid, text insert and other layout stuff are all out of range of this bug. We
can fix them later if they really cause any problem.

[leviw_travelin_and_unemployed, 2015-04-08 17:52:52]

On 2015/04/08 at 09:26:12, shivamidow wrote:
> On 2015/04/07 20:24:30, leviw wrote:
> > On 2015/04/07 at 18:42:33, shivamidow wrote:
> > > On 2015/04/07 17:48:22, leviw wrote:
> > > > What does it mean to have a position between two flex items? If the user
> > adds
> > > > content there, what do you get?
> > > 
> > > Maybe this is what you want? https://jsfiddle.net/j8tyba0m/5/
> > 
> > That's not exactly what I mean. Internally, Flexbox and Grid coerce their
> > children into flex items. In your test case, we'll wrap your text in the
middle
> > in an anonymous block that acts as that flex item.
> > 
> > I specifically mean if you have this:
> > 
> > <div "display: flex">
> >    <div>foo</div>
> >    <div>bar</div>
> > </div>
> > 
> > And you programmatically place the selection between the two children of
that
> > flex box, there's a question of whether or not we:
> > A) append the content to 'foo'
> > B) prepend the content to 'bar'
> > C) create a text node in the middle and wrap it in an anonymous block
> > 
> > What do other browsers do?
> 
> Not sure of I wrote a test as you said. see https://jsfiddle.net/s6f4fkf3/4/
> There is difference of handling focus between gecko and blink/webkit, I could
not compare browsers with a single code path.
> In short, B for blink/webkit. And C for gecko. (I don't know how IE works
since no window machine I have)
> 
> BTW is this related with this bug?

You wrote the test, and it shows different results in Blink vs Firefox, which
doesn't surprise me. Honestly, it suggests we should just always allow a
position inside a Flexbox to be valid, as their test is closer to what an Author
would expect.

I'll be happy to chat with anyone who disagrees, but my opinion is that the
"unwanted" blue line was fine and correct. It was the result of the block
selection gaps being painted after the padding between the text and the box grew
to a full pixel.

[pdr., 2015-04-08 20:34:12]

On 2015/04/08 at 17:52:52, leviw wrote:
> I'll be happy to chat with anyone who disagrees, but my opinion is that the
"unwanted" blue line was fine and correct. It was the result of the block
selection gaps being painted after the padding between the text and the box grew
to a full pixel.

This sounds reasonable to me too. Thanks for looking into it. Lets reland the
original patch after all.

[leviw_travelin_and_unemployed, 2015-04-08 20:38:04]

On 2015/04/08 at 20:34:12, pdr wrote:
> On 2015/04/08 at 17:52:52, leviw wrote:
> > I'll be happy to chat with anyone who disagrees, but my opinion is that the
"unwanted" blue line was fine and correct. It was the result of the block
selection gaps being painted after the padding between the text and the box grew
to a full pixel.
> 
> This sounds reasonable to me too. Thanks for looking into it. Lets reland the
original patch after all.

Thanks for your $0.02, PDR :)

Sorry for the churn shivamidow!

[changseok, 2015-04-09 04:18:19]

On 2015/04/08 20:38:04, leviw wrote:
> On 2015/04/08 at 20:34:12, pdr wrote:
> > On 2015/04/08 at 17:52:52, leviw wrote:
> > > I'll be happy to chat with anyone who disagrees, but my opinion is that
the
> "unwanted" blue line was fine and correct. It was the result of the block
> selection gaps being painted after the padding between the text and the box
grew
> to a full pixel.
Exactly. I agree with you. 
The blue line issue was introduced by my cl by chance though, strictly speaking,
my cl didn't make the regression.
  
> > 
> > This sounds reasonable to me too. Thanks for looking into it. Lets reland
the
> original patch after all.

I'm not sure if I understand correctly here.
In other words.. we reland the reverted patch, but we won't fix the blue line?

> Thanks for your $0.02, PDR :)
> 
> Sorry for the churn shivamidow!

No problem. =)

[leviw_travelin_and_unemployed, 2015-04-09 23:40:33]

On 2015/04/09 at 04:18:19, shivamidow wrote:
> On 2015/04/08 20:38:04, leviw wrote:
> > On 2015/04/08 at 20:34:12, pdr wrote:
> > > On 2015/04/08 at 17:52:52, leviw wrote:
> > > > I'll be happy to chat with anyone who disagrees, but my opinion is that
the
> > "unwanted" blue line was fine and correct. It was the result of the block
> > selection gaps being painted after the padding between the text and the box
grew
> > to a full pixel.
> Exactly. I agree with you. 
> The blue line issue was introduced by my cl by chance though, strictly
speaking, my cl didn't make the regression.
>   
> > > 
> > > This sounds reasonable to me too. Thanks for looking into it. Lets reland
the
> > original patch after all.
> 
> I'm not sure if I understand correctly here.
> In other words.. we reland the reverted patch, but we won't fix the blue line?

That's my suggestion. I'm saying while it's hard to tell in the specific case
the tester identified as a regression, your change could be considered a
*progression*.

> 
> > Thanks for your $0.02, PDR :)
> > 
> > Sorry for the churn shivamidow!
> 
> No problem. =)