text-shadow property serialization.

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
dom/animation/test-css-animations/test_keyframeeffect-getkeyframes.html:

We expect : "1px 1px 2px 0px rgb(0, 0, 0), 0px 0px 16px 0px rgb(0, 0, 255), 0px 0px 3.2px 0px rgb(0, 0, 255)"

But got   : "1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px rgb(0, 0, 255)"

ToCss of text-shadow's specified value doesn't output the spread radius if there aren't this value. [1]

[1] http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/servo/components/style/values/specified/mod.rs#742-745

If we output the spread radius value always, this test will success.
Blocks: 1292283
Is this really stylo bug?
The length values for text-shadow are defined as '<length>{2,3}'.
The general rule of thumb for CSS serialization is that components that have their default value are not serialized. So it sounds like it might be Gecko that should change.
(Assignee)

Comment 3

2 years ago
Thanks.

This tests is animate text-shadow to 'none', specified underlying value is '1px 1px 2px rgb(0, 0, 0), 0 0 16px rgb(0, 0, 255), 0 0 3.2px rgb(0, 0, 255)'. [1]

[2] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html#570-593

And test expect first keyframe will be '1px 1px 2px 0px rgb(0, 0, 0), 0px 0px 16px 0px rgb(0, 0, 255), 0px 0px 3.2px 0px rgb(0, 0, 255)'.

The gecko will get the first keyframe from base value since this test doesn't specified the 'from' value.
(Assignee)

Comment 4

2 years ago
Maybe, it is same to bug 1339334?
That test seems wrong. If blur-radius is zero the component should be omitted but, that said, there should still only be a maximum of 3 lengths for text-shadow (unlike box-shadow I guess). So I think that test should be checking for:

  1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px rgb(0, 0, 255)

(And even if text-shadow does later include a spread-radius, we shouldn't serialize it if it is an optional component with its default value of 0.)
(Assignee)

Comment 6

2 years ago
(In reply to Brian Birtles (:birtles) from comment #5)
> That test seems wrong. If blur-radius is zero the component should be
> omitted but, that said, there should still only be a maximum of 3 lengths
> for text-shadow (unlike box-shadow I guess). So I think that test should be
> checking for:
> 
>   1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px
> rgb(0, 0, 255)
> 
> (And even if text-shadow does later include a spread-radius, we shouldn't
> serialize it if it is an optional component with its default value of 0.)

Thanks.

I think we need to ignore the 4th blur-radius value when appending string of this value (nsCSSValue::Array). [1]

[1] https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/layout/style/nsCSSValue.cpp#1355

This is rough implementation for validating the above my understanding:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6185ebc14d408e0ebd5060c680668335409d490
We should also skip the third length if it is its default value. I notice Chrome doesn't seem to do this, although Edge does.
(Assignee)

Comment 8

2 years ago
Thank you for information!

(In reply to Brian Birtles (:birtles) from comment #7)
> We should also skip the third length if it is its default value. I notice
> Chrome doesn't seem to do this, although Edge does.

The Stylo won't skip the third length when it is default value.[1]

[1] http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/servo/components/style/values/specified/mod.rs#741

So I need to update this implementation as well. And I think that we will need to add same changes for box-shadow.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > That test seems wrong. If blur-radius is zero the component should be
> > omitted but, that said, there should still only be a maximum of 3 lengths
> > for text-shadow (unlike box-shadow I guess). So I think that test should be
> > checking for:
> > 
> >   1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px
> > rgb(0, 0, 255)
> > 
> > (And even if text-shadow does later include a spread-radius, we shouldn't
> > serialize it if it is an optional component with its default value of 0.)
> 
> Thanks.
> 
> I think we need to ignore the 4th blur-radius value when appending string of
> this value (nsCSSValue::Array). [1]
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 464b2a3c25aa1065760d9ecbb0870bca4a66c62e/layout/style/nsCSSValue.cpp#1355

Instead of tweaking serialization, should we disallow to set the forth length value for text-shadow?
Dropping stylo: prefix since we will re-use this bug to fix the serialization for text-shadow in getKeyframes() for gecko.
Component: CSS Parsing and Computation → DOM: Animation
Summary: stylo: text-shadow property serialization. → text-shadow property serialization.
(Assignee)

Comment 12

2 years ago
Try test:(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> > (In reply to Brian Birtles (:birtles) from comment #5)
> > > That test seems wrong. If blur-radius is zero the component should be
> > > omitted but, that said, there should still only be a maximum of 3 lengths
> > > for text-shadow (unlike box-shadow I guess). So I think that test should be
> > > checking for:
> > > 
> > >   1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px
> > > rgb(0, 0, 255)
> > > 
> > > (And even if text-shadow does later include a spread-radius, we shouldn't
> > > serialize it if it is an optional component with its default value of 0.)
> > 
> > Thanks.
> > 
> > I think we need to ignore the 4th blur-radius value when appending string of
> > this value (nsCSSValue::Array). [1]
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 464b2a3c25aa1065760d9ecbb0870bca4a66c62e/layout/style/nsCSSValue.cpp#1355
> 
> Instead of tweaking serialization, should we disallow to set the forth
> length value for text-shadow?

When parsing the text-shadow, we skip setting this fourth value (and inset value) actually[1]. But we set the zero value when convert to CSSShadowItem if the unit of this fourth value is eCSSUnit_Null[2]. And we generate serialized array from CSSShadowItem[3]. Even if we don't specify the spread radius, the gecko will serialize as zero value.

[1] http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/style/nsCSSParser.cpp#17129,17145
[2] http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/layout/style/nsRuleNode.cpp#4447
[3] http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/style/StyleAnimationValue.cpp#340


Rough implementation:
https://hg.mozilla.org/try/rev/6b71df9ce17b415b73fa0e2e2cdded0dd5b29b92
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
(Assignee)

Comment 14

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #13)
> Created attachment 8882379 [details]
> Bug 1374564 - Skip serialize box-shadow/text-shadow and filter's drop-shadow
> when it is not specified or default value.
> 
> Review commit: https://reviewboard.mozilla.org/r/153424/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/153424/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61eeb7947272514ec676506274ef61bc3053e2f2&group_state=expanded

Comment 15

2 years ago
mozreview-review
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158660

::: layout/style/StyleAnimationValue.cpp:337
(Diff revision 1)
>  
>    // X, Y, Radius, Spread, Color, Inset
>    RefPtr<nsCSSValue::Array> arr = nsCSSValue::Array::Create(6);
>    arr->Item(0).SetIntegerCoordValue(aShadow->mXOffset);
>    arr->Item(1).SetIntegerCoordValue(aShadow->mYOffset);
> +  if (aShadow->mRadius != 0) {

Instead of checking the value is not zero, should we check the property is not text-shadow?
I mean, we have no idea about what the default value of the radius in this function, right?
Attachment #8882379 - Flags: review?(hikezoe)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158660

> Instead of checking the value is not zero, should we check the property is not text-shadow?
> I mean, we have no idea about what the default value of the radius in this function, right?

I was trying to fix the serialization with default value, In next patch is focused on text-shadow's fourth value and I remove test skip code.
Could you please review again?
Thanks.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8882427 [details]
Bug 1374564 - Remove skipping code of getkeyframes which used text-shadow serialization.

https://reviewboard.mozilla.org/r/153524/#review158696

Should we drop isServoEnabled() function altogether?
Attachment #8882427 - Flags: review?(hikezoe) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158700

::: layout/style/StyleAnimationValue.cpp:330
(Diff revision 2)
>  }
>  
>  static void
>  AppendCSSShadowValue(const nsCSSShadowItem *aShadow,
> -                     nsCSSValueList **&aResultTail)
> +                     nsCSSValueList **&aResultTail,
> +                     bool isTextShadow)

Adding extra boolean argument is generally bad idea.

::: layout/style/StyleAnimationValue.cpp:1012
(Diff revision 2)
>      case eCSSKeyword_drop_shadow: {
>        MOZ_ASSERT(!func1.GetListValue()->mNext && !func2.GetListValue()->mNext,
>                   "drop-shadow filter func doesn't support lists");
>        if (!ComputeSingleShadowSquareDistance(func1.GetListValue(),
>                                               func2.GetListValue(),
> -                                             aSquareDistance)) {
> +                                             aSquareDistance, false)) {

Here, we have no idea what the 'false' means.
Attachment #8882379 - Flags: review?(hikezoe)
Drive by comment: Rather than passing an isTextShadow (should be aIsTextShadow) bool around, it's probably more descriptive to just pass aProperty along and compare against that. (If some of the call sites don't know what property is being used, then at very least we should add an enum param "aIncludesSpread" since that's what we really care about.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158700

Thank you so much,
I addressed the patch.
(Assignee)

Comment 25

2 years ago
(In reply to Brian Birtles (:birtles) from comment #21)
> Drive by comment: Rather than passing an isTextShadow (should be
> aIsTextShadow) bool around, it's probably more descriptive to just pass
> aProperty along and compare against that. (If some of the call sites don't
> know what property is being used, then at very least we should add an enum
> param "aIncludesSpread" since that's what we really care about.)

Thanks brian,

This comment helped my understand. I added the nsCSSPropertyId to the each functions.

Comment 26

2 years ago
mozreview-review
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158724

This looks much better now. Now I realized that there are at least two call sites for filter property. So my question is "don't we need to drop the Item(3) for drop-shadow in filter?
The spec says "drop-shadow() = drop-shadow( <length>{2,3} <color>? )", I am seeing old spec?
Attachment #8882379 - Flags: review?(hikezoe)
(Assignee)

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158724

Thanks.

OK, it is right. I will get it done on this bug.
I added the code of skipping the fourth value of drop-shadow.

And,, I added this drop-shadow's test into dom/animation/test/css-animations/file_keyeffect_getkeyframes.html since currently it is this file to using this serialization.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8882379 [details]
Bug 1374564 - Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter.

https://reviewboard.mozilla.org/r/153424/#review158860

::: layout/style/StyleAnimationValue.cpp:339
(Diff revision 4)
> -  // NOTE: This code sometimes stores mSpread: 0 even when
> -  // the parser would be required to leave it null.
> +  if (aProperty != eCSSProperty_text_shadow &&
> +      aProperty != eCSSProperty_filter) {

We should check aProperty == eCSSProperty_box_shadow instead?

Also if you generally prefer to factor out smaller functions, we can have a function that checks a given property needs spread value or not something like this;

static bool
NeedsSpreadForProperty(nsCSSPropertyID aProperty)
{
  MOZ_ASSERT(/* I guess we should check this function is supposed work for property which as shadow */);
  return aProperty == eCSSProperty_box_shadow;
}
?

::: layout/style/StyleAnimationValue.cpp:904
(Diff revision 4)
> +    if (i == 3 && (aProperty == eCSSProperty_text_shadow ||
> +                   aProperty == eCSSProperty_filter)) {

Likewise here.

::: layout/style/StyleAnimationValue.cpp:2065
(Diff revision 4)
> +    if (i == 3 && (aProperty == eCSSProperty_text_shadow ||
> +                   aProperty == eCSSProperty_filter)) continue;

Likewise.

::: layout/style/StyleAnimationValue.cpp:2804
(Diff revision 4)
>    nsCSSValueList* tail = nullptr;
>    while (aShadow1 && aShadow2) {
>      UniquePtr<nsCSSValueList> shadowValue =
>        AddWeightedShadowItems(aCoeff1, aShadow1->mValue,
>                               aCoeff2, aShadow2->mValue,
> -                             aColorAdditionType);
> +                             aColorAdditionType, aProperty);

Nit: please put |aProperty| on the next line.

::: layout/style/StyleAnimationValue.cpp:4841
(Diff revision 4)
>        }
>        nsAutoPtr<nsCSSValueList> result;
>        nsCSSValueList **resultTail = getter_Transfers(result);
>        for (uint32_t i = 0, i_end = shadowArray->Length(); i < i_end; ++i) {
> -        AppendCSSShadowValue(shadowArray->ShadowAt(i), resultTail);
> +        AppendCSSShadowValue(shadowArray->ShadowAt(i), resultTail,
> +                             aProperty);

Nit: Is this |aProperty| can't be put on the above line? If it's over 80 chars, then |resultTail| should be put on the next line of AppendCSSShadowValue.
Attachment #8882379 - Flags: review?(hikezoe) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8882612 [details]
Bug 1374564 - Add drop-shadow's serialization test.

https://reviewboard.mozilla.org/r/153696/#review158874

::: dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:125
(Diff revision 1)
>  @keyframes anim-filter {
>    to { filter: blur(5px) sepia(60%) saturate(30%); }
>  }
>  
> +@keyframes anim-filter-drop-shadow {
> +  to { filter: drop-shadow(50px 30px 10px red); }

I am pretty sure we should use rgb(255, 0, 0) intead of 'red' in this test case because we don't care about the color serialization differences in the test case.  I am guessing this test fails on gecko?

::: dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:580
(Diff revision 1)
> +
> +  assert_equals(frames.length, 2, "number of frames");
> +
> +  var expected = [
> +    { offset: 0, computedOffset: 0, easing: "ease",
> +      filter: "none" },

Could you please add an interesting test case in the 'from' keyframe as well?
Attachment #8882612 - Flags: review?(hikezoe) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8882612 [details]
Bug 1374564 - Add drop-shadow's serialization test.

https://reviewboard.mozilla.org/r/153696/#review158874

Thanks hiro,

This test will fail on Stylo since the order of serialized animation value is different from gecko.
I think that it is gecko serialization bug, so I file the bug and I will fix it on bug 1377541, and in this bug, I added code of skipping those shadow serialization test if enabled servo preference.
I'd really prefer we don't skip these tests. We're like to forget to enable them and it makes it harder to track which tests pass. If at all possible, please just annotate the test as failing.
(Assignee)

Comment 38

2 years ago
Thanks, Brian,

(In reply to Brian Birtles (:birtles) from comment #37)
> I'd really prefer we don't skip these tests. We're like to forget to enable
> them and it makes it harder to track which tests pass. If at all possible,
> please just annotate the test as failing.

I really wanted to fix in that way. But this test is the mochitest using the tesharness.js. So we can't add test fail annotation it. (I think we can skip the test by using 'skip-if = stylo' on mochitest.ini, but it will skip for the whole of this test file.)

So I added skipping code into this test file.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #38)
> I really wanted to fix in that way. But this test is the mochitest using the
> tesharness.js. So we can't add test fail annotation it. (I think we can skip
> the test by using 'skip-if = stylo' on mochitest.ini, but it will skip for
> the whole of this test file.)

We don't run these tests for Stylo at all (bug 1346103) so you can just let them fail. No need to skip them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

2 years ago
(In reply to Brian Birtles (:birtles) from comment #39)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #38)
> > I really wanted to fix in that way. But this test is the mochitest using the
> > tesharness.js. So we can't add test fail annotation it. (I think we can skip
> > the test by using 'skip-if = stylo' on mochitest.ini, but it will skip for
> > the whole of this test file.)
> 
> We don't run these tests for Stylo at all (bug 1346103) so you can just let
> them fail. No need to skip them.

Thanks.
Ok. I remove the skipping code.

Comment 44

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/73a5b3d5f996
Skip serialize shadow's fourth value when property is text-shadow or drop-shadow of filter. r=hiro
https://hg.mozilla.org/integration/autoland/rev/e59c1d5e8433
Remove skipping code of getkeyframes which used text-shadow serialization. r=hiro
https://hg.mozilla.org/integration/autoland/rev/07262834ebfa
Add drop-shadow's serialization test. r=hiro

Comment 45

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73a5b3d5f996
https://hg.mozilla.org/mozilla-central/rev/e59c1d5e8433
https://hg.mozilla.org/mozilla-central/rev/07262834ebfa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
What's the status here? Running dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a Stylo build from yesterday's m-c, I get as one of the failures:

  assert_equals: value for 'textShadow' on ComputedKeyframe #0 expected "1px 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px rgb(0, 0, 255)" but got "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0, 0, 255) 0px 0px 3.2px"

Perhaps both Gecko and the test are wrong?
Flags: needinfo?(mantaroh)
(Assignee)

Comment 47

2 years ago
(In reply to Brian Birtles (:birtles) from comment #46)
> What's the status here? Running
> dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a
> Stylo build from yesterday's m-c, I get as one of the failures:
> 
>   assert_equals: value for 'textShadow' on ComputedKeyframe #0 expected "1px
> 1px 2px rgb(0, 0, 0), 0px 0px 16px rgb(0, 0, 255), 0px 0px 3.2px rgb(0, 0,
> 255)" but got "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0,
> 0, 255) 0px 0px 3.2px"
> 
> Perhaps both Gecko and the test are wrong?

The bug of gecko is bug 1377541.
We need fix gecko side and these test.
Flags: needinfo?(mantaroh)
You need to log in before you can comment on or make changes to this bug.