stylo: add support for alternate ServoStyleSheets and disabling author styles

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: bz, Assigned: bradwerth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments, 4 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
Causes asserts in various tests
(Assignee)

Updated

6 months ago
Assignee: nobody → bwerth
Another one for Brad's list.
Flags: needinfo?(bwerth)
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months ago
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

I'm testing with /testing/web-platform/tests/html/semantics/links/linktypes/alternate-css.html where the two Page Styles work, but the "No Style" choice does not trigger a restyle. I've traced it to the point that I can see that ServoRestyleManager::ProcessPostTraversal does not get a nsChangeHint from Servo. https://dxr.mozilla.org/mozilla-central/source/layout/base/ServoRestyleManager.cpp#149 .

It appears that no damage is being assigned to the relevant elements (the body element and the sole div element). I had hoped that the call to Servo_StyleSet_FlushStyleSheets at https://dxr.mozilla.org/mozilla-central/source/layout/style/ServoStyleSet.cpp#116 would globally assign damage to give me a starting point, but that's not even happening. What is the appropriate trigger to get Servo to assign damage to the restyled elements?
Attachment #8848295 - Flags: feedback?(emilio+bugs)

Comment 4

5 months ago
mozreview-review
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123208

So, this is right now doing nothing. And it makes sense. If we're arriving to ProcessPostTraversal it means that we have actually restyled the content (that's because we end up calling `RestyleForCSSRuleChanges()` here: http://searchfox.org/mozilla-central/source/layout/base/PresShell.cpp#1440).

But in servo the styles doesn't really change, we shouldn't do more work in the post-traversal, which is what you're seeing.

Note that what Gecko does is effectively setting dirty "stylesheet level" bits, and at the end of the update it will check them and call GatherRuleProcessors() on all the dirty stylesheets, skipping the relevant levels: http://searchfox.org/mozilla-central/source/layout/style/nsStyleSet.cpp#452.

Now, the interesting part I guess this bug is about is figuring out how to tell Servo:

 1) We don't want to apply author stylesheets.
 2) The styles have changed, so you need to rebuild the internal data structures.

We have infra in place for 2), as you've seen above.

I believe the easiest way to do 1) is propagating the "author style disabled" bit up to Stylist. There, we can just skip the relevant stylesheets in `Stylist::update` depending on the origin of the stylesheet. Also, we need to avoid style attributes in `push_applicable_declarations` in the servo side, if I'm reading the Gecko code right.

There are even trickier semantics for presentational hints, too, it seems, where we skip them for ones, but not others. Manish could be able to help here faster than I I guess, but I guess that can be an entire followup bug.

