Closed Bug 1348173 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

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

People

(Reporter: xidorn, Assigned: chenpighead)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1348173 - stylo: combined gecko side patch for -moz-border-*-colors support.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1348173 - fix stylo bustage.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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)
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)
(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
Blocks: 1356087
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 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+
Depends on: 1358007
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 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 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 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+
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!!
Attachment #8859032 - Attachment is obsolete: true
Attachment #8860804 - Attachment is obsolete: true
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
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)
Blocks: 1359313
Attachment #8861295 - Flags: review?(cam)
Comment on attachment 8861295 [details]
Bug 1348173 - fix stylo bustage.

https://reviewboard.mozilla.org/r/133252/#review136044
Attachment #8861295 - Flags: review?(cam) → review+
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"...
https://hg.mozilla.org/mozilla-central/rev/8d976396b7f7
https://hg.mozilla.org/mozilla-central/rev/a12d2e2fb05a
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: