Closed Bug 1353191 Opened 7 years ago Closed 7 years ago

Stylo: Disable viewport units in @page context

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(1 file, 5 obsolete files)

Bug 1353191 - Update Stylo expectations.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
In bug 811391, Gecko's handling for units inside an @page context was changed to ignore any properties that use viewport units (vw, vh, vmin, vmax).

We should apply this same behavior in Stylo.
This sounds like well-scoped followup work, probably worth knocking out while we're here.
Assignee: nobody → jryans
Priority: -- → P1
Comment on attachment 8857322 [details]
Bug 1353191 - Pull rule_type into ParserContext.

https://reviewboard.mozilla.org/r/129290/#review131838

Looks sensible, r=me % nits and that parameter removed, thanks!

::: servo/components/style/properties/properties.mako.rs:982
(Diff revision 1)
>      /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser,
>      /// we only set them here so that we don't have to reallocate
>      pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser,
> -                 in_keyframe_block: bool, rule_type: CssRuleType)
> +                 in_keyframe_block: bool)
>                   -> Result<ParsedDeclaration, PropertyDeclarationParseError> {
> -        debug_assert!(rule_type == CssRuleType::Keyframe ||
> +        if let Some(r) = context.rule_type {

I guess you can do:

```
let rule_type = context.rule_type.expect("Declarations are only expected inside a keyframe, page or style rule");

debug_assert!(rule_type == ...);
```

Also, looks like we don't need the `in_keyframe_block` parameter now, do we? (we can check for RuleType::Keyframe).

::: servo/components/style/stylesheets.rs:1094
(Diff revision 1)
>          match prelude {
>              AtRulePrelude::FontFace => {
>                  Ok(CssRule::FontFace(Arc::new(self.shared_lock.wrap(
>                      parse_font_face_block(self.context, input).into()))))
>              }
>              AtRulePrelude::Media(media_queries) => {
Attachment #8857322 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8857323 [details]
Bug 1353191 - Set rule_type in context when descending into any rule.

https://reviewboard.mozilla.org/r/129292/#review131840

This should probably be merged with the previous patch, shouldn't it?
Attachment #8857323 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8857324 [details]
Bug 1353191 - Check context to test keyframe rule_type.

https://reviewboard.mozilla.org/r/129294/#review131842

Oh, here it is, much nicer :)
Attachment #8857324 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8857325 [details]
Bug 1353191 - Pass ParserContext down to lengths.

https://reviewboard.mozilla.org/r/129296/#review131844
Attachment #8857325 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8857326 [details]
Bug 1353191 - Disable viewport units in @page rules.

https://reviewboard.mozilla.org/r/129298/#review131848

::: servo/components/style/values/specified/length.rs:271
(Diff revision 1)
>  
>  impl NoCalcLength {
>      /// Parse a given absolute or relative dimension.
> -    pub fn parse_dimension(_context: &ParserContext, value: CSSFloat, unit: &str) -> Result<NoCalcLength, ()> {
> +    pub fn parse_dimension(context: &ParserContext, value: CSSFloat, unit: &str) -> Result<NoCalcLength, ()> {
> +        let mut in_page_rule = false;
> +        if let Some(r) = context.rule_type {

I'd actually add an accessor on the context, like `fn rule_type(&self) -> CssRuleType` that `expect()`s a rule type. Or do we expect parsers without a rule type here?

If so,

```
let in_page_rule = context.rule_type.map_or(false, |type| type == RuleType::Page);
```

Might be a bit cleaner.
Attachment #8857326 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8857327 [details]
Bug 1353191 - Update Stylo expectations.

https://reviewboard.mozilla.org/r/129300/#review131850

::: layout/style/test/stylo-failures.md:392
(Diff revision 1)
>      * ... `rect(1, ` [2]
>  * test_pseudoelement_parsing.html: support parsing some pseudo-classes on some pseudo-elements [5]
>  * Unit should be preserved after parsing servo/servo#15346
>    * test_units_length.html [5]
>    * test_units_time.html [1]
> -  * test_page_parser.html `192px` [8]
> +  * test_page_parser.html `192px` [16]

Why are there more failures here?
Attachment #8857327 - Flags: review?(emilio+bugs)
Comment on attachment 8857327 [details]
Bug 1353191 - Update Stylo expectations.

https://reviewboard.mozilla.org/r/129300/#review131856

We discussed this offline, there are more units that need to get disabled (inches). In the meantime, this is fine :)
Attachment #8857327 - Flags: review+
Comment on attachment 8857322 [details]
Bug 1353191 - Pull rule_type into ParserContext.

https://reviewboard.mozilla.org/r/129290/#review131838

> I guess you can do:
> 
> ```
> let rule_type = context.rule_type.expect("Declarations are only expected inside a keyframe, page or style rule");
> 
> debug_assert!(rule_type == ...);
> ```
> 
> Also, looks like we don't need the `in_keyframe_block` parameter now, do we? (we can check for RuleType::Keyframe).

I'll add a `rule_type` getter for this, like you suggest in a later patch here.
Comment on attachment 8857323 [details]
Bug 1353191 - Set rule_type in context when descending into any rule.

https://reviewboard.mozilla.org/r/129292/#review131840

Perhaps so?  The previous one is just a transformation, moving existing usages.  This one adds the remaining rule definitions at other boundaries.
Comment on attachment 8857326 [details]
Bug 1353191 - Disable viewport units in @page rules.

https://reviewboard.mozilla.org/r/129298/#review131848

> I'd actually add an accessor on the context, like `fn rule_type(&self) -> CssRuleType` that `expect()`s a rule type. Or do we expect parsers without a rule type here?
> 
> If so,
> 
> ```
> let in_page_rule = context.rule_type.map_or(false, |type| type == RuleType::Page);
> ```
> 
> Might be a bit cleaner.

Good idea, I added a getter like this.
Comment on attachment 8857326 [details]
Bug 1353191 - Disable viewport units in @page rules.

https://reviewboard.mozilla.org/r/129298/#review131848

> Good idea, I added a getter like this.

Actually for this case, it can be empty, as we parse length in some media query cases, where there's no obvious rule node.  So, I'll use the `map_or` version.
Servo PR merged.

I was going to land an expectations update here as the last step, but a Servo contributor also landed https://github.com/servo/servo/pull/16229 to preserve units, so the test expectations shifted a bit.

The combination of updates in:

https://hg.mozilla.org/mozilla-central/rev/970eb52865f5
https://hg.mozilla.org/mozilla-central/rev/bb3978c4c32c

covers expectation changes needed here.
Blocks: stylo-style-mochitest
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: