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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: jeremychen)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
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.
Flags: needinfo?(xidorn+moz)
(Reporter)

Comment 2

a year ago
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
Flags: needinfo?(xidorn+moz)
(Reporter)

Comment 3

a year ago
(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.
Priority: -- → P3
Affects form controls given the UA stylesheet rule.
Priority: P3 → P1
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
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
(Reporter)

Updated

a year ago
Blocks: 1356087
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
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?
Attachment #8859031 - Flags: review?(cam) → review+

Comment 12

a year ago
mozreview-review
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.)
Attachment #8859032 - Flags: review?(cam) → review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 14

a year ago
mozreview-review-reply
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.
Bug 1358007 should not block our progress here. Remove the dependency and let's move the work here forward.
No longer depends on: 1358007
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

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

https://reviewboard.mozilla.org/r/132776/#review135616
Attachment #8860804 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Attachment #8859032 - Attachment is obsolete: true
Attachment #8860804 - Attachment is obsolete: true

Comment 29

a year ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c7c061b959f
stylo: combined gecko side patch for -moz-border-*-colors support. r=heycam

Comment 31

a year ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b620c26083c
Backed out changeset 9c7c061b959f for hazard busted
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.
Flags: needinfo?(jeremychen)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

a year ago
mozreview-review
Comment on attachment 8861295 [details]
Bug 1348173 - fix stylo bustage.

https://reviewboard.mozilla.org/r/133252/#review136044
Attachment #8861295 - Flags: review?(cam) → review+

Comment 37

a year ago
Pushed by jichen@mozilla.com:
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
(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.
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d976396b7f7
https://hg.mozilla.org/mozilla-central/rev/a12d2e2fb05a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.