Add parsing support for display: contents, and minimum behavior similar to display: none

Add parsing support for display: contents, and minimum behavior similar to display: none

The following commit adds parsing for the display: contents value, gated after a
runtime enabled feature, and adds the basic code for display: contents elements
to not generate a LayoutObject.

BUG=593328

[svillar, 2016-10-19 17:58:13]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, svillar@igalia.com

[svillar, 2016-10-19 17:58:13]

lgtm, but let's wait for the intent to implement before landing

[jfernandez, 2016-10-19 20:53:07]

jfernandez@igalia.com changed reviewers:
+ jfernandez@igalia.com

[jfernandez, 2016-10-19 20:53:08]

The change looks fine, but perhaps we should add some tests for the parsing
logic, eg. invalid values, how it behave when runtime is not enabled, ...

[eae, 2016-10-19 21:58:38]

LGTM

[rune, 2016-10-19 22:30:35]

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

[rune, 2016-10-19 22:30:35]

This CL needs parser tests.

You need to send an intent to implement, and preferably write up a design
document before starting.

I started drafting an intent + identifying issues in this thread:

https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/nJz270uSkuA

Please read that thread, and reply if you intend to pick up this work.

[rune, 2016-10-19 22:33:43]

https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:191: case
EDisplay::Contents:
This is probably correct, but it would be good to hear how you plan to handle
the layout tree parenthood in LayoutTreeBuilder.

[eae, 2016-10-19 23:25:50]

As rune and jfernandez said please send the intent and respond to comments prior
to landing. Thank you.

[Manuel Rego, 2016-10-20 07:33:30]

So yeah this was just a first CL starting to explore the thing.
And I agree it lacks tests.

@rune our idea was sending a mail to style-dev into that thread
explaining that we're going to try to take a look into this,
as you said you didn't have time to work on that right now.
So if everyone is happy with the plan, Emilio will be working
on this in the next months.

Probably we should sent a intent-to-implement to blink-dev too,
explaining the ideas we've to implement it and reply the different
design questions there.

I hope this clarifies a little bit the whole thing.

[emilio, 2016-10-20 11:11:52]

On 2016/10/19 22:30:35, rune wrote:
> This CL needs parser tests.

Right, I will publish a new version with tests.

> You need to send an intent to implement, and preferably write up a design
> document before starting.
>
> I started drafting an intent + identifying issues in this thread:
> 
> https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/nJz270uSkuA
> 
> Please read that thread, and reply if you intend to pick up this work.

Yes, I read that, thanks for putting that up since it was really helpful :)

I just sent the intent to implement, happy to discuss with you about ideas.

I can try to make a document with my initial implementation plan if you want
(though I expect to iterate over it quite a bit over time).

[emilio, 2016-10-20 11:12:39]

ecobos@igalia.com changed reviewers:
+ ecobos@igalia.com

[emilio, 2016-10-20 11:12:40]

https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:191: case
EDisplay::Contents:
On 2016/10/19 22:33:43, rune wrote:
> This is probably correct, but it would be good to hear how you plan to handle
> the layout tree parenthood in LayoutTreeBuilder.

In my wip I return nullptr here and store the style in the rare data. My idea is
making LayoutTreeBuilderTraversal skip display: content nodes, but that means
also modify the style code to stop using it for the purpose of inheritance.

[rune, 2016-10-21 08:05:49]

On 2016/10/20 11:12:40, emilio wrote:
>
https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:191: case
> EDisplay::Contents:
> On 2016/10/19 22:33:43, rune wrote:
> > This is probably correct, but it would be good to hear how you plan to
handle
> > the layout tree parenthood in LayoutTreeBuilder.
> 
> In my wip I return nullptr here and store the style in the rare data. My idea
is
> making LayoutTreeBuilderTraversal skip display: content nodes, but that means
> also modify the style code to stop using it for the purpose of inheritance.

[rune, 2016-10-21 08:16:51]

