966992 - Implement overflow-clip-box: content-box

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: WIP part 1. style

Comment 2

[Mats Palmgren (inactive)]

Attachment: WIP part 2. layout

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

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1. Implement the overflow-clip-box property in the style system.

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

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2. Implement layout for the overflow-clip-box property.

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

Comment 5

[Mats Palmgren (inactive)]

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 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Mats Palmgren (inactive)]

BTW, try builds are available if you want to test something:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dcdc7b59b70e/

Comment 9

[Mats Palmgren (inactive)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 11

[Mats Palmgren (inactive)]

Attachment: caret clipped by padding-rect

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

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[Mats Palmgren (inactive)]

Attachment: Another special case for the caret...

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 2. Implement layout for the overflow-clip-box property.

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 18

[Mats Palmgren (inactive)]

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 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 2. Implement layout for the overflow-clip-box property.

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

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 3. tests

Reftest + mochitests for testing caret rendering.

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 23

[Cameron McCormack (:heycam)]

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.

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS

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

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 1. Implement the overflow-clip-box property in the style system.

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 26

[Cameron McCormack (:heycam)]

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 27

[Cameron McCormack (:heycam)]

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.

Comment 28

[Cameron McCormack (:heycam)]

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

Comment 29

[Mats Palmgren (inactive)]

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

Fixed

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

Fixed

Comment 30

[Mats Palmgren (inactive)]

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

Comment 31

[(no longer active)]

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

Comment 32

[Mats Palmgren (inactive)]

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.

Comment 33

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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

Comment 35

[Mats Palmgren (inactive)]

Attachment: part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS

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 36

[Cameron McCormack (:heycam)]

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.

Comment 37

[Mats Palmgren (inactive)]

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

Comment 38

[Cameron McCormack (:heycam)]

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

Comment 39

[Mats Palmgren (inactive)]

Attachment: part 0. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS

Added assertion messages and clarified the code comment per above.

Comment 40

[Mats Palmgren (inactive)]

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

Comment 41

[Phil Ringnalda (:philor)]

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 42

[Mats Palmgren (inactive)]

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

Comment 43

[Mats Palmgren (inactive)]

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

Comment 44

[Jean-Yves Perrier [:teoli]]

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

Comment 45

[Mats Palmgren (inactive)]

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

Comment 46

[Mats Palmgren (inactive)]

Attachment: Examples

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

Comment 47

[Mats Palmgren (inactive)]

Attachment: Screenshot of "Examples" with the pref enabled