Closed Bug 1345206 Opened 6 years ago Closed 6 years ago

stylo: Implement @page support

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: bholley, Assigned: jryans)

References

Details

Attachments

(4 files, 3 obsolete files)

Bug 1345206 - Correct faulty assertions in test_page_parser.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1345206 - Extract CSSPageRule base class.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1345206 - Create ServoPageRule.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1345206 - Wire up bindings for @page rule.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Apparently gecko supports a very stripped-down version of @page. We need to add minimal parser support to servo, and then add some glue.
Priority: -- → P2
It’s stripped down in the sense that Gecko probably only supports the set of functionality in CSS 2 but not anything added in CSS Paged Media Module Level 3. (This is from memory, though, so whoever does the implementation should double check what Gecko does.)

https://drafts.csswg.org/css2/page.html
https://drafts.csswg.org/css-page/
jryans is going to work on this \o/
Assignee: nobody → jryans
Priority: P2 → P1
Looking at Gecko's @page support, it seems to indeed be pretty minimal, as it's only a portion of @page from CSS 2 (https://drafts.csswg.org/css2/page.html).  Page selectors (:first, :left, :right) are not supported.

Gecko accepts the following properties inside @page (the complete list from CSS 2):

margin
margin-top
margin-right
margin-bottom
margin-left

In addition, the following properties from CSS Logical Properties Level 1 are accepted in @page, even though that spec lists them as "Media: visual":

margin-block-start
margin-block-end
margin-inline-start
margin-inline-end
> even though that spec lists them as "Media: visual"

Print is a visual medium, no?
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #4)
> > even though that spec lists them as "Media: visual"
> 
> Print is a visual medium, no?

Ah, my mistake, I wasn't sure how "visual" was defined for CSS specs.  But now I see there's a table[1] showing that "visual" means print, screen, and others too.  So, it seems like Gecko is following the spec then.

Thanks for pointing that out. :)

[1]: https://www.w3.org/TR/CSS22/media.html#media-groups
I've made good progress on this so far.

Done:

* @page parsing / serialization
* CSSPageRule objects wired up to Stylo

Not Done:

* Gecko expects viewport units to be ignored inside @page (bug 811391)

Some tests failures with @page remain, but they appear to be related to a known issue where units are all converted to px after parsing ( servo/servo#15346 ).
Awesome, thanks jryans!
I moved the remaining step of disabling viewport units to bug 1353191, as it seems like a separate task from basic @page support.
Attachment #8854611 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854612 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854613 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854614 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854615 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854616 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8854617 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Comment on attachment 8854611 [details]
Bug 1345206 - Correct faulty assertions in test_page_parser.

https://reviewboard.mozilla.org/r/126558/#review129200

Thanks for fixing this. r=me with the comment addressed.

::: layout/style/test/test_page_parser.html:59
(Diff revision 2)
>      } catch (e) {
>        ok(e.name == "SyntaxError"
>           && e instanceof DOMException
>           && e.code == DOMException.SYNTAX_ERR
>           && !('expected' in testset[curTest]),
>           testset[curTest].rule + " syntax error thrown", e);
>      }

You should remove this try-catch block, since `@page { something invalid }` wouldn't raise a syntax error. The syntax error is only raised if there is no rule inserted.

::: layout/style/test/test_page_parser.html:91
(Diff revision 2)
>          if (sheet.cssRules.length == 0) {
>            is(sheet.cssRules.length, 0,
>               testset[curTest].rule + " rule count (0)");
>          } else {
>            is(sheet.cssRules.length, 1,
> -             testset[curTest].rule + " rule count (1 non-page)");
> -          isnot(sheet.cssRules[0].type, CSSRule.PAGE_RULE,
> -                testset[curTest].rule + " rule type (1 non-page)");
> +             testset[curTest].rule + " rule count (1)");
> +          is(sheet.cssRules[0].type, CSSRule.PAGE_RULE,
> +             testset[curTest].rule + " rule type");
> +          is(Object.keys(sheet.cssRules[0].style).length, 0,
> +             testset[curTest].rule + " rule has no properties");
>          }

Like above, there should always be one `@page` rule, which is empty. Please remove unrelated branches.
Attachment #8854611 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854612 [details]
Bug 1345206 - Servo parsing / serialization for @page.

https://reviewboard.mozilla.org/r/126560/#review129204

::: servo/components/style/stylesheets.rs:578
(Diff revision 2)
> +        try!(dest.write_str("@page { "));
> +        let declaration_block = self.0.read_with(guard);
> +        try!(declaration_block.to_css(dest));
> +        if declaration_block.declarations().len() > 0 {
> +            try!(write!(dest, " "));
> +        }

You can use `?` syntax to simplify the code here.
Attachment #8854612 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854613 [details]
Bug 1345206 - Ignore non-margin properties in @page rule.

https://reviewboard.mozilla.org/r/126562/#review129208

::: servo/components/style/properties/properties.mako.rs:982
(Diff revision 2)
>      /// 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)
> +                 in_keyframe_block: bool, rule_type: CssRuleType)
>                   -> Result<ParsedDeclaration, PropertyDeclarationParseError> {
>          match id {

Probably you can add a `debug_assert!` here that `rule_type` can only be one of style, keyframe, and page.

::: servo/components/style/properties/properties.mako.rs:1008
(Diff revision 2)
> +                            match rule_type {
> +                                CssRuleType::Page => return Err(PropertyDeclarationParseError::NotAllowedInPageRule),
> +                                _ => {},
> +                            }

You can have `CssRuleType` derive `PartialEq` and `Eq` so that you can use `if` here which should make the code a little more compact.

::: servo/components/style/properties/properties.mako.rs:1042
(Diff revision 2)
> +                        match rule_type {
> +                            CssRuleType::Page => return Err(PropertyDeclarationParseError::NotAllowedInPageRule),
> +                            _ => {},
> +                        }

ditto.

::: servo/components/style/supports.rs:215
(Diff revision 2)
>              id
>          } else {
>              return false
>          };
>          let mut input = Parser::new(&self.val);
> -        let res = ParsedDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false);
> +        let res = ParsedDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false, CssRuleType::Supports);

This should probably use `CssRuleType::Style` because `@supports` is meant to check whether the corresponding property and value are supported in normal declarations. Although there is no distinguish between `Supports` and `Style` in the parsing function at the moment, it may still be worth not risking that.
Attachment #8854613 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854614 [details]
Bug 1345206 - Extract CSSPageRule base class.

https://reviewboard.mozilla.org/r/126564/#review129214

::: layout/style/CSSPageRule.h:19
(Diff revision 2)
> +class CSSPageRule : public css::Rule,
> +                    public nsIDOMCSSPageRule

```c++
class CSSPageRule : public css::Rule
                  , public nsIDOMCSSPageRule
```

::: layout/style/CSSPageRule.h:40
(Diff revision 2)
> +  uint16_t Type() const override {
> +    return nsIDOMCSSRule::PAGE_RULE;
> +  }

This can be written in one line, and you can use `final` instead of `override` here.

::: layout/style/nsCSSRules.cpp:2196
(Diff revision 2)
>    return false;
>  }
>  
>  // QueryInterface implementation for nsCSSPageRule
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(nsCSSPageRule)
>    NS_INTERFACE_MAP_ENTRY(nsIDOMCSSPageRule)

Since you have add this entry to `dom::CSSPageRule`, you can probably remove it here.
Attachment #8854614 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854615 [details]
Bug 1345206 - Create ServoPageRule.

https://reviewboard.mozilla.org/r/126566/#review129222

Mostly looks good.

::: layout/style/ServoPageRule.h:58
(Diff revision 2)
> +  bool IsCCLeaf() const override;
> +
> +  RawServoPageRule* Raw() const { return mRawRule; }
> +
> +  // WebIDL interface
> +  void GetCssTextImpl(nsAString& aCssText) const override;
> +  nsICSSDeclaration* Style() override;
> +
> +  // Methods of mozilla::css::Rule
> +  already_AddRefed<css::Rule> Clone() const override;
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> +    const override;

You can actually use `final` instead of `override` for all these methods... although this probably doesn't matter a lot.

::: layout/style/ServoPageRule.cpp:72
(Diff revision 2)
> +  ServoPageRule* rule = Rule();
> +  if (RefPtr<ServoStyleSheet> sheet = rule->GetStyleSheet()->AsServo()) {
> +    nsCOMPtr<nsIDocument> doc = sheet->GetAssociatedDocument();
> +    mozAutoDocUpdate updateBatch(doc, UPDATE_STYLE, true);
> +    if (aDecl != mDecls) {
> +      RefPtr<ServoDeclarationBlock> decls = aDecl->AsServo();
> +      Servo_PageRule_SetStyle(rule->Raw(), decls->Raw());
> +      mDecls = decls.forget();
> +    }
> +    if (doc) {
> +      doc->StyleRuleChanged(sheet, rule);
> +    }
> +  }

`SetCSSDeclaration` of `@page` rule doesn't seem to need to be this complicated. Consider taking `nsCSSPageStyleDeclaration::SetCSSDeclaration` as the base.

