Mats Palmgren (inactive) Assignee
	Description • 10 years ago

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.)

	Mats Palmgren (inactive) Assignee
	Comment 2 • 10 years ago

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...

	Mats Palmgren (inactive) Assignee
	Comment 3 • 10 years ago

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).

	Mats Palmgren (inactive) Assignee
	Comment 4 • 10 years ago

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).

	Mats Palmgren (inactive) Assignee
	Comment 5 • 10 years ago

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.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 10 years ago

(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.

	Mats Palmgren (inactive) Assignee
	Comment 8 • 10 years ago

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/

	Mats Palmgren (inactive) Assignee
	Comment 9 • 10 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 10 years ago

(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.

	Mats Palmgren (inactive) Assignee
	Comment 11 • 10 years ago

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).

	Mats Palmgren (inactive) Assignee
	Comment 12 • 10 years ago

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...

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 10 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 14 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 15 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 17 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 18 • 10 years ago

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...

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 10 years ago

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!

	Mats Palmgren (inactive) Assignee
	Comment 20 • 10 years ago

(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.

	Mats Palmgren (inactive) Assignee
	Comment 21 • 10 years ago

Reftest + mochitests for testing caret rendering.

https://tbpl.mozilla.org/?tree=Try&rev=1de5f2f4bdf2

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 10 years ago

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

	Cameron McCormack (:heycam)
	Comment 23 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 24 • 10 years ago

(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...

	Mats Palmgren (inactive) Assignee
	Comment 25 • 10 years ago

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

	Cameron McCormack (:heycam)
	Comment 26 • 10 years ago

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.

	Cameron McCormack (:heycam)
	Comment 27 • 10 years ago

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.

	Cameron McCormack (:heycam)
	Comment 28 • 10 years ago

(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.)

	Mats Palmgren (inactive) Assignee
	Comment 29 • 10 years ago

> Can I suggest eEnabledInUASheets as the name?  Might be more descriptive.

Fixed

> Newline after the first "|" for consistency with all the other entries.

Fixed

	Mats Palmgren (inactive) Assignee
	Comment 30 • 10 years ago
landing

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

	(no longer active)
	Comment 31 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 32 • 10 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 33 • 10 years ago

...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)

	Daniel Holbert [:dholbert]
	Comment 34 • 10 years ago

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 ^^]

	Mats Palmgren (inactive) Assignee
	Comment 35 • 10 years ago

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.

	Cameron McCormack (:heycam)
	Comment 36 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 37 • 10 years ago

(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).

	Cameron McCormack (:heycam)
	Comment 38 • 10 years ago

(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.

	Mats Palmgren (inactive) Assignee
	Comment 39 • 10 years ago

Added assertion messages and clarified the code comment per above.

	Mats Palmgren (inactive) Assignee
	Comment 40 • 10 years ago
landing

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

	Phil Ringnalda (:philor)
	Comment 41 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 42 • 10 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 43 • 10 years ago
landing

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

	Jean-Yves Perrier [:teoli]
	Comment 44 • 10 years ago

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 :-)

	Mats Palmgren (inactive) Assignee
	Comment 45 • 10 years ago

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...

	Mats Palmgren (inactive) Assignee
	Comment 46 • 10 years ago

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