1348173 - stylo: Implement the -moz-border-*-colors property

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

Having a look at our internal use of -moz-border-*-colors, it seems that it is pretty hard to remove its support. So sadly we would have to implement it in Stylo.

I guess it would be important if we want to enable stylo for chrome documents, but may not be that urgent if we don't.

Anyway, there are tests for it.

Comment 1

[Bobby Holley (:bholley)]

I can't quite tell from comment 0. Which of (a) chrome documents, (b) UA sheets, and (c) web pages make use of these properties?

If the answer is only (a), then this would be p4. But if we're unsure of whether web pages use this, it might be less work to just add support rather than trying to deprecate.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

For (a), we are definitely using these properties a lot in chrome documents.

For (b), heycam pointed out to me on IRC that we do use it in UA sheet, but at only one place [1], so we might be able to rewrite something to make it work without these properties. But that could be more complicated than just fixing this one.

For (c), I don't believe sites would use this... They should be using more reliably web technique available across browsers like border-image.

Another concern is that there are 

[1] https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/layout/style/res/forms.css#930-933

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2)
> Another concern is that there are 

Oops... Forgot to remove this line. I wanted to mention tests, but it doesn't seem to me we have many tests rely on it.

Comment 4

[Bobby Holley (:bholley)]

Affects form controls given the UA stylesheet rule.

Comment 5

[Jeremy Chen ([e-mail])]

Looks like more than 100 failures are about this, so I'd like to give it a shot.

From the computation step [1], looks like we compute and store these values in a structure, nsBorderColors [2]. So, maybe couple bindings might need to be added to support this. I haven't dug deep though.


