This bug will implement -moz-overflow-clip-box:content-box for specifying an alternative clip area to the padding-box mandated in 11.1: http://www.w3.org/TR/CSS21/visufx.html#overflow-clipping It's specifically designed to match the layout of <input type=text> and <textarea> with a specified padding, which cannot be described in CSS terms currently. (This bug will only implement the property; bug 965237 will change the UA sheet to use -moz-overflow-clip-box:content-box for the form controls that need it.)

This is just early WIP patches. The style system part is probably complete; the layout part still has some bugs. I just wanted to give you a chance for early feedback if you have any...

I'm enabling overflow-clip-box when mUnsafeRulesEnabled is true (with the intent of using overflow-clip-box:content-box in the UA sheet for <input type=text/password/number> and <textarea> to fix bug 965237) or when the pref "layout.css.overflow-clip-box.enabled" is true (off by default).

A couple of notes; when clipping to the content-box in BuildDisplayList (the added code) I only clip if there is any scrollable overflow. This is so that any *visual* overflow isn't clipped in that case. We have a few tests that depend on that, e.g. <input> with text-shadow or italic font-style. It seems worth preserving that behavior. Also, a special tweak to make the caret visible when at the very edge of the clip area (actually rendered outside). I added a few "XXX border-radius?" comments but I think we can safely punt on those since we don't do that in general (I filed bug 964222 on that).

I think we should consider proposing this property on www-style, for a few reasons: * It makes it possible to describe the rendering of padding for <input> and <textarea> in CSS terms, which isn't possible currently. * I think it would be useful for blocks in general, to be able to add padding that stays fixed inside the scroll port as you scroll. * It requires minimal changes to the CSS spec: the only thing that needs a change is 11.1, s/padding edge/clipping edge according to overflow-clip-box/ http://www.w3.org/TR/CSS21/visufx.html#overflow-clipping (I guess the spec where we would add it is css-overflow?) (N.B. the CSS Box Model is respected in this layout, unlike the proposal by Chrome engineers on www-style recently, and it might help the CSSWG to reject that proposal.)

Comment on attachment 8371408 [details] [diff] [review] part 2. Implement layout for the overflow-clip-box property. Review of attachment 8371408 [details] [diff] [review]: ----------------------------------------------------------------- Don't we have to modify the scroll range calculation to ensure that we can scroll far enough to bring all content into view inside the content-box? ::: layout/generic/nsGfxScrollFrame.cpp @@ +2417,5 @@ > + } > + } > + } > + clip.Deflate(padding); > + // XXX border-radius? I think it's not a good idea for the caret position to affect the clipping of other content. Can we make the content-box clipping an additional clipping pass that runs over all the display items and updates their clip state, skipping the caret? So we would clip all descendants to both the padding-box and the content-box.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Don't we have to modify the scroll range calculation to ensure that we can > scroll far enough to bring all content into view inside the content-box? I'm inflating the scrolled frame overflow with the right amount to get the content inside the padding, so we get that for free. > Can we make the content-box clipping an additional clipping pass that runs > over all the display items and updates their clip state, skipping the caret? Sorry, I don't understand, can you give me some pointers on how to do that? > So we would clip all descendants to both the padding-box and the content-box. I'm assuming I'm missing something here, because that doesn't make sense to me. If something is clipped to the content-box then it's pointless to clip it to the padding-box since the padding-box contains the content-box.

BTW, try builds are available if you want to test something: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-dcdc7b59b70e/

I'd be happy to change the clipping for the caret as you suggested, if you can point me at the code I should change.

