Currently we are parsing and creating the @page rules in servo side. But it appears we don't use/compute these rules so it can make an impact on printing page. To reproduce the test case in macOS, Open the html page and pres Cmd+P, then press the "PDF" combobox and select the "Open PDF in Preview". Gecko shows 300px margin on all sides but Stylo doesn't show that margin.

Nazim, I'm picking up bugs. If you want to offload this to me, feel free.

Thanks for the offer Brad but I'm working on this currently, sorry.

That code correspondes to this part of the gecko code: http://searchfox.org/mozilla-central/source/layout/style/nsStyleSet.cpp#2116 Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=543f6d6d9633b63c2b3479d95b41a0cd4e7dd1f3 I'm not happy about extra_style_data clone inside glue.rs file but that seemed the only way to please borrow checker :/

Comment on attachment 8902123 [details] Bug 1394035 - stylo: Pass the @page rule values into precomputed pseudo element declarations https://reviewboard.mozilla.org/r/173568/#review178920 ::: servo/ports/geckolib/glue.rs:1640 (Diff revision 1) > } > let metrics = get_metrics_provider_for_product(); > + > + if pseudo == PseudoElement::PageContent { > + let extra_data = data.extra_style_data.clone(); > + data.stylist.append_with_page_rules(&guards, &extra_data); What is this trying to do? Rules should only be inserted into the stylist once. This inserts them each time you resolve the style, which is pretty wrong. Also, precomputed_pseudo_element_decls is rebuilt only when UserAgent stylesheets change, so this is going to be wrong in presence of dynamic changes.

Comment on attachment 8902123 [details] Bug 1394035 - stylo: Pass the @page rule values into precomputed pseudo element declarations https://reviewboard.mozilla.org/r/173568/#review178920 > What is this trying to do? > > Rules should only be inserted into the stylist once. This inserts them each time you resolve the style, which is pretty wrong. > > Also, precomputed_pseudo_element_decls is rebuilt only when UserAgent stylesheets change, so this is going to be wrong in presence of dynamic changes. In the gecko side, that is being handled inside `nsStyleSet::ResolveInheritingAnonymousBoxStyle`. The servo equivalent of this is `ServoStyleSet::ResolveInheritingAnonymousBoxStyle` and that method calls `Servo_ComputedValues_GetForAnonymousBox`. This event needs to be happen when user click to print. I did what gecko did and if that is true, I'm pretty sure that is true as well.