It actually reminds me the issue that our `ServoStyleRule::SetCSSDeclaration` is probably wrong that it doesn't seem to call `SetOwningRule` on the declarations. It probably doesn't matter for now, but we may still want to do it.
Attachment #8854615 - Flags: review?(xidorn+moz)
Comment on attachment 8854616 [details]
Bug 1345206 - Servo glue for @page rule.

https://reviewboard.mozilla.org/r/126568/#review129224
Attachment #8854616 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854616 [details]
Bug 1345206 - Servo glue for @page rule.

https://reviewboard.mozilla.org/r/126568/#review129226

::: servo/components/style/gecko/arc_types.rs:12
(Diff revision 2)
>  use gecko_bindings::bindings::{RawServoStyleSheet, RawServoStyleRule, RawServoImportRule};
> -use gecko_bindings::bindings::{ServoComputedValues, ServoCssRules};
> +use gecko_bindings::bindings::{RawServoPageRule, ServoComputedValues, ServoCssRules};

This would likely trigger a lint error in Servo CI (which you can check with `./mach test-tidy` once you generate the patch for Servo). You probably want to add `RawServoPageRule` to a `use` statement above.
Comment on attachment 8854617 [details]
Bug 1345206 - Wire up bindings for @page rule.

https://reviewboard.mozilla.org/r/126570/#review129230

The last three commits should probably be one commit... but that probably doesn't matter.
Attachment #8854617 - Flags: review?(xidorn+moz) → review+
You can also do

cd servo/ && ./mach test-tidy --stylo

From mc.
Comment on attachment 8854616 [details]
Bug 1345206 - Servo glue for @page rule.

https://reviewboard.mozilla.org/r/126568/#review129226

> This would likely trigger a lint error in Servo CI (which you can check with `./mach test-tidy` once you generate the patch for Servo). You probably want to add `RawServoPageRule` to a `use` statement above.

Ah, good catch, and thanks for the tip :bholley! :) I'll fix it up.
Comment on attachment 8854611 [details]
Bug 1345206 - Correct faulty assertions in test_page_parser.

https://reviewboard.mozilla.org/r/126558/#review129200

> You should remove this try-catch block, since `@page { something invalid }` wouldn't raise a syntax error. The syntax error is only raised if there is no rule inserted.

Ah, good point! :) I'll clean it up.

> Like above, there should always be one `@page` rule, which is empty. Please remove unrelated branches.

Good catch, I'll remove the extra branchces and deduplicate the test assertions that are shared.
Comment on attachment 8854612 [details]
Bug 1345206 - Servo parsing / serialization for @page.

https://reviewboard.mozilla.org/r/126560/#review129204

> You can use `?` syntax to simplify the code here.

Good catch, fixed!
Comment on attachment 8854613 [details]
Bug 1345206 - Ignore non-margin properties in @page rule.

https://reviewboard.mozilla.org/r/126562/#review129208

> Probably you can add a `debug_assert!` here that `rule_type` can only be one of style, keyframe, and page.

Good idea, added the assert.

> You can have `CssRuleType` derive `PartialEq` and `Eq` so that you can use `if` here which should make the code a little more compact.

Ah yeah, I thought about that but wasn't sure if it was the right thing.  Thanks, added!

> This should probably use `CssRuleType::Style` because `@supports` is meant to check whether the corresponding property and value are supported in normal declarations. Although there is no distinguish between `Supports` and `Style` in the parsing function at the moment, it may still be worth not risking that.

Okay, sounds like a reasonable arugment to me, changed.
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #29)
> ::: layout/style/ServoPageRule.cpp:72
> (Diff revision 2)
> > +  ServoPageRule* rule = Rule();
> > +  if (RefPtr<ServoStyleSheet> sheet = rule->GetStyleSheet()->AsServo()) {
> > +    nsCOMPtr<nsIDocument> doc = sheet->GetAssociatedDocument();
> > +    mozAutoDocUpdate updateBatch(doc, UPDATE_STYLE, true);
> > +    if (aDecl != mDecls) {
> > +      RefPtr<ServoDeclarationBlock> decls = aDecl->AsServo();
> > +      Servo_PageRule_SetStyle(rule->Raw(), decls->Raw());
> > +      mDecls = decls.forget();
> > +    }
> > +    if (doc) {
> > +      doc->StyleRuleChanged(sheet, rule);
> > +    }
> > +  }
> 
> `SetCSSDeclaration` of `@page` rule doesn't seem to need to be this
> complicated. Consider taking `nsCSSPageStyleDeclaration::SetCSSDeclaration`
> as the base.
> 
> It actually reminds me the issue that our
> `ServoStyleRule::SetCSSDeclaration` is probably wrong that it doesn't seem
> to call `SetOwningRule` on the declarations. It probably doesn't matter for
> now, but we may still want to do it.