(In reply to Mats Palmgren (:mats) from comment #7) > I'm assuming I'm missing something here, because that doesn't make sense > to me. If something is clipped to the content-box then it's pointless to > clip it to the padding-box since the padding-box contains the content-box. We need to clip the caret to the padding-box and the rest of the content to the content-box. The main question is what to set clipState to when we build the display list for the scrolled frame. If we clip everything to the content-box, it's going to be difficult to compute the correct clip for the nsDisplayCaret item after we've built the display list, because we won't be able to determine which part of its clip is due to content-box clipping and which part is due to other clipping imposed by descendants of the scrolled frame. So I think we have to set clipState to clip to the padding-box (as currently). So then, set the clipState to the padding-box as now, build the display list for the scrolled frame, then in overflow-clip-box:content-box situations, traverse the scrolledContent display lists, and for everything that's clipped by this scrollframe *and* not the caret, intersect its clip with the content-box. Watch out for display items that change their coordinate system ... in that case you can just stop trying to clip descendants, because you'll have clipped the parent anyway.

OK, thanks for elaborating. So I've implemented a first cut of what you described and it seems to work as intended. However, there are a couple of cases where it doesn't work so well. First, when there is a scrollbar and you use it to scroll the caret out-of-view then it still displays while over the padding (as intended), but it looks weird when the padding is more than a few pixels (see screenshot). Second, when using keyboard scrolling there appears to be some delay between the scrolling code and painting that makes the caret flicker over the padding area, even for single-line text inputs. So I think we should consider limiting how far into the padding we allow the caret to be. Maybe require its nearest edge to be just outside the content-box, or 1px away or something like that? (I'll experiment and report back, let me know if you have any input on that).

So the "everything that's clipped by this scrollframe" bit - is there a simple test for that? I guess I should skip abs.pos. frames that have a containing block that is an ancestor of the scroll frame, but I suspect it's more complicated than that...

Back in 2001, the CSS WG agreed to add an 'overflow-clip' property that did what 'clip' did in CSS 2.0 (which is not what browsers had implemented prior to CSS 2.0). In CSS 2.0, the 'clip' property was supposed to change the region that the 'overflow' property applied to -- to arbitrary rectangles. I wonder whether whether it makes more sense to use something more general, like that, but allow it to take 'content-box', 'padding-box', and 'border-box' keywords at the end of its value as a modifier. (There was also discussion about offset-rect() in addition to rect(), to represent CSS 2.0's rect() function.) The best documentation of this photo is unfortunately this photograph of the whiteboard: http://people.opera.com/howcome/2001/i/dec-sf/103-0328_IMG.JPG at the working group's December 2001 meeting in Redwood City, California. (In reply to Mats Palmgren (:mats) from comment #5) > (N.B. the CSS Box Model is respected in this layout, unlike the proposal by > Chrome engineers on www-style recently, and it might help the CSSWG to reject > that proposal.) Do you have a URL for this proposal?

... and also http://people.opera.com/howcome/2001/i/dec-sf/103-0327_IMG.JPG

...and the minutes at https://lists.w3.org/Archives/Member/w3c-css-wg/2001OctDec/0264.html

The "just outside" caret test fixed both problems mentioned in comment 11. I added a heuristic to avoid clipping the caret in the zero-height line box case also. I'm not sure if ShouldBeClippedByFrame() is what you had in mind though... or if it's even correct... https://tbpl.mozilla.org/?tree=Try&rev=9f787bb38529

Comment on attachment 8373807 [details] [diff] [review] part 2. Implement layout for the overflow-clip-box property. I see that I propagated 'mClipAllDescendants' but then never used it...

Comment on attachment 8373807 [details] [diff] [review] part 2. Implement layout for the overflow-clip-box property. Review of attachment 8373807 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +2291,5 @@ > +{ > + return aClipFrame != aClippedFrame && > + !((aClippedFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) && > + aClippedFrame->GetParent() != aClipFrame && > + !nsLayoutUtils::IsProperAncestorFrame(aClipFrame, aClippedFrame->GetParent())); Can't we just check nsLayoutUtils::IsProperAncestorFrame(scrolledFrame, aClippedFrame)? @@ +2310,5 @@ > + nsRect bounds = i->GetBounds(aBuilder, &unused); > + bool isAffectedByClip = aClip.IsRectAffectedByClip(bounds); > + if (isAffectedByClip && nsDisplayItem::TYPE_CARET == i->GetType()) { > + // Don't clip the caret if it's just outside on the right side. > + nsRect rightSide(bounds.x - 1, bounds.YMost() - 1, 1, 1); why not bounds.x - 1, bounds.y, 1, bounds.height? Seems a little less arbitrary. @@ +2323,5 @@ > + if (currentClip.HasClip()) { > + DisplayItemClip newClip; > + newClip.IntersectWith(currentClip); > + newClip.IntersectWith(aClip); > + i->SetClip(aBuilder, newClip); We may as well just do this block unconditionally. I don't think currentClip.HasClip() does a worthwhile optimization. @@ +2474,5 @@ > > mOuter->BuildDisplayListForChild(aBuilder, mScrolledFrame, dirtyRect, scrolledContent); > } > > + if (!usingDisplayport && aBuilder->GetCaret() && Hmm, if there's a displayport or no caret, then we don't clip to the content-box at all? That seems wrong!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) Thanks, I've addressed those points, except one: > why not bounds.x - 1, bounds.y, 1, bounds.height? Seems a little less > arbitrary. For small 'line-height's, the caret overflows vertically into the top padding. I think we should try to avoid clipping it in this case and the suggested y/height fails to do that. I've expanded the caret heuristics a bit and added comments for each check.

Reftest + mochitests for testing caret rendering. https://tbpl.mozilla.org/?tree=Try&rev=1de5f2f4bdf2

Comment on attachment 8375780 [details] [diff] [review] part 3. tests Review of attachment 8375780 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/bug966992-1-ref.html @@ +6,5 @@ > + html,body { > + color:black; background-color:white; font:16px monospace; padding:0; margin:7px; > + } > +.block { > + border:1px solid grey; height:50px; width:200px; padding:20px; trailing whitespace

Comment on attachment 8371394 [details] [diff] [review] part 1. Implement the overflow-clip-box property in the style system. Review of attachment 8371394 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSKeywordList.h @@ +124,5 @@ > CSS_KEY(-moz-nativehyperlinktext, _moz_nativehyperlinktext) > CSS_KEY(-moz-none, _moz_none) > CSS_KEY(-moz-oddtreerow, _moz_oddtreerow) > CSS_KEY(-moz-oriya, _moz_oriya) > +CSS_KEY(overflow-clip-box, overflow_clip_box) Put this later in the file in its sorted position. ::: layout/style/nsCSSPropList.h @@ +2476,5 @@ > + // property when mUnsafeRulesEnabled or the preference > + // layout.css.overflow-clip-box.enabled is set to true. > + CSS_PROPERTY_PARSE_VALUE | > + CSS_PROPERTY_APPLIES_TO_PLACEHOLDER, > + "", The use of CSS_PROP_LIST_EXCLUDE_INTERNAL above will cause the property to be hidden from CSSStyleDeclaration objects, even if the pref is turned on. What about putting the pref in the string here, and adding a flag like CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS, which you can then check in nsCSSParser::ParseProperty to avoid returning early when finding the pref is off? Then remove the change to ParseSingleValueProperty.

(In reply to Cameron McCormack (:heycam) from comment #23) > > +CSS_KEY(overflow-clip-box, overflow_clip_box) > > Put this later in the file in its sorted position. Actually, that's the property name so I don't know why I added it in this list in the first place. I've removed it. > What about putting the pref in the string here, and adding a flag like > CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS, which you can then check in > nsCSSParser::ParseProperty to avoid returning early when finding the pref is > off? Then remove the change to ParseSingleValueProperty. That's a great idea, only it's a bit more complicated than that... The nsCSSProps::IsEnabled call in ParseProperty is just one of many enabled-checks, and most of them don't through here. The important ones are the nsCSSProps::LookupProperty(prop,eEnabled) from the parser. I've fixed those by adding a new EnabledState (eEnabledOrUA) that checks the new bit, and added a convenience method in the parser that does: LookupProperty(aProperty, mUnsafeRulesEnabled ? nsCSSProps::eEnabledOrUA : nsCSSProps::eEnabled); This seems to work. An alternative would be to let the current eEnabled state include those with the UA bit, and add a new state that doesn't and use that from the parser, something like this: LookupProperty(aProperty, mUnsafeRulesEnabled ? nsCSSProps::eEnabled : nsCSSProps::eEnabledNotUA); It makes a difference for all the existing (non-parser) calls to LookupProperty(prop,eEnabled). Not sure which is best...

As you suggested. Also added a property_database.js test which wasn't possible to have before. https://tbpl.mozilla.org/?tree=Try&rev=1809bea11799

Comment on attachment 8377867 [details] [diff] [review] part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS Review of attachment 8377867 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: layout/style/nsCSSProps.h @@ +259,1 @@ > eAny Can I suggest eEnabledInUASheets as the name? Might be more descriptive.

Comment on attachment 8377868 [details] [diff] [review] part 1. Implement the overflow-clip-box property in the style system. Review of attachment 8377868 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSPropList.h @@ +2482,5 @@ > CSS_PROP_DISPLAY( > + overflow-clip-box, > + overflow_clip_box, > + OverflowClipBox, > + CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS | Newline after the first "|" for consistency with all the other entries.

(In reply to Cameron McCormack (:heycam) from comment #26) > ::: layout/style/nsCSSProps.h > @@ +259,1 @@ > > eAny > > Can I suggest eEnabledInUASheets as the name? Might be more descriptive. As the name of the new value, that is. (Stupid Splinter quoting.)

> Can I suggest eEnabledInUASheets as the name? Might be more descriptive. Fixed > Newline after the first "|" for consistency with all the other entries. Fixed

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddfc587edda https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3420cc77d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/740b13866bac https://hg.mozilla.org/integration/mozilla-inbound/rev/f11cad93ceee

I had to back this out because of PGO only build failures: http://hg.mozilla.org/integration/mozilla-inbound/rev/31e4260825dc example: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3f443622555e

Yeah, it looks like a valid concern: static bool IsEnabled(nsCSSProperty aProperty, EnabledState aEnabled) { return IsEnabled(aProperty) || (aEnabled == eEnabledInUASheets && PropHasFlags(aProperty, CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS)) || aEnabled == eAny; } aProperty can be in the range 0 .. eCSSProperty_COUNT_with_aliases here, but PropHasFlags() use kFlagsTable[eCSSProperty_COUNT] and I assume eCSSProperty_COUNT_with_aliases > eCSSProperty_COUNT.

...and in particular, nsCSSProps::LookupProperty explicitly calls the 2-arg version of IsEnabled() (the one quoted in comment 32) with values that are >= _COUNT. So that's the problematic call-site. (or at least, one such call-site)

Comment on attachment 8377867 [details] [diff] [review] part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS >+++ b/layout/style/nsCSSProps.cpp >@@ -387,25 +387,25 @@ nsCSSProps::LookupProperty(const nsACStr > } > > nsCSSProperty res = nsCSSProperty(gPropertyTable->Lookup(aProperty)); > // Check eCSSAliasCount against 0 to make it easy for the > // compiler to optimize away the 0-aliases case. > if (eCSSAliasCount != 0 && res >= eCSSProperty_COUNT) { > static_assert(eCSSProperty_UNKNOWN < eCSSProperty_COUNT, > "assuming eCSSProperty_UNKNOWN doesn't hit this code"); >- if (IsEnabled(res) || aEnabled == eAny) { >+ if (IsEnabled(res, aEnabled)) { [this being that call-site ^^]

It seems very unlikely that there will ever be a need for a pref-enabled property that should be enabled in UA sheets that also needs to have an alias. So I simply skipped that check when the lookup is for an alias. We can implement it later if needed. I haven't tested this in a linux- pgo build, but it looks like it should be OK. (if anyone knows how to submit a linux-pgo job to Try, let me know) There is a "NS_ABORT_IF_FALSE(0 <= res && res < eCSSProperty_COUNT," for the value the alias maps to, so that 'res' should be safe to use with PropHasFlags(). The problem is convincing the compiler of that without any runtime cost. Some ugly casts will probably solve it - it just seems unnecessary to add that as long as there's no need.

Comment on attachment 8378790 [details] [diff] [review] part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS Review of attachment 8378790 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSProps.cpp @@ +395,2 @@ > } > + MOZ_ASSERT(eCSSAliasCount != 0); Why assert here? Just so we can remove this block of code if our alias list is ever empty? ::: layout/style/nsCSSProps.h @@ +194,5 @@ > +// This property is always enabled in UA sheets. This is meant to be used > +// together with a pref that enables the property for non-UA sheets. > +// Note that this doesn't affect aliases. An alias lookup is only > +// controlled by the pref (if any) and it's currently not possible to > +// make them available in UA sheets also when the pref is disabled. The "Note that this doesn't affect aliases" is actually true of all of these flags, since nsCSSPropAliasList.h doesn't have a flags field.

(In reply to Cameron McCormack (:heycam) from comment #36) > > + MOZ_ASSERT(eCSSAliasCount != 0); > > Why assert here? Just so we can remove this block of code if our alias list > is ever empty? I don't think we should remove this block even if we have no aliases becasue at some point we will need one again. My intent was to make sure we have some aliases because that's the only valid option at this point in the code -- that is, the current code already implies that calling LookupProperty with aProperty >= eCSSProperty_COUNT_with_aliases is illegal (IsEnabled(res) asserts that). http://hg.mozilla.org/mozilla-central/annotate/1238ef12b996/layout/style/nsCSSProps.cpp#l379 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProperty.h#45 I could have used this stronger assertion instead: MOZ_ASSERT(res < eCSSProperty_COUNT_with_aliases); which is clearer in intent, but IsEnabled(res) already does that. So MOZ_ASSERT(eCSSAliasCount != 0) is just a consistency-check that follows from the "if (res < eCSSProperty_COUNT)" that we have already dealt with. Perhaps a message would clarify? e.g. MOZ_ASSERT(eCSSAliasCount != 0, "'res' must be an alias at this point so we better have some!"); > > +// This property is always enabled in UA sheets. This is meant to be used > > +// together with a pref that enables the property for non-UA sheets. > > +// Note that this doesn't affect aliases. An alias lookup is only > > +// controlled by the pref (if any) and it's currently not possible to > > +// make them available in UA sheets also when the pref is disabled. > > The "Note that this doesn't affect aliases" is actually true of all of these > flags, since nsCSSPropAliasList.h doesn't have a flags field. Right, that's not what I was trying to express. Let me try again: // This property is always enabled in UA sheets. This is meant to be used // together with a pref that enables the property for non-UA sheets. // Note that if such a property has an alias, then any use of that alias // in an UA sheet will still be ignored unless the pref is enabled. // IOW, this bit has no effect on the use of aliases. (I'm happy to take suggestions for improvements on the text.) The reason I made the note is that disabling a property also disables any aliases (the last IsEnabled(res) checks the result from gAliases), so I wanted to point out that this UA bit does not affect aliases in the same way. Note also that we *can* make this bit have an effect on aliases too with some extra work, it just doesn't seem worth the effort to support that edge case until there's a need (which I doubt will ever arise).

(In reply to Mats Palmgren (:mats) from comment #37) > So MOZ_ASSERT(eCSSAliasCount != 0) is just a consistency-check that > follows from the "if (res < eCSSProperty_COUNT)" that we have already > dealt with. Perhaps a message would clarify? e.g. > > MOZ_ASSERT(eCSSAliasCount != 0, > "'res' must be an alias at this point so we better have some!"); Yes please. :-) > Right, that's not what I was trying to express. > Let me try again: > > // This property is always enabled in UA sheets. This is meant to be used > // together with a pref that enables the property for non-UA sheets. > // Note that if such a property has an alias, then any use of that alias > // in an UA sheet will still be ignored unless the pref is enabled. > // IOW, this bit has no effect on the use of aliases. > > (I'm happy to take suggestions for improvements on the text.) Ah I understand better now. That text sounds good. > The reason I made the note is that disabling a property also disables > any aliases (the last IsEnabled(res) checks the result from gAliases), > so I wanted to point out that this UA bit does not affect aliases in the > same way. > > Note also that we *can* make this bit have an effect on aliases too > with some extra work, it just doesn't seem worth the effort to support > that edge case until there's a need (which I doubt will ever arise). Yeah I agree not worth the trouble.

Added assertion messages and clarified the code comment per above.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1429315d28 https://hg.mozilla.org/integration/mozilla-inbound/rev/7437570602a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbda9035da4 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba884d47b839

https://hg.mozilla.org/mozilla-central/rev/ef1429315d28 https://hg.mozilla.org/mozilla-central/rev/7437570602a7 https://hg.mozilla.org/mozilla-central/rev/0fbda9035da4 https://hg.mozilla.org/mozilla-central/rev/ba884d47b839

Comment on attachment 8380089 [details] [diff] [review] part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 157846 User impact if declined: broken layout of <input type=text/password/number> and <textarea>, high visibility, breaks Google translate (bug 961347) for example Testing completed (on m-c, etc.): on m-c since 2014-02-23 Risk to taking this patch (and alternatives if risky): the patch in bug 965237 is also needed to fix the regression, and together they are medium risk. The alternative is to backout bug 157846. String or IDL/UUID changes made by this patch: none The request is for all patches in this bug, and bug 965237

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc5579a6f46f https://hg.mozilla.org/releases/mozilla-aurora/rev/f1885bd2a0bf https://hg.mozilla.org/releases/mozilla-aurora/rev/5637570fb387 https://hg.mozilla.org/releases/mozilla-aurora/rev/92ea8e6bdb6e

Mats, in order to update the doc (kindly created by ziyunfei), I deduced from this bug/patch: - We ships (or will) this (unprefixed) in Firefox 29. - There is a pref ( layout.css.overflow-clip-box.enabled ), but it is false by default (not enabled for the Web); only UA stylesheet can use it. - There is no spec (yet). - Does not inherit; does not animate - Initial value: padding-box - Applies to visual media - No other engines implement it. If you could comfirm/infirm these assertions, it would be nice :-)

Those statements are correct. The actual layout that "overflow-clip-box:content-box" now describes has a longer history though - all UAs layout <input type=text> in this way and has for decades. So perhaps the article could start with that background? the motivation for adding this property is to describe that layout in CSS terms. IOW, the 'content-box' clipping was hardcoded for <input type=text> etc, and 'padding-box' hardcoded for everything else. With this property you can now choose which clipping you want for any element. The reference to CSS2 is not correct for this property. I would expect it to be specified in "CSS Overflow Module" http://dev.w3.org/csswg/css-overflow-3/ if it's adopted by the CSSWG. Since there is no spec yet, I think it would be good to have a few examples in the article to illustrate what the property does to the clipping. I'll whip that up in a bit...

Here's a few simple examples for the MDN page if you need them.