(In reply to Nazım Can Altınova [:canaltinova] from comment #7) > Comment on attachment 8902123 [details] > Bug 1394035 - stylo: Pass the @page rule values into precomputed pseudo > element declarations > > https://reviewboard.mozilla.org/r/173568/#review178920 > > > What is this trying to do? > > > > Rules should only be inserted into the stylist once. This inserts them each time you resolve the style, which is pretty wrong. > > > > Also, precomputed_pseudo_element_decls is rebuilt only when UserAgent stylesheets change, so this is going to be wrong in presence of dynamic changes. > > In the gecko side, that is being handled inside > `nsStyleSet::ResolveInheritingAnonymousBoxStyle`. The servo equivalent of > this is `ServoStyleSet::ResolveInheritingAnonymousBoxStyle` and that method > calls `Servo_ComputedValues_GetForAnonymousBox`. This event needs to be > happen when user click to print. I did what gecko did and if that is true, > I'm pretty sure that is true as well. Note that Gecko only creates one rule path and throws it away, you're constantly inserting into the stylist each time.

Comment on attachment 8902124 [details] Bug 1394035 - stylo: Update @page rule test expectations https://reviewboard.mozilla.org/r/173570/#review179110 This is obviously fine, but we need to sort out part 1 still. What does the spec says about precedence and importance of `@page` rules?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8) > Note that Gecko only creates one rule path and throws it away, you're > constantly inserting into the stylist each time. Yes but we have to keep these rules somewhere, right? Otherwise how can we handle and process them? (In reply to Emilio Cobos Álvarez [:emilio] from comment #9) > Comment on attachment 8902124 [details] > Bug 1394035 - stylo: Update @page rule test expectations > > https://reviewboard.mozilla.org/r/173570/#review179110 > > This is obviously fine, but we need to sort out part 1 still. What does the > spec says about precedence and importance of `@page` rules? Spec doesn't say something about precedence. But given that @page rule doesn't apply any elements but page itself that shouldn't be a problem. @page rule only accepts margins currently. Made a few tests and it appears it matches with gecko. Note: Also try run shows that tests are passing.

(In reply to Nazım Can Altınova [:canaltinova] from comment #10) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #8) > > Note that Gecko only creates one rule path and throws it away, you're > > constantly inserting into the stylist each time. > > Yes but we have to keep these rules somewhere, right? Otherwise how can we > handle and process them? There's nothing preventing you to call insert_ordered_rules on a rule node. However if you want to share that code path with the rest of the functions, you're right we need to insert them in the selector maps. However, your patch is doing it _once per call to ResolveInheritingAnonBoxStyle_, which means that the list will only grow and grow. Also, inserting into the stylist from there is just a hack, so let's do the right fix instead.

Sorry for the delay. Split the `precomputed_values_for_pseudo` into two parts and added the @page rules to the rule node without appending `precomputed_pseudo_element_decls` every time.

Comment on attachment 8902123 [details] Bug 1394035 - stylo: Pass the @page rule values into precomputed pseudo element declarations https://reviewboard.mozilla.org/r/173568/#review181684 ::: servo/components/style/stylist.rs:745 (Diff revision 2) > + extra_data: &PerOrigin<ExtraStyleData> > + ) -> StrongRuleNode { > + // If the pseudo element is PageContent, we should append the precomputed > + // pseudo element declerations with specified page rules. > + let page_decls = match *pseudo { > + PseudoElement::PageContent => { There's no way this compiles for servo, plus this is not a great way to do it, I believe. What about: ``` fn rule_node_for_precomputed_pseudo( &self, guards: &StylesheetGuards, pseudo: &PseudoElements, extra_declarations: Option<Vec<ApplicableDeclarationBlock>> ) -> StrongRuleNode { } ``` plus: ``` pub fn precomputed_values_for_pseudo_with_rule_node( &self, guards: .., pseudo: .., parent: .., flags: .., metrics: &FontMetricsProvider, rule_node: StrongRuleNode, ) -> Arc<ComputedValues> { } ``` And changing `precomputed_values_for_pseudo` to just be a sequential call to both, with the `extra_declarations` argument being `None`?

Thanks for the review. Addressed your comment.

Comment on attachment 8902123 [details] Bug 1394035 - stylo: Pass the @page rule values into precomputed pseudo element declarations https://reviewboard.mozilla.org/r/173568/#review181968 Still not ultra-happy about the setup, but not your fault. Maybe file a bug to convert `:-moz-page-content` to just use `@page` rules if you agree that'd simplify the setup? Otherwise r=me, with the cascade level fixed. ::: servo/components/style/stylist.rs:768 (Diff revision 4) > + pseudo: &PseudoElement, > + extra_declarations: Option<Vec<ApplicableDeclarationBlock>>, > + ) -> StrongRuleNode { > + let mut decl; > + let declarations = match self.cascade_data.precomputed_pseudo_element_decls.get(pseudo) { > + Some(declarations) => { Kinda nasty, but I guess :) This is not common anyway... ::: servo/ports/geckolib/glue.rs:1666 (Diff revision 4) > } > let metrics = get_metrics_provider_for_product(); > - data.stylist.precomputed_values_for_pseudo( > + > + // If the pseudo element is PageContent, we should append the precomputed > + // pseudo element declerations with specified page rules. > + let page_decls = match pseudo { Still kinda sad at this special-case... I wonder if we could just use `@page` with arbitrary properties in UA sheets and just use those to style the `:-moz-page-content` box... Not this bug of course, but could be nice. ::: servo/ports/geckolib/glue.rs:1673 (Diff revision 4) > + let mut declarations = vec![]; > + let iter = data.extra_style_data.iter_origins_rev() > + .flat_map(|(d, _)| d.pages.iter()); > + for rule in iter { > + declarations.push(ApplicableDeclarationBlock::from_declarations( > + rule.read_with(&guards.author).block.clone(), Just realised, this needs to somehow account for the origin of the stylesheet, I think, to handle `!important` properly.

Servo PR: https://github.com/servo/servo/pull/18398

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d3fc822d7cf6 stylo: Update @page rule test expectations r=emilio