For `ServoPageRuleDeclaration::SetCSSDeclaration`, I have essentially copied it from `ServoStyleRuleDeclaration`.  When you say it's "too complicated", I don't think I know enough to follow what is and is not needed here.

Looking at `ServoStyleRuleDeclaration`, it seems things like `updateBatch` and `StyleRuleChanged` calls were modified recently by bug 1332353, but otherwise it's similar to the version you added in bug 1307357.

Is something like `nsCSSPageStyleDeclaration::SetCSSDeclaration` all that is really needed?  Could you provide a bit more detail?  Thanks! :)
Flags: needinfo?(xidorn+moz)
ServoStyleRuleDeclaration::SetCSSDeclaration is implemented based on DOMCSSDeclarationImpl::SetCSSDeclaration, and ServoPageRuleDeclaration::SetCSSDeclaration should probably be implemented based on nsCSSPageStyleDeclaration::SetCSSDeclaration rather than ServoStyleRuleDeclaration's, given it is the Stylo equivalent of nsCSSPageStyleDeclaration.

There may be certain things need to be changed in Stylo way, but the basic idea is that the Stylo equivalent should behave as similar as possible to its Gecko counterpart in general.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8854615 [details]
Bug 1345206 - Create ServoPageRule.

https://reviewboard.mozilla.org/r/126566/#review129222

> You can actually use `final` instead of `override` for all these methods... although this probably doesn't matter a lot.

Okay, I applied the same change to `ServoStyleRule` as well.

> `SetCSSDeclaration` of `@page` rule doesn't seem to need to be this complicated. Consider taking `nsCSSPageStyleDeclaration::SetCSSDeclaration` as the base.
> 
> It actually reminds me the issue that our `ServoStyleRule::SetCSSDeclaration` is probably wrong that it doesn't seem to call `SetOwningRule` on the declarations. It probably doesn't matter for now, but we may still want to do it.

Thanks for the extra details!  I've updated this to make it similar to `nsCSSPageStyleDeclaration`, minus the `SetModifiedByChildRule()` as discussed on IRC.

I added `SetOwningRule` to `ServoStyleRule` as well.
Comment on attachment 8854615 [details]
Bug 1345206 - Create ServoPageRule.

https://reviewboard.mozilla.org/r/126566/#review130124

r=me with the issues addressed.

::: layout/style/ServoPageRule.cpp:92
(Diff revision 3)
> +void
> +ServoPageRuleDeclaration::GetCSSParsingEnvironment(
> +  CSSParsingEnvironment& aCSSParseEnv)
> +{
> +  GetCSSParsingEnvironmentForRule(Rule(), aCSSParseEnv);
> +}

You would need to rebase your patch on top of bug 1343964 which makes Stylo path doesn't reach this method but instead using a new method `GetURLData` (part 7 of that bug).

::: layout/style/ServoStyleRule.cpp
(Diff revision 3)
>  #include "mozilla/DeclarationBlockInlines.h"
>  #include "mozilla/ServoBindings.h"
>  #include "mozilla/ServoDeclarationBlock.h"
>  #include "mozilla/dom/CSSStyleRuleBinding.h"
>  
> -#include "mozAutoDocUpdate.h"

Why do you remove this? It seems to me we should still need it for `mozAutoDocUpdate`, no? The issue may not show up because of unified build. But in case the file is moved to the first of a unified source file, this could be a problem.
Attachment #8854615 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8854615 [details]
Bug 1345206 - Create ServoPageRule.

https://reviewboard.mozilla.org/r/126566/#review130124

> You would need to rebase your patch on top of bug 1343964 which makes Stylo path doesn't reach this method but instead using a new method `GetURLData` (part 7 of that bug).

Thanks for the tip, I'll rebase and add that change.

> Why do you remove this? It seems to me we should still need it for `mozAutoDocUpdate`, no? The issue may not show up because of unified build. But in case the file is moved to the first of a unified source file, this could be a problem.

Ah, good catch, I edited the wrong file.  I meant to remove this line from `ServoPageRule.cpp` instead, where we no longer need this anymore since `mozAutoDocUpdate` was removed after the first review.
Please push commits without servo side change to MozReview for landing.
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #60)
> Please push commits without servo side change to MozReview for landing.

Yep, that's my next step! :)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b262bb736515
Correct faulty assertions in test_page_parser. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/df2ca32bf841
Extract CSSPageRule base class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/421e7a149446
Create ServoPageRule. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/7b6ccaf5c438
Wire up bindings for @page rule. r=xidorn
You need to log in before you can comment on or make changes to this bug.