[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/layout/style/nsRuleNode.cpp#7721-7762
[2] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/layout/style/nsStyleStruct.h#971

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review commit: https://reviewboard.mozilla.org/r/131046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131046/

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1348173 - Stylo: add -moz-border-*-colors support.

Note that we also added -moz-border-*-colors as sub_properties of the 'border'
shorthand, so we can make the 'border' shorthand reset -moz-border-*-colors.

Review commit: https://reviewboard.mozilla.org/r/131048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131048/

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/1-2/

Comment 9

[Jeremy Chen ([e-mail])]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131048/diff/1-2/

Comment 10

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19f01a87ca2b7e64555cdd31b5467d6c92f97768

Looks like we got couple UNEXPECTED-PASS on reftests. \o/

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/131046/#review133746

::: layout/style/ServoBindings.cpp:969
(Diff revision 2)
> +void
> +Gecko_CopyMozBorderColors(nsStyleBorder* aDest, const nsStyleBorder* aSrc,
> +                          mozilla::Side aSide)
> +{

Please assert in here that aDest->mBorderColors is null.

::: layout/style/ServoBindings.cpp:973
(Diff revision 2)
> +  if (aSrc->mBorderColors) {
> +    aDest->EnsureBorderColors();
> +    aDest->ClearBorderColors(aSide);
> +    if (aSrc->mBorderColors[aSide]) {
> +      aDest->mBorderColors[aSide] = aSrc->mBorderColors[aSide]->Clone();
> +    }
> +  }

Maybe factor out this code so that we can call it from here and from the nsStyleBorder copy constructor?

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/131048/#review133752

r=me with these fixed.

::: commit-message-6a3ff:3
(Diff revision 2)
> +In Gecko, we use double pointers to nsBorderColors to store -moz-border-*-colors.
> +We can simplify the implementation of computed value in Servo by using Option<Vec>.

(I think it would have been fine to make Gecko's storage use an nsTArray for simplicity, too.)

::: servo/components/style/properties/gecko.mako.rs:909
(Diff revision 2)
> +                    bindings::Gecko_ClearMozBorderColors(&mut self.gecko,
> +                                                         structs::Side::eSide${to_camel_case(side.ident)});

Hmm, so if it's necessary to call ClearMozBorderColors here, then it's also necessary to to do after the EnsureMozBorderColors call below.

::: servo/components/style/properties/longhand/border.mako.rs:151
(Diff revision 2)
> +                match *self {
> +                    SpecifiedValue::Colors(ref vec) => {
> +                        let mut result = Vec::new();
> +                        for color in vec {
> +                            result.push(color.to_computed_value(context));
> +                        }
> +                        computed_value::T(Some(result))
> +                    },
> +                    SpecifiedValue::None => computed_value::T(None),
> +                }

You could call map() on the Vec to avoid the loop, e.g.:

  let value = self.map(|v| v.map(|c| c.to_computed_value(context)));
  computed_value::T(value);

or if that's a little unclear:

  match *self {
      SpecifiedValue::Colors(ref vec) => {
          computed_value::T(Some(vec.map(|c| c.to_computed_value(context))
      }
      SpecifiedValue::None => {
          computed_value::T(None)
      }
  }

And similarly below in from_computed_value.

::: servo/components/style/properties/shorthand/border.mako.rs:134
(Diff revision 2)
> -)}" spec="https://drafts.csswg.org/css-backgrounds/#border">
> +                    ${' '.join('-moz-border-%s-colors' % side
> +                    for side in ['top', 'right', 'bottom', 'left']) if product == 'gecko' or data.testing else ''}"
> +    spec="https://drafts.csswg.org/css-backgrounds/#border">
> +
> +    % if product == "gecko" or data.testing:
> +        use properties::longhands::{_moz_border_top_colors, _moz_border_right_colors, _moz_border_bottom_colors, _moz_border_left_colors};

Please keep to 120 columns.  (Servo's tidy will probably fail due to this.)

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/131046/#review133746

> Please assert in here that aDest->mBorderColors is null.

Will do.

> Maybe factor out this code so that we can call it from here and from the nsStyleBorder copy constructor?

Sounds good to me, will do.

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/131048/#review133752

> (I think it would have been fine to make Gecko's storage use an nsTArray for simplicity, too.)

Filed Bug 1358007 for this.

> Hmm, so if it's necessary to call ClearMozBorderColors here, then it's also necessary to to do after the EnsureMozBorderColors call below.

Yes, I should've added ClearMozBorderColors() call here. Thanks.

> You could call map() on the Vec to avoid the loop, e.g.:
> 
>   let value = self.map(|v| v.map(|c| c.to_computed_value(context)));
>   computed_value::T(value);
> 
> or if that's a little unclear:
> 
>   match *self {
>       SpecifiedValue::Colors(ref vec) => {
>           computed_value::T(Some(vec.map(|c| c.to_computed_value(context))
>       }
>       SpecifiedValue::None => {
>           computed_value::T(None)
>       }
>   }
> 
> And similarly below in from_computed_value.

This is nice!!! Lesson learned!! Thank you. :-)

> Please keep to 120 columns.  (Servo's tidy will probably fail due to this.)

Okay.

Comment 15

[Jeremy Chen ([e-mail])]

Bug 1358007 should not block our progress here. Remove the dependency and let's move the work here forward.

Comment 16

[Jeremy Chen ([e-mail])]

Attachment: Bug 1348173 - update test expectations for -moz-border-*-colors support.

With the support of -moz-border-*-colors (see previous parts of the patchset),
165 mochitests and 16 reftests could be fixed.

This is just for the convenience of review. Once this patch get r+, I'll merge
this part with other Gecko changes into one single commit, so we could minimize
the risk of breaking autoland repo while doing the Servo/Gecko syncing stuff.

Review commit: https://reviewboard.mozilla.org/r/132776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132776/

Comment 17

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/2-3/

Comment 18

[Jeremy Chen ([e-mail])]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131048/diff/2-3/

Comment 19

[Jeremy Chen ([e-mail])]

Comment on attachment 8860804 [details]
Bug 1348173 - update test expectations for -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/132776/#review135598

This patch is created based on the previous try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19f01a87ca2b.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8860804 [details]
Bug 1348173 - update test expectations for -moz-border-*-colors support.

https://reviewboard.mozilla.org/r/132776/#review135616

Comment 21

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/3-4/

Comment 22

[Jeremy Chen ([e-mail])]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131048/diff/3-4/

Comment 23

[Jeremy Chen ([e-mail])]

Comment on attachment 8860804 [details]
Bug 1348173 - update test expectations for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132776/diff/1-2/

Comment 24

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/4-5/

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8859032 [details]
Bug 1348173 - Stylo: add -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131048/diff/4-5/

Comment 26

[Jeremy Chen ([e-mail])]

Comment on attachment 8860804 [details]
Bug 1348173 - update test expectations for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132776/diff/2-3/

Comment 27

[Jeremy Chen ([e-mail])]

try looks good except for one more unexpected-pass \o/:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc21f69edb2

I'm merging all the gecko changes into one single commit, so we could be ready once servo PR (https://github.com/servo/servo/pull/16586) is merged into autoland!!

Comment 28

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/5-6/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c7c061b959f
stylo: combined gecko side patch for -moz-border-*-colors support. r=heycam

Comment 30

[Iris Hsiao [:ihsiao]]

back out for hazard bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=93943016&repo=autoland&lineNumber=23776
https://queue.taskcluster.net/v1/task/eOs1ie-sQkiNuZeJehvSRw/runs/0/artifacts/public/build/heapWriteHazards.txt.gz

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b620c26083c
Backed out changeset 9c7c061b959f for hazard busted

Comment 32

[Jeremy Chen ([e-mail])]

Will push a quick fix for now, so we don't need to backout servo part as well. I'll file another followup to actually fix this.

Comment 33

[Bobby Holley (:bholley)]

You just need to annotate the outparams in http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/js/src/devtools/rootAnalysis/analyzeHeapWrites.js#153

Comment 34

[Jeremy Chen ([e-mail])]

Attachment: Bug 1348173 - fix stylo bustage.

Review commit: https://reviewboard.mozilla.org/r/133252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133252/

Comment 35

[Jeremy Chen ([e-mail])]

Comment on attachment 8859031 [details]
Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131046/diff/6-7/

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8861295 [details]
Bug 1348173 - fix stylo bustage.

https://reviewboard.mozilla.org/r/133252/#review136044

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d976396b7f7
stylo: combined gecko side patch for -moz-border-*-colors support. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a12d2e2fb05a
fix stylo bustage. r=heycam

Comment 38

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #32)
> Will push a quick fix for now, so we don't need to backout servo part as
> well. I'll file another followup to actually fix this.

Filed Bug 1359313 as a followup.

Comment 39

[Jeremy Chen ([e-mail])]

(In reply to Pulsebot from comment #37)
> https://hg.mozilla.org/integration/autoland/rev/a12d2e2fb05a
> fix stylo bustage. r=heycam

Should be "hazard bustage" not "stylo bustage"...

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d976396b7f7
https://hg.mozilla.org/mozilla-central/rev/a12d2e2fb05a