Closed Bug 1369588 Opened 7 years ago Closed 7 years ago

stylo: Parse negative lengths

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

Bug 1369588 - Update reftest expectations for negative length handing for SMIL.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
We have parsing for negative numbers thanks to bug 1357295, but I notice that we fail to parse a stroke-width of -20px since it is a length.

The stack up to the point where we fail to parse is:

 	xul.dll!style::values::specified::length::LengthOrPercentage::parse_internal(cssparser::parser::Parser * context, style_traits::values::specified::AllowedLengthType input, style::values::specified::AllowQuirks num_context) Line 835	Unknown
 	xul.dll!style::values::specified::length::LengthOrPercentage::parse_non_negative_quirky(cssparser::parser::Parser * context, style::values::specified::AllowQuirks input) Line 852	Unknown
 	xul.dll!style::values::specified::length::LengthOrPercentage::parse_non_negative(cssparser::parser::Parser * context) Line 843	Unknown
 	xul.dll!style::values::Either<style::values::specified::Number, style::values::specified::length::LengthOrPercentage>::parse_non_negative(cssparser::parser::Parser * context) Line 1202	Unknown
 	xul.dll!style::properties::longhands::stroke_width::parse(style::parser::ParserContext * context, cssparser::parser::Parser * input) Line 60021	Unknown
 	xul.dll!style::properties::longhands::stroke_width::parse_specified(style::parser::ParserContext * context, cssparser::parser::Parser * input) Line 60096	Unknown
 	xul.dll!style::properties::longhands::stroke_width::parse_declared(style::parser::ParserContext * context, cssparser::parser::Parser * input) Line 60104	Unknown
 	xul.dll!style::properties::PropertyDeclaration::parse_into(style::properties::PropertyId declarations, style::parser::ParserContext * id, cssparser::parser::Parser * context) Line 149974	Unknown
 	xul.dll!style::properties::declaration_block::parse_one_declaration_into::{{closure}}(closure parser, cssparser::parser::Parser *) Line 774	Unknown
 	xul.dll!cssparser::parser::Parser::parse_entirely<closure,()>(closure self) Line 347	Unknown
 	xul.dll!style::properties::declaration_block::parse_one_declaration_into(style::properties::SourcePropertyDeclaration * declarations, style::properties::PropertyId id, &str input, style::gecko_bindings::sugar::refptr::RefPtr<style::gecko_bindings::structs::root::mozilla::URLExtraData> * url_data, style::error_reporting::&ParseErrorReporter error_reporter, style::parser::ParsingMode parsing_mode, style::context::QuirksMode quirks_mode) Line 773	Unknown
 	xul.dll!geckoservo::glue::parse_property_into(style::properties::SourcePropertyDeclaration * declarations, style::properties::PropertyId property_id, nsstring_vendor::nsACString * value, style::gecko_bindings::structs::root::mozilla::URLExtraData * data, ..., style::context::QuirksMode parsing_mode) Line 1496	Unknown
 	xul.dll!geckoservo::glue::Servo_ParseProperty(style::gecko_bindings::structs::root::nsCSSPropertyID property, nsstring_vendor::nsACString * value, style::gecko_bindings::structs::root::mozilla::URLExtraData * data, ..., style::gecko_bindings::structs::root::nsCompatibility parsing_mode) Line 1515	Unknown
CCing Jeremy since I guess bug 1367327 regressed bug 1357295.
Summary: stylo: Parse negative lengths → stylo: Parse negative lengths for stroke-width and stroke-dashoffset
Summary: stylo: Parse negative lengths for stroke-width and stroke-dashoffset → stylo: Parse negative lengths
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8876958 [details]
Bug 1369588 - Move ParingMode into style_traits.

https://reviewboard.mozilla.org/r/148276/#review152656

::: toolkit/library/gtest/rust/Cargo.lock:905
(Diff revision 1)
>  [[package]]
>  name = "style_traits"
>  version = "0.0.1"
>  dependencies = [
>   "app_units 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
> + "bitflags 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)",

I don't know that this change will be automatically done by revendor, but I guess so.
Comment on attachment 8876961 [details]
Bug 1369588 - Update reftest expectations for negative length handing for SMIL.

https://reviewboard.mozilla.org/r/148282/#review152658
Attachment #8876961 - Flags: review?(bbirtles) → review+
Comment on attachment 8876960 [details]
Bug 1369588 - AllowedNumericType.is_ok() takes ParingMode as well.

https://reviewboard.mozilla.org/r/148280/#review152790

::: servo/components/style/values/specified/mod.rs:370
(Diff revision 1)
>                                          clamping_mode: AllowedNumericType)
>                                          -> Result<Self, ParseError<'i>> {
> +        use style_traits::PARSING_MODE_DEFAULT;
> +
>          match input.next() {
> -            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(value.value) => {
> +            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(PARSING_MODE_DEFAULT, value.value) => {

Why not context.parsing_mode? We don't want to allow out-of-range values for this stuff in this case?
Comment on attachment 8876958 [details]
Bug 1369588 - Move ParingMode into style_traits.

https://reviewboard.mozilla.org/r/148276/#review152792

::: commit-message-e3638:1
(Diff revision 1)
> +Bug 1369588 - Move ParingMode into style_traits. r?emilio

nit: s/ParingMode/ParsingMode (also in the other commits).
Attachment #8876958 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8876959 [details]
Bug 1369588 - Make AllowedLengthType.is_ok() returning true if parsing mode allows all numeric values.

https://reviewboard.mozilla.org/r/148278/#review152796

::: servo/components/style_traits/viewport.rs:148
(Diff revision 1)
> +        use PARSING_MODE_DEFAULT;
>          use cssparser::Token;
> +        use values::specified::AllowedLengthType::NonNegative;
>  
>          match try!(input.next()) {
> -            Token::Percentage(ref value) if AllowedLengthType::NonNegative.is_ok(value.unit_value) =>
> +            Token::Percentage(ref value) if NonNegative.is_ok(PARSING_MODE_DEFAULT, value.unit_value) =>

Let's make this take the context and use it instead, since it's only called in a few places.
Attachment #8876959 - Flags: review?(emilio+bugs) → review+
Priority: -- → P2
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8876960 [details]
> Bug 1369588 - AllowedNumericType.is_ok() takes ParingMode as well.
> 
> https://reviewboard.mozilla.org/r/148280/#review152790
> 
> ::: servo/components/style/values/specified/mod.rs:370
> (Diff revision 1)
> >                                          clamping_mode: AllowedNumericType)
> >                                          -> Result<Self, ParseError<'i>> {
> > +        use style_traits::PARSING_MODE_DEFAULT;
> > +
> >          match input.next() {
> > -            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(value.value) => {
> > +            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(PARSING_MODE_DEFAULT, value.value) => {
> 
> Why not context.parsing_mode? We don't want to allow out-of-range values for
> this stuff in this case?

Right, this part is for time duration, SMIL does not animate time duration.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Comment on attachment 8876959 [details]
> Bug 1369588 - Make AllowedLengthType.is_ok() returning true if parsing mode
> allows all numeric values.
> 
> https://reviewboard.mozilla.org/r/148278/#review152796
> 
> ::: servo/components/style_traits/viewport.rs:148
> (Diff revision 1)
> > +        use PARSING_MODE_DEFAULT;
> >          use cssparser::Token;
> > +        use values::specified::AllowedLengthType::NonNegative;
> >  
> >          match try!(input.next()) {
> > -            Token::Percentage(ref value) if AllowedLengthType::NonNegative.is_ok(value.unit_value) =>
> > +            Token::Percentage(ref value) if NonNegative.is_ok(PARSING_MODE_DEFAULT, value.unit_value) =>
> 
> Let's make this take the context and use it instead, since it's only called
> in a few places.

Actually I wanted to do it but.. This file is in an independent crate, style_traits, if we take the context, ParserContext actually, here, it will be more complex I guess? That's why I did move just a ParsingMode into this crate.
Attachment #8876958 - Attachment is obsolete: true
Attachment #8876959 - Attachment is obsolete: true
Attachment #8876960 - Attachment is obsolete: true
Attachment #8876960 - Flags: review?(emilio+bugs)
Oh, gosh. A patch did not get r+..
D'oh! MozReview is confused...
Comment on attachment 8877342 [details]
Bug 1369588 - Move ParsingMode into style_traits.

https://reviewboard.mozilla.org/r/148708/#review153168

This patch was previously r+'d
Attachment #8877342 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8877343 [details]
Bug 1369588 - Make AllowedLengthType.is_ok() returning true if parsing mode allows all numeric values.

https://reviewboard.mozilla.org/r/148710/#review153170

::: servo/components/style_traits/viewport.rs:148
(Diff revision 1)
> +        use PARSING_MODE_DEFAULT;
>          use cssparser::Token;
> +        use values::specified::AllowedLengthType::NonNegative;
>  
>          match try!(input.next()) {
> -            Token::Percentage(ref value) if AllowedLengthType::NonNegative.is_ok(value.unit_value) =>
> +            Token::Percentage(ref value) if NonNegative.is_ok(PARSING_MODE_DEFAULT, value.unit_value) =>

Please leave a `TODO` for this. We probably want to move most of this stuff to `style::stylesheets::viewport_rule` anyway.
Attachment #8877343 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8877344 [details]
Bug 1369588 - AllowedNumericType.is_ok() takes ParingMode as well.

https://reviewboard.mozilla.org/r/148712/#review153172

Ok, r=me

::: servo/components/style/values/specified/mod.rs:370
(Diff revision 1)
>                                          clamping_mode: AllowedNumericType)
>                                          -> Result<Self, ParseError<'i>> {
> +        use style_traits::PARSING_MODE_DEFAULT;
> +
>          match input.next() {
> -            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(value.value) => {
> +            Ok(Token::Dimension(ref value, ref unit)) if clamping_mode.is_ok(PARSING_MODE_DEFAULT, value.value) => {

Please leave a note here about why passing `PARSING_MODE_DEFAULT`.

I guess this one-off makes the `is_ok(ParsingMode, value)` strategy a bit less clean, but it's probably acceptable, as long as it's properly documented.
Attachment #8877344 - Flags: review?(emilio+bugs) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> Comment on attachment 8877343 [details]
> Bug 1369588 - Make AllowedLengthType.is_ok() returning true if parsing mode
> allows all numeric values.
> 
> https://reviewboard.mozilla.org/r/148710/#review153170
> 
> ::: servo/components/style_traits/viewport.rs:148
> (Diff revision 1)
> > +        use PARSING_MODE_DEFAULT;
> >          use cssparser::Token;
> > +        use values::specified::AllowedLengthType::NonNegative;
> >  
> >          match try!(input.next()) {
> > -            Token::Percentage(ref value) if AllowedLengthType::NonNegative.is_ok(value.unit_value) =>
> > +            Token::Percentage(ref value) if NonNegative.is_ok(PARSING_MODE_DEFAULT, value.unit_value) =>
> 
> Please leave a `TODO` for this. We probably want to move most of this stuff
> to `style::stylesheets::viewport_rule` anyway.

I will leave a TODO comment there.  Thank you!  Your review is always easy to understand for a man (actually it's me) who is not familiar with servo code. I've never thought about moving this stuff into style/style_sheets. :)
Attachment #8877342 - Attachment is obsolete: true
Attachment #8877343 - Attachment is obsolete: true
Attachment #8877344 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a44e9e207e6
Update reftest expectations for negative length handing for SMIL. r=birtles
https://hg.mozilla.org/mozilla-central/rev/4a44e9e207e6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: