1422839 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Mats Palmgren (inactive)]

We currently have an internal property 'overflow-clip-box' with
values 'padding-box' and 'content-box' to control how overflow
is clipped.  'padding-box' is the standard CSS2 model and
'content-box' is used for a few form controls where overflowing
into the padding is undesirable, e.g. <input> <textarea> etc.

Bug 752790 shows that single-line text inputs do overflow into
the padding *in the block direction only* in Chrome and some web
sites depends on that.  e.g.
data:text/html,<input style="height:20px;padding:20px">

(Arguably, that's just a bug in Chrome that web sites have now
come to depend upon.)

In this bug I'll add overflow-clip-box-block/-inline properties
and make overflow-clip-box a shorthand for those.  My intention is
then to "fix" bug 752790 by changing the initial value for single-line
form controls to: 'overflow-clip-box: padding-box content-box'.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

Comment 2

[Mats Palmgren (inactive)]

Attachment: Stylo changes

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (automated devtools update)

Comment 4

[Mats Palmgren (inactive)]

There's still one test failure with the above patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3283f4ca9c0720efc5f163eb5c699f6084742c0&selectedJob=149432914
FAIL | /web-animations/animation-model/animation-types/accumulation-per-property.html | overflow-clip-box: "content-box" onto "padding-box"
assert_equals: The value should be content-box at 0ms expected "content-box" but got ""

Does anyone understand what the problem is there?

Comment 5

[Daniel Holbert [:dholbert]]

The failing line is here:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#1482-1485

======
    assert_equals(getComputedStyle(target, type)[idlName],
                  testSample.expected,
                  `The value should be ${testSample.expected}` +
                  ` at ${testSample.time}ms`);
======

And shorthands don't work in getComputedStyle, IIRC.  (they're not present in the hash that getComputedStyle returns)

So I think you just want to remove the overflow-clip-box propery from that property-list.js file:
https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#1057

(I don't see any other shorthands in that file (no "margin", "padding", "background", "border") -- I do see "overflow" but that property looks like it's got empty data there so maybe it's really being skipped effectively.  So, looks like it's correct to remove the property from that list if it's becoming a shorthand. And it's extra-correct given that this is a Mozilla-only (?) nonstandard property, and this is a web-platform test that we share with other vendors.)

Comment 6

[Mats Palmgren (inactive)]

Ah, I thought that file was generated somehow, and I couldn't figure out from what...
I pushed a job to check that animating the longhands works correctly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b41d866a7ccfb8e0a45c4354d5599e3e7173c0c
but you're probably right that we shouldn't include internal Gecko properties in
this list.  Thanks!

Comment 7

[Daniel Holbert [:dholbert]]

Sure! Also, jgraham has told me that we're fine to update web-platform-tests directly in gecko (at least, if they're trivial changes or changes to mozilla-authored code) -- no need to make changes upstream and wait for them to be synchronized or anything like that.

Also, it'd be worth it for somebody to update this MDN page:
 https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box

--> dev-doc-needed

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8934179 [details] [diff] [review]
part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

Review of attachment 8934179 [details] [diff] [review]:
-----------------------------------------------------------------

I'll do this in 2 parts so you can see my first comments while I'm doing the second half.  Part 1:

::: layout/base/PresShell.cpp
@@ +3633,5 @@
>      if (sf) {
>        nsPoint oldPosition = sf->GetScrollPosition();
>        nsRect targetRect = rect;
> +      auto* disp = container->StyleDisplay();
> +      if (disp->mOverflowClipBoxBlock ==

Could you add a comment here to document the purpose of this now-a-bit-more-complex chunk of code?

Something like:
"Inflate scrolled rect by container's padding in each dimension, unless we have "overflow-clip-box-*: content" in that dimension.

@@ +3640,4 @@
>              NS_STYLE_OVERFLOW_CLIP_BOX_CONTENT_BOX) {
> +        WritingMode wm = container->GetWritingMode();
> +        bool clipHorizontally = (wm.IsVertical() ? disp->mOverflowClipBoxBlock
> +                                                 : disp->mOverflowClipBoxInline) ==

This line is too long.  Maybe wrap before the open-paren?  Or s/Horizontally/Horiz/ and s/Vertically/Vert/?

::: layout/generic/nsFrame.cpp
@@ +2458,5 @@
> +        bp.top = bp.bottom = 0;
> +      } else {
> +        bp.left = bp.right = 0;
> +      }
> +    }

Could you collapse away the code-duplication here, by using the same "clipVertically" / "clipHorizontally" strategy that you use in PresShell.cpp and in ScrollFrameHelper::BuildDisplayList?

(nice for avoiding redundancy, and for making the logic more-directly-comparable/consistent between the different chunks of code.)

::: layout/style/nsCSSParser.cpp
@@ +15377,5 @@
> +    return false;
> +  }
> +  AppendValue(eCSSProperty_overflow_clip_box_block, first);
> +  AppendValue(eCSSProperty_overflow_clip_box_inline,
> +              result == CSSParseResult::NotFound ? first : second);

Is there a reason you chose "block, inline" as the ordering? There are a lot of pair-valued properties in CSS that use the opposite ordering (or at least use "horizontal, vertical"), which leads me to wonder if this should flip.

Examples of 'horizontal, vertical' pair-orderings:
 - The 2-value syntax on https://developer.mozilla.org/en-US/docs/Web/CSS/background-repeat (2 keywords, and the first one represents the horizontal value)
 - All point-valued or size-valued properties (obviously)
 - Also, perhaps most-importantly, our internal representation of "overflow" and our code that handles it (e.g. CSSParserImpl::ParseOverflow), which seems to consistently do horizontal stuff before vertical stuff.
 - Perhaps other things - I didn't to a comprehensive search.

In contrast, I can think of two examples of the opposite ordering, but I think they're both "special":
 - The two-value syntax for margin-valued properties (margin & padding) - but those are special IMO because they define a spectrum of behavior from 1 up to 4 values in an attempt at being coherent, and the 2-value syntax is just an intermediate on that spectrum.
 - Grid properties which are typically row,column, where the "row" direction is the block-direction.  Those are also special IMO because there's there's an established "row,column" matrix-arrangement paradigm.

*shrug*.  I'm not necessarily objecting to the current ordering; just want to be sure it's not going to cause confusion.

::: layout/style/test/property_database.js
@@ +7444,5 @@
> +    type: CSS_TYPE_LONGHAND,
> +    applies_to_placeholder: true,
> +    initial_values: [ "padding-box" ],
> +    other_values: [ "content-box" ],
> +    invalid_values: [ "none", "auto", "border-box", "0" ]

Please include a double-value in each of the longhand properties' invalid_values arrays.

e.g. "content-box padding-box" (which is valid for the shorthand but not for the longhand)

@@ +7453,5 @@
> +    type: CSS_TYPE_TRUE_SHORTHAND,
> +    subproperties: [ "overflow-clip-box-block", "overflow-clip-box-inline" ],
> +    initial_values: [ "padding-box" ],
> +    other_values: [ "content-box", "padding-box content-box", "content-box padding-box" ],
> +    invalid_values: [ "none", "auto", "content-box none", "border-box", "0" ]

Can you include an entry with the same value repeated-twice (e.g. "content-box content-box"), in other_values or invalid_values depending on whether that extra-verbose syntax is allowed?  That seems like an edge case that'd be worth testing.

(I'm assuming it's allowed.)

Also: please include an invalid_value with a comma separator (but which is otherwise valid), to enforce that we only allow a space-separated list.

Comment 9

[Daniel Holbert [:dholbert]]

RE ordering -- the most-similar property I've found seems to be "overscroll-behavior" (shorthand for "overscroll-behavior-x" / "overscroll-behavior-y", which can optionally take both keywords, and which I think is also an internal-only property).

There, the horizontal value is parsed first:
https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/layout/style/nsCSSParser.cpp#17503

Since both that property and this new property are overflow/scrollframe-related (and start out with "over***", it seems confusing to me that their syntaxes would have *opposite* orderings by default (in the default writing-mode).

It's possible that there's no strong consistency around this and so it doesn't matter.  But to the extent that we can find consistency in a particular direction for similar concepts/properties, it seems like we should follow it...

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8934179 [details] [diff] [review]
part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

Review of attachment 8934179 [details] [diff] [review]:
-----------------------------------------------------------------

Got through the rest of the first patch, and I don't actually have any more review-comments beyond comment 8-9. :) sorry for the bait-and-switch on 2-part review feedback there.

I'll mark it r- for now since it might merit another round of review, depending on your thoughts on my review feedback.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8934182 [details] [diff] [review]
part 3 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (automated devtools update)

Review of attachment 8934182 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on automated devtools patch

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

(In reply to Daniel Holbert [:dholbert] from comment #8)
>::: layout/base/PresShell.cpp
>Could you add a comment here to document the purpose of this now-a-bit-more-complex chunk of code?

Fixed.

>This line is too long.

Fixed.

>::: layout/generic/nsFrame.cpp
>Could you collapse away the code-duplication here, by using the same "clipVertically" / "clipHorizontally" strategy that you use in PresShell.cpp and in ScrollFrameHelper::BuildDisplayList?

Sure, fixed.

>::: layout/style/nsCSSParser.cpp
>Is there a reason you chose "block, inline" as the ordering?

I'm pretty sure block axis first is the preferred order for new properties,
per CSSWG discussion.  (recent examples: grid-gap, place-self etc)
(it was a somewhat recent discussion, like in the last year or so,
but I couldn't find a link, fantasai probably knows)

(I suspect that the new -x/-y physical properties you mention are in
that order for legacy/consistency reasons.)

>::: layout/style/test/property_database.js
>Please include a double-value in each of the longhand properties' invalid_values arrays.

Fixed.

>Can you include an entry with the same value repeated-twice (e.g. "content-box content-box")

Fixed.

>(I'm assuming it's allowed.)

Correct.

>Also: please include an invalid_value with a comma separator

Good idea, fixed.

(I've also removed the shorthand from WPT in this patch)

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 4 - Reftest for 'overflow-clip-box' properties

https://treeherder.mozilla.org/#/jobs?repo=try&revision=229ecfe7de179ba9b61b0ac172075fe331b72fc9

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8934346 [details] [diff] [review]
part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand

Review of attachment 8934346 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mats Palmgren (:mats) from comment #12)
> I'm pretty sure block axis first is the preferred order for new properties,
> per CSSWG discussion.  (recent examples: grid-gap, place-self etc)
> (it was a somewhat recent discussion, like in the last year or so,
> but I couldn't find a link, fantasai probably knows)
> 
> (I suspect that the new -x/-y physical properties you mention are in
> that order for legacy/consistency reasons.)

Fair enough, OK.

I skimmed the interdiff and it looks good to me! r=me

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8934181 [details] [diff] [review]
Stylo changes

Review of attachment 8934181 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleConsts.h
@@ +715,5 @@
>  
>  // See nsStyleDisplay.mOverflowClipBox
>  #define NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX  0
>  #define NS_STYLE_OVERFLOW_CLIP_BOX_CONTENT_BOX  1
> +#define NS_STYLE_OVERFLOW_CLIP_BOX_BLOCK_PADDING_BOX  NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX

There's no need for this, you can use `gecko_enum_prefix` on the relevant declarations in box.mako.rs

::: servo/components/style/properties/shorthand/box.mako.rs
@@ +63,5 @@
> +                    spec="Internal, not web-exposed, may be standardized in the future (https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box)"
> +                    products="gecko">
> +  use properties::longhands::{overflow_clip_box_block, overflow_clip_box_inline};
> +
> +  pub fn to_inline_value(block_value: overflow_clip_box_block::SpecifiedValue)

This looks fine, but it'd be nicer if both longhands reused the same type, you can look at overscroll-behavior, for an example.

Feel free to land this and I'll fix it up if you want though.

Comment 16

[Mats Palmgren (inactive)]

FTR, I found the link regarding the preferred order of block/inline values:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0267.html
"
  - RESOLVED: Make logical coordinates always block, then inline,
              even though physical coordinates in background-
              position are horizontal, vertical.
"

I get the impression that they also felt rather strongly about it
since a number of Grid properties were interoperably implemented
using inline-then-block at that point, but they still changed it.
(we changed those in bug 1240956)

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 2 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (Stylo changes)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> > +#define NS_STYLE_OVERFLOW_CLIP_BOX_BLOCK_PADDING_BOX  NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX
> 
> There's no need for this, you can use `gecko_enum_prefix` on the relevant
> declarations in box.mako.rs

I think you meant 'gecko_constant_prefix'...
I added gecko_constant_prefix="NS_STYLE_OVERFLOW_CLIP_BOX" on both
the longhands and it seems to work.  Thanks.

> This looks fine, but it'd be nicer if both longhands reused the same type,
> you can look at overscroll-behavior, for an example.

I tried that initially, using the nearby overflow-x/y as a template,
but it produced compilation errors so I gave up.
This mako format is still pretty opaque to me... :(

> Feel free to land this and I'll fix it up if you want though.

Yeah, I'll happily take that option... :-)

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8934347 [details] [diff] [review]
part 4 - Reftest for 'overflow-clip-box' properties

r=me

Comment 19

[Mats Palmgren (inactive)]

Green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=229ecfe7de179ba9b61b0ac172075fe331b72fc9

Since this has stylo changes I prefer someone else to land it.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/436d10234366
part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/83101ef21e45
part 2 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (Stylo changes).  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc039432e4a6
part 3 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (automated devtools update).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9d1a10cb0b
part 4 - Reftest for 'overflow-clip-box' properties. r=test-only

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because stylo changes have to land in the servo repository first:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3942ce38b1f62fbc3154e42281d31cbed1e81ce0

Comment 22

[Daniel Holbert [:dholbert]]

(emilio says in #layout he'll take care of getting the servo pieces landed.)

Comment 23

[Mats Palmgren (inactive)]

Yes, I pointed that out in comment 19.

Comment 24

[Emilio Cobos Álvarez [:emilio]]

https://github.com/servo/servo/pull/19497 are the Servo changes.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf93ccd03ca7
part 1 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand.  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/5e83d64761ed
part 3 - Add internal overflow-clip-box-block/-inline properties and make overflow-clip-box a shorthand (automated devtools update).  r=dholbert
https://hg.mozilla.org/integration/autoland/rev/18737f466667
part 4 - Reftest for 'overflow-clip-box' properties. r=mats

Comment 26

[Mats Palmgren (inactive)]

Thanks Emilio!

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/436d10234366
https://hg.mozilla.org/mozilla-central/rev/83101ef21e45
https://hg.mozilla.org/mozilla-central/rev/fc039432e4a6
https://hg.mozilla.org/mozilla-central/rev/ff9d1a10cb0b

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/a928be5dacc3
Backed out 4 changesets for landing stylo changes on inbound r=backout a=backout

Comment 29

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cf93ccd03ca7
https://hg.mozilla.org/mozilla-central/rev/5e83d64761ed
https://hg.mozilla.org/mozilla-central/rev/18737f466667

Comment 30

[Chris Mills [:cmills]]

I've documented these:

Created new pages for the new properties:
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box-block
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box-inline

Updated the existing overflow-clip-box page to mention shorthand status, multiple values, etc.
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box

Send a PR into our data repo to generate the formal syntax blocks that are currently missing off the new pages:
https://github.com/mdn/data/pull/165

Added theses properties to the Mozilla CSS extensions page:
https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Mozilla-only_properties_and_pseudo-classes_(avoid_using_on_websites)

Let me know if these look OK. Thanks!

Comment 31

[Mats Palmgren (inactive)]

LGTM, thanks.

Comment 32

[Jean-Yves Perrier [:teoli]]

Chris, Mats,

Should we document these 2 properties? They are UA-only and not web facing I think. They may go to the Mozilla/ part of MDN but there shouldn't be under Web/ if they can't be used be web devs.

Have I understood correctly these properties?

Comment 33

[Mats Palmgren (inactive)]

> Should we document these 2 properties?

They were documented already, see comment 30.

> They may go to the Mozilla/ part of MDN but there shouldn't be under Web/ if they can't be used be web devs.

Moving them from Web/ to Mozilla/ seems reasonable since they are UA-only.

Comment 34

[Jean-Yves Perrier [:teoli]]

Thanks Mats,

Chris, can you move them under Mozilla/ and remove the compatibility table (the PR has been closed by Florian, as it is not web-facing).

Thank you.

Comment 35

[Chris Mills [:cmills]]

(In reply to Jean-Yves Perrier [:teoli] from comment #34)
> Thanks Mats,
> 
> Chris, can you move them under Mozilla/ and remove the compatibility table
> (the PR has been closed by Florian, as it is not web-facing).
> 
> Thank you.

I've moved these properties to

https://developer.mozilla.org/en-US/docs/Mozilla/CSS

Does that work for you?