Closed Bug 1329088 Opened 7 years ago Closed 7 years ago

stylo: Servo CSS parser probably needs a way to parse "in SVG mode" to support SVG presentation attributes

Categories

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

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

Tracking

()

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

People

(Reporter: bzbarsky, Assigned: jryans)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug 1329088 - Add LengthParsingMode enum to Gecko side.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1329088 - Pass SVG length mode to Stylo.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
See http://searchfox.org/mozilla-central/rev/568e68ade5c6e3d29abba1d1bc25fe87fa1da962/layout/style/nsCSSParser.cpp#18156 and the consumer, which is MappedAttrParser::ParseMappedAttrValue in nsSVGElement.  The upshot is that this:

  <svg fill="stuff">

is more or less equivalent to:

  <svg style="fill: stuff">

but with slightly different rules.  Specifically, as far as I can tell:

1) CSS comments like /* foo */ are not allowed.
2) Unitless lengths _are_ allowed in non-shorthands, and
   the unit is inferred to be pixels.
Note that in SVG 2 we changed the definition of presentation attributes to parse with the "parsing value with grammar" algorithm from css-syntax-3, one of the results of which is that comments would now be allowed.  We still would need to allow unitless lengths, though.

https://svgwg.org/svg2-draft/types.html#presentation-attribute-css-value

Maybe we should remove the checks from nsCSSScanner for comments first and land that, so that we can decouple the compat risk from stylo.
Blocks: 1329093
How certain are we that allowing comments like that is even web-compatible?  What do browsers actually do?
On https://people-mozilla.org/~cmccormack/temp/pres-attr-comment.svg, we show a red rectangle, and Chrome, Safari and Edge all show a green rectangle.
Ah, great.  So yes, we should just do that in a separate bug on Gecko tip directly...
!important is not allowed in attributes but is allowed in styles (unless that's also changed in SVG 2)
Ah, true.  In terms of Gecko's implementation, the "parse a property value" API on the CSS parser that SVG is using excludes the !important bits, because it just parses the value.  The spec is also still excluding !important, afaict, since it's just using a value grammar.

Oh, and for the record, the testcase from comment 3 (in case it ever goes away) is:

  <svg xmlns="http://www.w3.org/2000/svg" fill="red">
    <rect width="100" height="100" fill="/* comment */ green"/>
  </svg>
Assignee: nobody → manishearth
Manish, is there any work left to be done here?
Flags: needinfo?(manishearth)
Priority: -- → P2
No, this still needs to be done. Will block shipping svg, but is not high priority.
Flags: needinfo?(manishearth)
Priority: P2 → P3
Blocks: stylo-svg
Bumping the priority given bug 1352284.
Assignee: manishearth → jryans
Priority: P3 → P1
Comment on attachment 8858213 [details]
Bug 1329088 - Centralize ParserContext for tests.

https://reviewboard.mozilla.org/r/130166/#review132806

Seemed very mechanical, so I didn't go through it in a lot of detail, lmk if there's something that would require special attention.

Thanks _a lot_ for this! :)

::: servo/tests/unit/style/properties/mod.rs:10
(Diff revision 1)
> +use cssparser::Parser;
> +use media_queries::CSSErrorReporterTest;
> +use style::parser::ParserContext;
> +use style::stylesheets::{CssRuleType, Origin};
> +
> +fn parse<T, F: Fn(&ParserContext, &mut Parser) -> Result<T, ()>>(f: F, s: &str) -> Result<T, ()> {

I would have probably reverted the argument order fwiw, but it's fine like it is, thanks for doing this!
Attachment #8858213 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132808

r=me with those renamed for clarity.

::: servo/components/style/parser.rs:16
(Diff revision 1)
>  use style_traits::OneOrMoreCommaSeparated;
>  use stylesheets::{CssRuleType, Origin, UrlExtraData};
>  
> +/// The mode to use when parsing lengths.
> +#[derive(PartialEq, Eq, Copy, Clone)]
> +pub enum LengthMode {

LengthParsingMode would be clearer I think.

::: servo/components/style/parser.rs:37
(Diff revision 1)
>      /// An error reporter to report syntax errors.
>      pub error_reporter: &'a ParseErrorReporter,
>      /// The current rule type, if any.
>      pub rule_type: Option<CssRuleType>,
> +    /// The mode to use when parsing lengths.
> +    pub length_mode: LengthMode,

similarly, `length_parsing_mode`.
Attachment #8858214 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8858215 [details]
Bug 1329088 - Add LengthParsingMode enum to Gecko side.

https://reviewboard.mozilla.org/r/130170/#review132804

r=me with the equivalent rename.
Attachment #8858215 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132810

::: servo/components/style/values/specified/length.rs:555
(Diff revision 1)
>                        -> Result<Length, ()> {
>          match try!(input.next()) {
>              Token::Dimension(ref value, ref unit) if num_context.is_ok(value.value) =>
>                  Length::parse_dimension(context, value.value, unit),
>              Token::Number(ref value) if value.value == 0. => Ok(Length::zero()),
> +            Token::Number(ref value) if value.value != 0. => {

Oh, also, another nit. I believe instead of having these excluding conditions we should do something like:

```
Token::Number(ref value) => {
    if value.value != 0. && context.length_parsing_mode != LengthParsingMode::SVG {
        return Err(());
    }
    return Ok(Length::NoCalc(..));
}
```

For extra niceness, you could implement a method on LenghtParsingMode like:

```
impl LengthParsingMode {
    fn allows_unitless_lengths(&self) -> bool {
        *self == LengthParsingMode::SVG
    }
}
```

But I don't know how useful would that be in practice, so your choice :)
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132812

::: servo/tests/unit/style/viewport.rs:294
(Diff revision 1)
>  
>  #[test]
>  fn constrain_viewport() {
>      let url = ServoUrl::parse("http://localhost").unwrap();
>      let reporter = CSSErrorReporterTest;
> -    let context = ParserContext::new(Origin::Author, &url, &reporter, Some(CssRuleType::Viewport));
> +    let context = ParserContext::new(Origin::Author, &url, &reporter, Some(CssRuleType::Viewport), LengthMode::Default);

Also, perhaps one last nit (no action required if you don't want to). Since this is mostly going to be used for once, perhaps would be nice to just use `LengthParsingMode::Default` inside `ParserContext::new` and add another `ParserContext::with_length_parsing_mode(..)` or something, but your call.
Comment on attachment 8858216 [details]
Bug 1329088 - Expose CSS length mode in Stylo glue.

https://reviewboard.mozilla.org/r/130172/#review132814
Attachment #8858216 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8858217 [details]
Bug 1329088 - Pass SVG length mode to Stylo.

https://reviewboard.mozilla.org/r/130174/#review132816

\o/
Attachment #8858217 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132808

> LengthParsingMode would be clearer I think.

Okay!  I wasn't sure if that would feel too redundant since it's in the `parser` module / `ParserContext` struct, so I didn't do it at first.  I don't really have a preference, so seems fine to me. :)
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132812

> Also, perhaps one last nit (no action required if you don't want to). Since this is mostly going to be used for once, perhaps would be nice to just use `LengthParsingMode::Default` inside `ParserContext::new` and add another `ParserContext::with_length_parsing_mode(..)` or something, but your call.

Hmm, I think I'll leave this alone for now.  If we end up adding a lot more fields to `ParserContext`, we can think about a better API that lets you get everything as default, but then tweak just one after that in a general way.
Comment on attachment 8858214 [details]
Bug 1329088 - SVG length parsing mode.

https://reviewboard.mozilla.org/r/130168/#review132810

> Oh, also, another nit. I believe instead of having these excluding conditions we should do something like:
> 
> ```
> Token::Number(ref value) => {
>     if value.value != 0. && context.length_parsing_mode != LengthParsingMode::SVG {
>         return Err(());
>     }
>     return Ok(Length::NoCalc(..));
> }
> ```
> 
> For extra niceness, you could implement a method on LenghtParsingMode like:
> 
> ```
> impl LengthParsingMode {
>     fn allows_unitless_lengths(&self) -> bool {
>         *self == LengthParsingMode::SVG
>     }
> }
> ```
> 
> But I don't know how useful would that be in practice, so your choice :)

Okay, I made both changes, thanks! :)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/535e4a20a290
Add LengthParsingMode enum to Gecko side. r=emilio
https://hg.mozilla.org/integration/autoland/rev/426b7ff4ce19
Pass SVG length mode to Stylo. r=emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: