1341667 - stylo: Figure out why all the W3C masking reftests fail

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Pretty much everything in layout/reftests/w3c-css/submitted/masking/reftest-stylo.list is marked as failing.

Comment 1

[Bobby Holley (:bholley)]

Over to CJ for investigation.

Comment 3

[u459114]

All nsStyleSVGReset attributes at gecko side are empty, so no nsDisplayMask been created.
I guess something wrong in gecko.mako.rs(I did not see style_struct_name="SVGReset" statement in that rs file).

Comment 4

[Bobby Holley (:bholley)]

stylo has the opposite naming convention, so SVG->SVGInherited and SVGReset->SVG.

So you want http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/servo/components/style/properties/gecko.mako.rs#3001

Comment 5

[u459114]

IC, thanks, bholley.

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/servo/components/style/properties/gecko.mako.rs#2288

There are two types of mask-image, image mask and SVG mask
#2288 does not setup image mask correctly and #2288 does not handle SVG mask at all. Working on the patch.

Comment 6

[u459114]

Attachment: Bug 1341667 - Part 1. Keep url in nsStyleImage for local-fragment url.

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

Comment 7

[u459114]

Attachment: Bug 1341667 - Part 2. (Stylo) Correct computed value of mask-image.

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

Comment 8

[u459114]

Comment on attachment 8853079 [details]
Bug 1341667 - Part 1. Keep url in nsStyleImage for local-fragment url.

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

Comment 9

[u459114]

Comment on attachment 8853080 [details]
Bug 1341667 - Part 2. (Stylo) Correct computed value of mask-image.

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

Comment 10

[u459114]

Attachment: Bug 1341667 - Part 3. Enable css-masking reftests in stylo.

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

Comment 11

[u459114]

Comment on attachment 8853079 [details]
Bug 1341667 - Part 1. Keep url in nsStyleImage for local-fragment url.

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

Comment 12

[u459114]

Comment on attachment 8853080 [details]
Bug 1341667 - Part 2. (Stylo) Correct computed value of mask-image.

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

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8853079 [details]
Bug 1341667 - Part 1. Keep url in nsStyleImage for local-fragment url.

https://reviewboard.mozilla.org/r/125166/#review131232

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8853080 [details]
Bug 1341667 - Part 2. (Stylo) Correct computed value of mask-image.

https://reviewboard.mozilla.org/r/125168/#review131236

r=me with this.

::: servo/components/style/properties/longhand/svg.mako.rs:264
(Diff revision 3)
>      pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
>          if input.try(|input| input.expect_ident_matching("none")).is_ok() {
>              Ok(SpecifiedValue::None)
>          } else {
>              let image = try!(Image::parse(context, input));
>              match image {
>                  Image::Url(url_value) => {
> -                    if url_value.is_fragment() {
> -                        Ok(SpecifiedValue::Url(url_value))
> -                    } else {
> -                        Ok(SpecifiedValue::Image(Image::Url(url_value)))
> +                    Ok(SpecifiedValue::Image(Image::Url(url_value)))
> -                    }
> +                }
> -                }
>                  image => Ok(SpecifiedValue::Image(image))
>              }
>          }

Does this change mean we never set a SpecifiedValue::Url value any more, and we also never use computed_value::T::Url?  If so, remove these types from the enums.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8856365 [details]
Bug 1341667 - Part 3. Enable css-masking reftests in stylo.

https://reviewboard.mozilla.org/r/128290/#review131238

Comment 16

[u459114]

Comment on attachment 8853080 [details]
Bug 1341667 - Part 2. (Stylo) Correct computed value of mask-image.

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

Comment 17

[u459114]

Comment on attachment 8856365 [details]
Bug 1341667 - Part 3. Enable css-masking reftests in stylo.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40afcc9cae69
Part 1. Keep url in nsStyleImage for local-fragment url. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7c4ec7e0db
Part 2. Enable css-masking reftests in stylo. r=heycam

Comment 19

[u459114]

Attachment: Bug 1341667 - Part 2. Enable css-masking reftests in stylo.

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

Comment 20

[u459114]

Comment on attachment 8853079 [details]
Bug 1341667 - Part 1. Keep url in nsStyleImage for local-fragment url.

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

Comment 21

[Iris Hsiao [:ihsiao]]

backed this out for causing stylo tests busted in m-i, and this should be landed in autoland

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=79bef3ce0ad383522d0098cbb00ce1bee137a573

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8857332 [details]
Bug 1341667 - Part 2. Enable css-masking reftests in stylo.

https://reviewboard.mozilla.org/r/129312/#review131832

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309d2b0b0f4e
Backed out changeset ff7c4ec7e0db for stylo tests bustage and the patches should be landed to autoland

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b487ab8d26
Part 1. Keep url in nsStyleImage for local-fragment url. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6be0200a4446
Part 2. Enable css-masking reftests in stylo. r=heycam

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1b487ab8d26
https://hg.mozilla.org/mozilla-central/rev/6be0200a4446