The default bug view has changed. See this FAQ.

stylo: Expose computed style list to dev tools

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
12 days ago
an hour ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

12 days ago
We need a Servo mechanism to expose computed styles to the dev tools. A method is proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1337305#c13.
(Assignee)

Updated

12 days ago
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

12 days ago
Comment on attachment 8845956 [details]
Bug 1346256 Part 1: Add Servo methods to expose style rules to Gecko.

Emilio, I am struggling with the Servo types for this case. I would greatly appreciate it if you would review my non-working Servo code. I've annotated the code with comments to show my thought process.
Attachment #8845956 - Flags: feedback?(emilio+bugs)

Comment 4

12 days ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1: Add Servo methods to expose style rules to Gecko.

https://reviewboard.mozilla.org/r/119046/#review121050

::: servo/ports/geckolib/glue.rs:1227
(Diff revision 2)
> +// it's possible, or what the bindings should look like.
> +pub extern "C" fn Servo_Element_GetStyleRule(element: RawGeckoElementBorrowed) -> RawServoStyleRuleStrong
> +{
> +    let element = GeckoElement(element);
> +    let style_source_option = element.borrow_data().unwrap().styles().primary.rules.style_source();
> +    

nit: Trailing whitespace should go before landing.

::: servo/ports/geckolib/glue.rs:1230
(Diff revision 2)
> +    let element = GeckoElement(element);
> +    let style_source_option = element.borrow_data().unwrap().styles().primary.rules.style_source();
> +    
> +    // This is clearly unsafe, but I'm not sure how to choose a sensible return
> +    // value if style_source_option is None.
> +    let style_source = style_source_option.unwrap();

If there's no style source, it means the element doesn't match any rules, otherwise this should be ok.

::: servo/ports/geckolib/glue.rs:1237
(Diff revision 2)
> +    // Similar problem here, that I want to return a real style rule, and not
> +    // an Option. The "unreachable!" code is clearly reachable.
> +    let style_rule = match style_source {
> +        &StyleSource::Style(ref rule) => rule.clone().into_strong(),
> +        _ => {
> +            unreachable!("Servo_Element_GetStyleRule should only be called on an element with style rule.");

This is wrong. Presumably you want to return either a StyleRule or a ServoDeclarationBlock. Gecko uses `nsIStyleRule` to represent the equivalent of both.

We can coalesce Servo to represent style attributes, etc. as style rules, but I'd prefer not.

If for now you only want to return the last CSS rule matched, you could do something like:

```
let mut rules = element.borrow_data().unwrap().styles().primary.rules.clone();

loop {
    match rules.style_source() {
        Some(&StyleSource::Style(ref rule)) => return rule.clone().into_strong(),
        None => return RawServoStyleRuleStrong::null(),
        _ => {},
    }
    
    rules = rules.get().parent.unwrap()
}
```

Or something like that (haven't really tested).
Comment on attachment 8845956 [details]
Bug 1346256 Part 1: Add Servo methods to expose style rules to Gecko.

It's close :). I think you definitely should be able to return null from this function, since you may inspect an element which doesn't match any rules for example.

I think something like my last comment should be fine for now, we can care about the style attribute and animation rules later.
Attachment #8845956 - Flags: feedback?(emilio+bugs)
Priority: -- → P1
Comment hidden (mozreview-request)
Created attachment 8849426 [details] [diff] [review]
wip patch

I think you would want something like this in Servo side.

If you also need style elements, there are several other functions you would need to add, but this should be the basic bit of things you may need.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> If you also need style elements, there are several other functions you would
> need to add,

I meant... if you also need style of pseudo-elements...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 hours ago
Attachment #8845956 - Flags: review?(cam)
Attachment #8850152 - Flags: review?(cam)

Comment 11

an hour ago
mozreview-review
Comment on attachment 8850152 [details]
Bug 1346256 Part 2: Change GetCSSStyleRules to pull servo style data directly from the element.

https://reviewboard.mozilla.org/r/122842/#review125140

::: layout/inspector/inDOMUtils.cpp:270
(Diff revision 1)
> +  #ifdef MOZ_STYLO
> +  else {

I don't think you need `#ifdef MOZ_STYLO` here. You should probably:
1. early return if `source.IsNull()`, then
2. just check `if (source.IsGeckoRuleNodeOrNull())` and its `else` without any condition compile.

`source.IsGeckoRuleNodeOrNull()` always returns true if stylo is not compiled.

::: layout/inspector/inDOMUtils.cpp:282
(Diff revision 1)
> +      RefPtr<css::Rule> ruleObj = new ServoStyleRule(
> +        Servo_StyleSourceList_GetStyleRuleAt(styleSourceList, i).Consume());

A style source can be either a rule object or a property declaration block. In the latter case, `Servo_StyleSourceList_GetStyleRuleAt` would return nullptr. You should probably check that and avoid creating `ServoStyleRule` for that.

And I wonder whether you want declaration block as well, which you should probably create a `ServoDeclarationBlock` object instead.
You need to log in before you can comment on or make changes to this bug.