Closed Bug 1363246 Opened 7 years ago Closed 7 years ago

Add w-p-t of font stretch's additive test.

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(2 files)

test-case1
704 bytes, text/html
Details
Bug 1363246 - Add w-p-t of font stretch's addition/accumulation.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
STR:  Open attachment file.

Expected Result: calculated value is 'normal'.
Actually Result: calculated value is 'ultra-condensed'.

I think that this property's animation treat as composite='replace'.
However spec doesn't explain that this property animatable type is 'non-additive'.
Attached file test-case1 — 
Testcase1: Animate font-stretch from 'ultra-condensed' to 'ultra-expanded'. An initial value is 'normal'.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> STR:  Open attachment file.
> 
> Expected Result: calculated value is 'normal'.

The exepected value is 'semi-expanded' here. i.e. normal + ultra-condensed = semi-expanded.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> > STR:  Open attachment file.
> > 
> > Expected Result: calculated value is 'normal'.
> 
> The exepected value is 'semi-expanded' here. i.e. normal + ultra-condensed =
> semi-expanded.

According to css-fonts-4[1], we should treat font-stretch as number. 
Perhaps, expected result is 'extra-expanded'.

normal(100%) + ultra-condensed(50%) = extra-expanded(150%)

[1] https://drafts.csswg.org/css-fonts-4/#font-stretch-prop
Oh, did not know that. We already support level 4?
We should fix this ASAP, and definitely before the next upstream to web-platform-tests. Landing web platform tests that fail due to bugs in the test itself imposes a burden on other browser vendors to mark those same tests as failing (and risks having those tests remain disabled in other browser vendors' respositories) and is something we should avoid.

Also, we should fix the tabs added to property-types.js.
And if writing tests that pass in both Gecko and Servo proves difficult to do in a hurry, then at very least we should define an empty testAddition method (or whatever it is called) with a TODO comment so that we can remove the failure annotation from the meta .ini file.
Ah, indeed. Then I'd say we should implement additive test case for level 3 since interpolation test there is level 3.
Mantaroh, would you mind taking this soonish? If you have no time, I will take this.
Flags: needinfo?(mantaroh)
Thanks.
As discussed, We will be empty additive and accumulate tests for now.
Flags: needinfo?(mantaroh)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #8)
> Thanks.
> As discussed, We will be empty additive and accumulate tests for now.

Could you please post the discussion here?
I'd like to know how we track this missing implementation.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #8)
> > Thanks.
> > As discussed, We will be empty additive and accumulate tests for now.
> 
> Could you please post the discussion here?
> I'd like to know how we track this missing implementation.

I'm sorry I didn't make it clear enough. It is same to comment 6. I think that we need to remove failure annotation first.
Attachment #8870312 - Flags: review?(bbirtles)
Clear the review? flag since my understand is wrong.

Current implementation of gecko will support to accumulate animation. I thought that animation from underlying value(i.e. start value is 'normal' when current time = 0 in the case of attachment 8865709 [details]).

If we run following test-case, gecko will display 'normal'.
----------------------------------------------------------------------------
target.style.fontStretch='condensed';
animation = target.animate({fontStretch:['expanded', 'ultra-expanded']},
                           {duration: 1000, composite:'add'});

logger.log("Web Animations:" + getComputedStyle(target).fontStretch);
----------------------------------------------------------------------------
Summary: The font-stretch doesn't apply composite='add'. → Add w-p-t of font stretch's additive test.
Assignee: nobody → mantaroh
Comment on attachment 8870312 [details]
Bug 1363246 - Add w-p-t of font stretch's addition/accumulation.

https://reviewboard.mozilla.org/r/141770/#review145838

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1646
(Diff revision 2)
> +      var animation = target.animate({ [idlName]:
> +                                         ['expanded',
> +                                          'ultra-expanded'] },
> +                                     { duration: 1000, composite: composite });

The wrapping here is a bit odd. I'd probably just break after the = in this case to make it:

      var animation =
        target.animate({ [idlName]: ['expanded', 'ultra-expanded'] },
                       { duration: 1000, composite: composite });

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1650
(Diff revision 2)
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'normal' }]);

Here too, the wrapping is a bit odd.

This might be easier to read / shorter as:

      testAnimationSamples(animation, idlName,
                           [ { time: 0, expected: 'normal' } ]);

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1650
(Diff revision 2)
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'normal' }]);

Can we add second sample time where the result is not 'normal'?

I'm concerned that this test might pass even in implementations that don't implement this if they just naively return 'normal' for all values.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1653
(Diff revision 2)
> +                                          'ultra-expanded'] },
> +                                     { duration: 1000, composite: composite });
> +      testAnimationSamples(
> +        animation, idlName,
> +        [{ time: 0, expected: 'normal' }]);
> +    }, property + ': font-stretch');

This seems a bit odd. Won't this produce, 'font-stretch: font-stretch'?

Perhaps this should be:

  `${property} uses font-stretch behavior for composite type '${composite}'`

or something like that.
Attachment #8870312 - Flags: review?(bbirtles) → review+
Comment on attachment 8870312 [details]
Bug 1363246 - Add w-p-t of font stretch's addition/accumulation.

https://reviewboard.mozilla.org/r/141770/#review145838

Thanks for reviewing this.

I fixed the previous patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d3561bc990dedf26873674cb99e0763ae15d5d5
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/42e125f14c1b
Add w-p-t of font stretch's addition/accumulation. r=birtles
https://hg.mozilla.org/mozilla-central/rev/42e125f14c1b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: