Return '' for overflow if overflow-x and overflow-y are different.

Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value
[2] https://github.com/w3c/csswg-drafts/issues/1100

BUG=701235
Review-Url: https://codereview.chromium.org/2752623002
Cr-Commit-Position: refs/heads/master@{#461352}
Committed: https://chromium.googlesource.com/chromium/src/+/02c25323de5c3f16bb47d5dc6fc6c7b9a939b784

[shend, 2017-03-14 04:33:03]

shend@chromium.org changed reviewers:
+ nainar@chromium.org

[shend, 2017-03-14 04:33:03]

Hi Naina, PTAL :)

[shend, 2017-03-14 04:33:07]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 04:33:23]

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

[nainar, 2017-03-14 04:49:16]

lgtm

https://codereview.chromium.org/2752623002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html
(right):

https://codereview.chromium.org/2752623002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html:9: },
'Test different overflow x and y returns empty string')
nit Change to: Test that if overflowX != overflowY you are returned an empty
string.

[commit-bot: I haz the power, 2017-03-14 05:53:00]

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

[commit-bot: I haz the power, 2017-03-14 05:53:01]

Dry run: This issue passed the CQ dry run.

[shend, 2017-03-16 00:33:44]

Description was changed from

==========
Return '' for overflow if overflow-x and overflow-y are different.

Currently, when an element has different overflow-x and overflow-y,
the computed value for the overflow shorthand property is the max of
overflow-x and overflow-y in terms of their enum values. This is very
unintuitive and not what Firefox does.

This patch fixes this issue by return an empty string if the two
overflows are different, which follows Firefox's behaviour.

BUG=701235
==========

to

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

BUG=701235
==========

[shend, 2017-03-16 00:33:52]

Description was changed from

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

BUG=701235
==========

to

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value

BUG=701235
==========

[shend, 2017-03-16 00:40:59]

shend@chromium.org changed reviewers:
+ meade@chromium.org

[shend, 2017-03-16 00:40:59]

Hi Eddy, PTAL

https://codereview.chromium.org/2752623002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html
(right):

https://codereview.chromium.org/2752623002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html:9: },
'Test different overflow x and y returns empty string')
On 2017/03/14 at 04:49:16, nainar wrote:
> nit Change to: Test that if overflowX != overflowY you are returned an empty
string.

I changed this to "Overflow is empty string if overflowX != overflowY" so it
follows the same style as some other layout tests.

[shend, 2017-03-17 06:34:38]

Description was changed from

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value

BUG=701235
==========

to

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value
[2] https://github.com/w3c/csswg-drafts/issues/1100

BUG=701235
==========

[meade_UTC10, 2017-03-17 06:37:55]

meade@chromium.org changed reviewers:
+ shans@chromium.org

[meade_UTC10, 2017-03-17 06:37:56]

This seems maybe ok, but I'd like to double check that changing visible
behaviour like this is ok. Added Shane to have a look.

My read of the spec was it should return "visible foo" or "foo visible" instead
of empty string.

[shans, 2017-03-20 07:01:37]

On 2017/03/17 at 06:37:56, meade wrote:
> This seems maybe ok, but I'd like to double check that changing visible
behaviour like this is ok. Added Shane to have a look.
> 
> My read of the spec was it should return "visible foo" or "foo visible"
instead of empty string.

The overflow specification makes it clear that the overflow shorthand can only
contain one value, not two:
https://drafts.csswg.org/css-overflow-3/#propdef-overflow.
There's a "box model" specification which has a different definition but I don't
trust it... particularly given that Firefox's behavior matches what Darren has
implemented I think we're good.

This leaves us with the problem of changing visible behavior - changing the
serialization is relatively lower risk than changing input values. It would be
hard to look for examples where this might break a page. While we could
use-counter etc. to maybe figure out if there's a major impact I don't know if
it's worth it.

You could maybe ping Rick if you were concerned about it?

[meade_UTC10, 2017-03-29 22:49:00]

lgtm

I'm not that concerned if you think it's low risk shane :) Thanks for double
checking.

https://codereview.chromium.org/2752623002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html
(right):

https://codereview.chromium.org/2752623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html:2: <div
id="target" style="overflow-x: hidden; overflow-y: scroll"></div>
nit: Move this down below the testharness includes, but before the testing
script.

[shend, 2017-03-29 23:59:28]

shend@chromium.org changed reviewers:
- shans@chromium.org

[shend, 2017-03-29 23:59:29]

Thanks!

https://codereview.chromium.org/2752623002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html
(right):

https://codereview.chromium.org/2752623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/different-overflow-x-and-y.html:2: <div
id="target" style="overflow-x: hidden; overflow-y: scroll"></div>
On 2017/03/29 at 22:49:00, Eddy wrote:
> nit: Move this down below the testharness includes, but before the testing
script.

Done.

[shend, 2017-03-30 02:43:23]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:43:54]

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, 2017-03-30 04:13:44]

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

[commit-bot: I haz the power, 2017-03-30 04:13:45]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-02 22:28:45]

The CQ bit was checked by shend@chromium.org

[shend, 2017-04-02 22:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org,
meade@chromium.org
Link to the patchset: https://codereview.chromium.org/2752623002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-02 22:28:59]

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, 2017-04-03 00:20:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491172125983050,
"parent_rev": "86396aa4a030f140170a52d24972f90cfd75bce9", "commit_rev":
"02c25323de5c3f16bb47d5dc6fc6c7b9a939b784"}

[commit-bot: I haz the power, 2017-04-03 00:21:44]

Description was changed from

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value
[2] https://github.com/w3c/csswg-drafts/issues/1100

BUG=701235
==========

to

==========
Return '' for overflow if overflow-x and overflow-y are different.

According to the CSSOM spec [1], when overflow-x and overflow-y
are different, the computed value of overflow should be the empty
string since it cannot be expressed in the grammar (step 1.2.).

However, we currently return the max of overflow-x and overflow-y
in terms of their enum values, which does not follow the spec.
Firefox handles this issue correctly.

This patch fixes this issue by return an empty string if the two
overflows are different.

[1] https://drafts.csswg.org/cssom/#serialize-a-css-value
[2] https://github.com/w3c/csswg-drafts/issues/1100

BUG=701235

Review-Url: https://codereview.chromium.org/2752623002
Cr-Commit-Position: refs/heads/master@{#461352}
Committed:
https://chromium.googlesource.com/chromium/src/+/02c25323de5c3f16bb47d5dc6fc6...
==========

[commit-bot: I haz the power, 2017-04-03 00:21:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/02c25323de5c3f16bb47d5dc6fc6...