Closed Bug 1394035 Opened 7 years ago Closed 7 years ago

stylo: @page rules aren't working properly

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: canova, Assigned: canova)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Attached file testcase.html
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.
Priority: -- → P2
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.
Attachment #8902123 - Flags: review?(emilio) → review-
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?
Attachment #8902124 - Flags: review?(emilio) → review+
(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`?
Attachment #8902123 - Flags: review?(emilio)
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.
Attachment #8902123 - Flags: review?(emilio) → review+
Attachment #8902123 - Attachment is obsolete: true
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d3fc822d7cf6
stylo: Update @page rule test expectations r=emilio
https://hg.mozilla.org/mozilla-central/rev/d3fc822d7cf6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.