1055285 - Implement CSS parsing and computation of object-fit, object-position

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: add CSS support for 'object-fit'

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: add CSS support for 'object-fit'

(Sorry, previous version included an unrelated chunk by accident. Removed it here.)

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8475492 [details] [diff] [review]
part 1: add CSS support for 'object-fit'

Looks good.

Comment 4

[Cameron McCormack (:heycam)]

(I'll await later patches for changes to property_database.js.)

Comment 5

[Daniel Holbert [:dholbert]]

This patch had all the necessary changes to property_database.js for object-fit, I think (correct me if I missed something.

Later patch still coming for object-position, though.

Comment 6

[Cameron McCormack (:heycam)]

My apologies, it did indeed.  I just got off a plane, so I'll blame that.  ;)

I haven't seen this testing/profiles/prefs_general.js before -- do you know is that something we should now use to features which are not preffed on yet, rather than doing a SpecialPowers.pushPrefEnv?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #6)
> I haven't seen this testing/profiles/prefs_general.js before -- do you know
> is that something we should now use to features which are not preffed on
> yet, rather than doing a SpecialPowers.pushPrefEnv?

Sorry, I forgot to respond to this comment.

For new CSS properties that want testing in layout/style, "yes" -- I think we do want to use testing/profiles/prefs_general.js.

The alternative is:
 (a) Add SpecialPowers.pushPrefEnv calls to every single property_database.js-based test (assuming we want this property to get tested), **and**...
 (b) shift the test logic out of the "test_whatever.html" files and into a "file_whatever.html" helper, which gets dynamically loaded in an iframe *after* we've mad the pref-tweak. Otherwise, the pref won't be recognized.  This is necessary because (IIRC) prefs are enabled or disabled for a given page based on the pref settings _at document-load time_.  So, a call to pushPrefEnv will be too late, unless we shift all of the logic into an iframe that gets loaded dynamically after the call.

This alternative is doable, but probably more complexity/trouble than it's worth. (though I did this for the flexbox mochitests before flexbox was enabled by default on all channels, FWIW. And there, we simply didn't get property_database.js-based testing for the flexbox properties, when the pref was disabled by default, which was unfortunate.)

Comment 8

[Daniel Holbert [:dholbert]]

(Anyway -- this particular pref shouldn't be disabled-by-default for very long -- I'm only intending for it to be default-disabled until the yet-to-be-written layout-support patches land on trunk (in bug 624647, or perhaps a helper-bug), and they shouldn't be very complex.  So, the testing/profiles/prefs_general.js usage doesn't matter much in this case, I think.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 2: add CSS support for 'object-position' (except animation support)

This adds support for 'object-position', except for animation support, which will be coming in a subsequent patch. (Hence the eStyleAnimType_None in nsCSSPropList.h, making this non-animatable for now, so that we pass tests without animation support.)

Relevant facts about 'object-position':
 - It takes a single <position> value. (The only other thing to take a <position> is background-position, which takes one or more <position> values.)
 - Its initial value is "50% 50%".
More here: http://dev.w3.org/csswg/css-images-3/#object-position
The <position> type is defined here: http://dev.w3.org/csswg/css-backgrounds/#position

Also: this patch builds on top of bug 1060090, which refactors <position>-handling code a bit.

Comment 10

[Daniel Holbert [:dholbert]]

Try run with bug 1060090's patches and this bug's parts 1 & 2:
 https://tbpl.mozilla.org/?tree=Try&rev=503dbbe4b431

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8483834 [details] [diff] [review]
part 2: add CSS support for 'object-position' (except animation support)

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

::: layout/style/nsStyleStruct.cpp
@@ +1222,5 @@
>  //
>  nsStylePosition::nsStylePosition(void)
>  {
>    MOZ_COUNT_CTOR(nsStylePosition);
>    // positioning values not inherited

Maybe add a blank line above this comment, since with the blank lines around the SetInitialPercentValues call it's harder to tell that it applies to all of the following value initializations too.

Comment 12

[Daniel Holbert [:dholbert]]

Added that blank line locally. Thanks!

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 3: support animation/transitions on values for 'object-position'

This patch (the last one for this bug, I think) stacks on top of the just-attached final patch on bug 1060090, to add support animation for 'object-position' using the refactored-out functionality.

The StyleAnimation representation for an 'object-position' value is a single nsCSSValue, backed by an array of 4 entries.   That's what we use for each of the entries in a 'background-position' value-list, so this lets us share code with 'background-position'.  In a few cases here, though (e.g. operator= and operator==), I'm having us share code with eUnit_Calc, because that's our only other nsCSSValue-backed StyleAnimation representation.

(See bug 1060090 part 10 for a little more info on this 4-entry array representation.)

Comment 14

[Daniel Holbert [:dholbert]]

(Also, FWIW, I added the "This clause is for handling nsCSSValue-backed units" assertions because those clauses are directly touching mValue.mCSSValue (which is part of a union).  This lets us verify that "IsCSSValueUnit()" and these switch-clauses are in sync.)

(I'm also using NS_ABORT_IF_FALSE for those assertions because one of them is right up against an existing NS_ABORT_IF_FALSE; I'm happy to switch to MOZ_ASSERT if you prefer, though.)

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> (See bug 1060090 part 10 for a little more info on this 4-entry array representation.)

Sorry, meant 'bug 1060090 comment 10'


Here's a Try run with latest patches from bug 1060090 and this bug:
https://tbpl.mozilla.org/?tree=Try&rev=39a0211a6372

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8486155 [details] [diff] [review]
part 3: support animation/transitions on values for 'object-position'

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

r=me with these comments addressed.

::: layout/style/StyleAnimationValue.cpp
@@ +559,5 @@
>        return true;
>      }
> +    case eUnit_ObjectPosition: {
> +      const nsCSSValue *position1 = aStartValue.GetCSSValueValue();
> +      const nsCSSValue *position2 = aEndValue.GetCSSValueValue();

"*"s next to the types please.  (Let's slowly migrate this file to matching the style guide.)

@@ +2077,5 @@
>        return true;
>      }
> +    case eUnit_ObjectPosition: {
> +      const nsCSSValue *position1 = aValue1.GetCSSValueValue();
> +      const nsCSSValue *position2 = aValue2.GetCSSValueValue();

And here.

@@ +3208,5 @@
>            break;
>          }
>  
> +        case eCSSProperty_object_position: {
> +          const nsStylePosition *stylePos =

And here.

@@ +3620,5 @@
>        NS_ABORT_IF_FALSE(aOther.mValue.mCSSValue, "values may not be null");
>        mValue.mCSSValue = new nsCSSValue(*aOther.mValue.mCSSValue);
>        if (!mValue.mCSSValue) {
>          mUnit = eUnit_Null;
>        }

The |new nsCSSValue| call should be infallible now; mind filing a followup to remove the null checks in this and a few other cases in this switch statement?

::: layout/style/StyleAnimationValue.h
@@ +214,5 @@
>      eUnit_Color,
>      eUnit_Calc, // nsCSSValue* (never null), always with a single
>                  // calc() expression that's either length or length+percent
> +    eUnit_ObjectPosition, // nsCSSValue* (never null), always with a
> +                          // 4-entry nsCSSValue::Array.

No full stop at the end, to match the other comments.

::: layout/style/test/test_transitions_per_property.html
@@ +181,5 @@
>                      test_length_clamped, test_percent_clamped ],
>      "min-width": [ test_length_transition, test_percent_transition,
>                     test_length_percent_calc_transition,
>                     test_length_clamped, test_percent_clamped ],
> +    "object-position": [ test_background_position_transition ],

Could you add a/some tests to test_background_position_transition in test_transitions_per_property.html with values that have both the edge keywords and calc() values?

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #16)
> Comment on attachment 8486155 [details] [diff] [review]
> part 3: support animation/transitions on values for 'object-position'
> "*"s next to the types please.  (Let's slowly migrate this file to matching
> the style guide.)

Fixed those.

> The |new nsCSSValue| call should be infallible now; mind filing a followup
> to remove the null checks in this and a few other cases in this switch
> statement?

Great minds think alike! :) Bug already filed, patch pending review from dbaron -- bug 1063775.

> > +    eUnit_ObjectPosition, // nsCSSValue* (never null), always with a
> > +                          // 4-entry nsCSSValue::Array.
> 
> No full stop at the end, to match the other comments.

Fixed.

> ::: layout/style/test/test_transitions_per_property.html
> > +    "object-position": [ test_background_position_transition ],
> 
> Could you add a/some tests to test_background_position_transition in
> test_transitions_per_property.html with values that have both the edge
> keywords and calc() values?

Will do. (I'll give those a round of testing, and then will land.)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 4: better tests for transitions with edge keywords & calc()

Here's a patch to add a few more transitions tests.

Try run with all patches:
  https://tbpl.mozilla.org/?tree=Try&rev=67274dc40080

Comment 19

[Daniel Holbert [:dholbert]]

Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb99e0c02a7
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bce3f21f995
part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/156fa5c40afc
part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a702947b274

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bcb99e0c02a7
https://hg.mozilla.org/mozilla-central/rev/2bce3f21f995
https://hg.mozilla.org/mozilla-central/rev/156fa5c40afc
https://hg.mozilla.org/mozilla-central/rev/7a702947b274

Comment 21

[Jean-Yves Perrier [:teoli]]

Just to assess if this _already_ needs documentation (both properties surely will at some point:-) ) and because it was unclear in some other similar bugs:

This bug only implements the parsing, computation and allows animation of one of the property, but has no layout effect (it will be another bug)?

Comment 22

[Daniel Holbert [:dholbert]]

Exactly correct. Layout support, to react to these properties, is coming in bug 624647. Until then, this doesn't really do anything.