On 2016/10/21 08:05:49, rune wrote:
> On 2016/10/20 11:12:40, emilio wrote:
> >
>
https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2439503002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:191: case
> > EDisplay::Contents:
> > On 2016/10/19 22:33:43, rune wrote:
> > > This is probably correct, but it would be good to hear how you plan to
> handle
> > > the layout tree parenthood in LayoutTreeBuilder.
> > 
> > In my wip I return nullptr here and store the style in the rare data. My
idea
> is
> > making LayoutTreeBuilderTraversal skip display: content nodes, but that
means
> > also modify the style code to stop using it for the purpose of inheritance.

As mentioned in the blink-dev thread, I'm not sure that is the right approach.

We need to define what LayoutTreeBuilderTraversal really is. It needs unit tests
to support that. My current mental model, is that it is flat tree traversal
including pseudo elements.

[emilio, 2016-10-21 13:26:03]

On 2016/10/21 08:16:51, rune wrote:
> As mentioned in the blink-dev thread, I'm not sure that is the right approach.
> 
> We need to define what LayoutTreeBuilderTraversal really is. It needs unit
tests
> to support that. My current mental model, is that it is flat tree traversal
> including pseudo elements.

Yep, that makes sense, after some thought I don't think I need to split those so
radically, and it makes sense for LayoutTreeBuilderTraversal to remain as it is.

I added a test to this CL for the parsing of the property, let me know if it
looks good to you.

Thanks!

[rune, 2016-10-21 13:35:35]

lgtm

[ecoal95, 2016-10-21 17:49:29]

The CQ bit was checked by ecoal95@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:50:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 18:46:03]

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

[commit-bot: I haz the power, 2016-10-21 18:46:05]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Manuel Rego, 2016-10-21 22:10:41]

This is just a suggestion but I'd use a different folder
for the tests.

Instead of:
* css-display-3/display-contents/display-contents-set-get.html

Maybe:
* css3/display/display-contents-set-get.html
Or:
* css3/display-contents/display-contents-set-get.html

BTW, regarding the error in the presubmit bot
it's because you're not using your @igalia.com account.
You should upload a new CL using your @igalia.com,
as we've a wildcard in AUTHORS file and a corporate CLA
already in place.

https://codereview.chromium.org/2439503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2439503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:263:
SendBeaconThrowForBlobWithNonSimpleType status=experimental
Nit: This change is not needed.

[Manuel Rego, 2016-10-21 22:15:13]

Last, as we've reported #657748 as tracking bug,
it would be probably better to link it in the description
instead of #593328.

It's also nice to reference the tests in the description with:
TEST=path/to/test.html

[emilio, 2016-10-22 02:16:05]

On 2016/10/21 22:10:41, Manuel Rego wrote:
> This is just a suggestion but I'd use a different folder
> for the tests.
> 
> Instead of:
> * css-display-3/display-contents/display-contents-set-get.html

I used css-display-3/display-contents-set-get.html to keep it
consistent with the CSSWG's convention:

https://github.com/w3c/csswg-test/tree/master/css-display-3 

> BTW, regarding the error in the presubmit bot
> it's because you're not using your @igalia.com account.
> You should upload a new CL using your @igalia.com,
> as we've a wildcard in AUTHORS file and a corporate CLA
> already in place.

https://codereview.chromium.org/2443693003

>
https://codereview.chromium.org/2439503002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:263:
> SendBeaconThrowForBlobWithNonSimpleType status=experimental
> Nit: This change is not needed.

Good call, reverted, though trailing newline is in every other place.

Also added the proper BUG= and TEST= in the new CL.

Sorry for the noise everyone else :)

[Manuel Rego, 2016-10-24 11:12:13]

Description was changed from

==========
Add parsing support for display: contents, and minimum behavior similar to
display: none

The following commit adds parsing for the display: contents value, gated after a
runtime enabled feature, and adds the basic code for display: contents elements
to not generate a LayoutObject.

BUG=593328
==========

to

==========
Add parsing support for display: contents, and minimum behavior similar to
display: none

The following commit adds parsing for the display: contents value, gated after a
runtime enabled feature, and adds the basic code for display: contents elements
to not generate a LayoutObject.

BUG=593328
==========