::: layout/style/ServoStyleSet.cpp:100
(Diff revision 1)
>  }
>  
>  nsresult
>  ServoStyleSet::SetAuthorStyleDisabled(bool aStyleDisabled)
>  {
> -  MOZ_CRASH("stylo: not implemented");
> +  if (aStyleDisabled == !mAuthorStyleDisabled) {

nit: I think it's clearer to early-return:

```
if (aStyleDisabled == mAuthorStyleDisabled) {
    return NS_OK;
}
// Do work.
```

Comment 5

5 months ago
mozreview-review
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123224

Feel free to ask for more feedback as you see fit, happy to help! :)

Comment 6

5 months ago
mozreview-review-reply
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review123208

> nit: I think it's clearer to early-return:
> 
> ```
> if (aStyleDisabled == mAuthorStyleDisabled) {
>     return NS_OK;
> }
> // Do work.
> ```

Though I guess this is completely unreachable, given the check I pointed out in my previous comment near RestyleForCSSRuleChanges().

So I think you should be able to `MOZ_ASSERT(aStyleDisabled != mAuthorStyleDisabled)`.
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

Those previous comments should've gone together, sorry for the bugspam everybody.
Attachment #8848295 - Flags: feedback?(emilio+bugs)
See Also: → bug 1351996
Duplicate of this bug: 1351996
(Since these are are related but a little different.)
Summary: stylo: add support for alternate ServoStyleSheets → stylo: add support for alternate ServoStyleSheets and disabling author styles
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1243581
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8848295 - Flags: review?(cam)
Attachment #8856864 - Flags: review?(cam)
Attachment #8854253 - Flags: review?(cam)
Attachment #8854254 - Flags: review?(cam)

Comment 17

4 months ago
mozreview-review
Comment on attachment 8848295 [details]
Bug 1341721 Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled.

https://reviewboard.mozilla.org/r/121216/#review131342
Attachment #8848295 - Flags: review?(cam) → review+

Comment 18

4 months ago
mozreview-review
Comment on attachment 8856864 [details]
Bug 1341721 Part 2a: Add a bool argument to Servo_StyleSet_NoteStyleSheetsChanged.

https://reviewboard.mozilla.org/r/128782/#review131344
Attachment #8856864 - Flags: review?(cam) → review+

Comment 19

4 months ago
mozreview-review
Comment on attachment 8854253 [details]
Bug 1341721 Part 2b: Add a bool property to PerDocumentStyleDataImpl.

https://reviewboard.mozilla.org/r/126190/#review131346
Attachment #8854253 - Flags: review?(cam) → review+

Comment 20

4 months ago
mozreview-review
Comment on attachment 8854254 [details]
Bug 1341721 Part 3: Pass the author_style_disabled flag through ExtraStyleData and ignore document styles during update, if set.

https://reviewboard.mozilla.org/r/126192/#review131348

::: servo/components/style/stylist.rs:130
(Diff revision 2)
>      #[allow(missing_docs)]
>      #[cfg(feature = "servo")]
>      pub marker: PhantomData<&'a usize>,
> +
> +    /// A parameter to note whether we should ignore author styles.
> +    pub author_style_disabled: bool,

Keep the marker member at the end.

::: servo/components/style/stylist.rs:247
(Diff revision 2)
>                  self.add_stylesheet(&ua_stylesheets.quirks_mode_stylesheet,
>                                      guards.ua_or_user, extra_data);
>              }
>          }
>  
> +        if !extra_data.author_style_disabled {

I guess it would be nice if we could do this at the top of add_stylesheet, but I don't think we know that cascade level the sheet as it in there...
Attachment #8854254 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #0)
> Causes asserts in various tests

Patch gets enable/disable of authored styles working, and alternate stylesheets seemed to me to be working already. Try server doesn't show any fails or unexpected passes. I want to make sure this is done right before I land the patch. Which tests are or were failing?
(Assignee)

Updated

4 months ago
Flags: needinfo?(bwerth) → needinfo?(bzbarsky)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8854253 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8854254 - Attachment is obsolete: true
> Which tests are or were failing?

layout/reftests/bugs/360746-1.html has this assertion failure:

###!!! ASSERTION: stylo: can't handle alternate ServoStyleSheets yet: 'Error', file /Users/bzbarsky/mozilla/stylo/mozilla/dom/base/nsDocument.cpp, line 6441
Flags: needinfo?(bzbarsky)
And the patches here don't remove that assert, or the "continue" in that function.

I guess you would need a test that exercises that codepath.  That means either hitting it via SetHeaderData as 360746-1.html does (I can try to come up with a testcase that produces incorrect rendering here, too) or via the selectedStyleSheetSet setter or enableStyleSheetsForSet method.

selectedStyleSheetSet is used by layout/reftests/font-face/sheet-set-switch-1.html which is set as skip-if(stylo) at the moment, possibly because it doesn't perform the switch and hence times out.  It's also used in session restore, looks like; see PageStyleInternal.restoreTree.restoreFrame in browser/components/sessionstore/PageStyle.jsm.

enableStyleSheetsForSet seems to only be used in-tree in sessionstore tests, which stylo doesn't run.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #31)
> I guess you would need a test that exercises that codepath.  That means
> either hitting it via SetHeaderData as 360746-1.html does (I can try to come
> up with a testcase that produces incorrect rendering here, too) or via the
> selectedStyleSheetSet setter or enableStyleSheetsForSet method.
Thank you. Since this path now works, I've removed the assert and turned the reftest back on.

> selectedStyleSheetSet is used by
> layout/reftests/font-face/sheet-set-switch-1.html which is set as
> skip-if(stylo) at the moment, possibly because it doesn't perform the switch
> and hence times out.
That issue is filed as Bug 1290228.
(Assignee)

Updated

4 months ago
Attachment #8857742 - Flags: review?(cam)
Attachment #8857743 - Flags: review?(cam)
Attachment #8857754 - Flags: review?(cam)
> Since this path now works, I've removed the assert and turned the reftest back on.

I don't understand how this path can work with stylo.  It's quite explicitly doing sheet->AsGecko(), no?

> That issue is filed as Bug 1290228.

That bug is about media queries.  There are no media queries in either layout/reftests/font-face/sheet-set-switch-1.html or layout/reftests/font-face/resize-detector-iframe.html that I can see, so I doubt that test is affected by bug 1290228.
Flags: needinfo?(bwerth)

Comment 37

4 months ago
mozreview-review
Comment on attachment 8857742 [details]
Bug 1341721 Part 2b: Add a bool property to PerDocumentStyleDataImpl.

https://reviewboard.mozilla.org/r/129684/#review132332
Attachment #8857742 - Flags: review?(cam) → review+

Comment 38

4 months ago
mozreview-review
Comment on attachment 8857743 [details]
Bug 1341721 Part 3: Update Stylist to maintain a state of whether author styles are disabled, and ExtraData parameters to modify that setting.

https://reviewboard.mozilla.org/r/129686/#review132334

::: servo/components/style/stylist.rs:264
(Diff revision 1)
> +        for ref stylesheet in doc_stylesheets.iter()
> +          .filter(|&s| author_style_enabled || s.origin != Origin::Author) {
>              self.add_stylesheet(stylesheet, guards.author, extra_data);
>          }
>  
> +

Nit: probably don't need this extra blank line.
Attachment #8857743 - Flags: review?(cam) → review+

Comment 39

4 months ago
mozreview-review
Comment on attachment 8857754 [details]
Bug 1341721 Part 4: Re-enable reftest 360746, and remove the assert saying that alternate sheets are not supported.

https://reviewboard.mozilla.org/r/129720/#review132336
Attachment #8857754 - Flags: review?(cam) → review+
And in particular, applying the "part 4" patch and running layout/reftests/bugs/360746-1.html fails the IsGecko() assertions that AsGecko() does and crashes quite nicely.  As expected.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #40)
> And in particular, applying the "part 4" patch and running
> layout/reftests/bugs/360746-1.html fails the IsGecko() assertions that
> AsGecko() does and crashes quite nicely.  As expected.

Urg, yes, I'm working too fast and misreading the console logs. I've fixed the crash noted in comment 36, and I'm seeing if I can get layout/reftests/font-face/sheet-set-switch-1.html working.
Flags: needinfo?(bwerth)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Blocks: 1356158
(Assignee)

Comment 46

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #31)
 
> selectedStyleSheetSet is used by
> layout/reftests/font-face/sheet-set-switch-1.html which is set as
> skip-if(stylo) at the moment, possibly because it doesn't perform the switch
> and hence times out.

The test still doesn't complete, but the stylesheet switch does happen with this patch. Bug 1356158 opened to figure out what's going wrong with the test.
(Assignee)

Updated

4 months ago
Attachment #8857793 - Flags: review?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 53

4 months ago
mozreview-review
Comment on attachment 8857793 [details]
Bug 1341721 Part 5: Remove asserts from some unexpected-pass reftests.

https://reviewboard.mozilla.org/r/129778/#review132452
Attachment #8857793 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8857742 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8857743 - Attachment is obsolete: true

Comment 56

4 months ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95bd9774b6fb
Part 1: ServoStyleSet implementation of SetAuthorStyleDisabled. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a5ed50a011ff
Part 2a: Add a bool argument to Servo_StyleSet_NoteStyleSheetsChanged. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aac6714c518a
Part 4: Re-enable reftest 360746, and remove the assert saying that alternate sheets are not supported. r=heycam
https://hg.mozilla.org/integration/autoland/rev/782e957654a2
Part 5: Remove asserts from some unexpected-pass reftests. r=heycam
https://hg.mozilla.org/mozilla-central/rev/95bd9774b6fb
https://hg.mozilla.org/mozilla-central/rev/a5ed50a011ff
https://hg.mozilla.org/mozilla-central/rev/aac6714c518a
https://hg.mozilla.org/mozilla-central/rev/782e957654a2
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.