1384275 - stylo: icon fonts are not loaded on fluentcpp.com

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load http://www.fluentcpp.com/2017/07/25/understandable-statements-run-slower/
2. Look at the buttons below the text "Jonathan Boccara's blog" in page's left column.
3. Scroll down the page until a "Tweet" button is shown at the top of the page.

RESULT:
When Stylo is disabled, the buttons are rendered as images of a hamburger menu button, a magnifying glass, and the Twitter logo respectively. When Stylo is enabled, the buttons are rendered as unrecognized Unicode characters.

Comment 1

[Astley Chen (inactive)]

Compared to normal render, the custom web font was not loaded. Stylo was using default system font.

icomoon - woff

Used as: "voice-icomoon"

@font-face {
  font-family: "voice-icomoon";
  font-style: normal;
  font-weight: normal;
  src: url("//www.fluentcpp.com/wp-content/themes/voice/fonts/voice-icomoon.eot?#iefix4seflc") format("embedded-opentype"), url("//www.fluentcpp.com/wp-content/themes/voice/fonts/voice-icomoon.woff?4seflc") format("woff"), url("//www.fluentcpp.com/wp-content/themes/voice/fonts/voice-icomoon.ttf?4seflc") format("truetype"), url("//www.fluentcpp.com/wp-content/themes/voice/fonts/voice-icomoon.svg?4seflc#icomoon") format("svg");
}

Comment 2

[Cameron McCormack (:heycam)]

When I use the CSSOM to inspect the rules from the style sheet that includes that @font-face rule, it doesn't appear.  The rules directly before it are some @import rules, incorrectly placed in the middle of a style sheet.  I wonder if this is an issue with the C++ rule wrappers?

Comment 3

[Cameron McCormack (:heycam)]

When I dump the contents of the StylesheetContents object, the @import rules and the @font-face rules are missing.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: reduced test

Comment 5

[Cameron McCormack (:heycam)]

Attachment: reduced test more suitable for a reftest

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1384275 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place.

Currently, attempting to parse an at-rule that is out of place, such as
an @import rule after a regular style rule, will cause the parser state
to be set to Invalid.  This will cause any following at-rule to be
rejected until we encounter a regular style rule, at which point we'll
go back to the Body state.  There's nothing in the CSS specs about
needing to reject all following at-rules (or, as the comment above
Invalid says, ignoring the entire rest of the style sheet).

Review commit: https://reviewboard.mozilla.org/r/162718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162718/

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1384275 - Reftest.

Review commit: https://reviewboard.mozilla.org/r/162720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162720/

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8891606 [details]
Bug 1384275 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place.

https://reviewboard.mozilla.org/r/162718/#review168072

::: servo/components/style/stylesheets/mod.rs:262
(Diff revision 1)
> -        match parse_one_rule(&mut input, &mut rule_parser) {
> -            Ok(result) => Ok((result, rule_parser.state)),
> -            Err(_) => {
> -                Err(match rule_parser.state {
> -                    State::Invalid => SingleRuleParseError::Hierarchy,
> -                    _ => SingleRuleParseError::Syntax,
> -                })
> +        parse_one_rule(&mut input, &mut rule_parser)
> +            .map(|result| (result, rule_parser.state))
> +            .map_err(|_| {
> +                if rule_parser.get_and_reset_had_hierarchy_error() {
> +                    SingleRuleParseError::Hierarchy
> +                } else {
> +                    SingleRuleParseError::Syntax
> -            }
> -        }
> +                }
> +            })

Hmm, this seemed to cause a test failure.  Will look into it.

```
 FAIL | layout/style/test/test_group_insertRule.html | insert_import_throws
insert_import_throws: assert_throws: inserting a disallowed rule should throw HIERARCHY_REQUEST_ERR function "function() {
                         grouping_rule.inser..." threw object "SyntaxError: An invalid or illegal string was specified" that is not a DOMException HIERARCHY_REQUEST_ERR: property "code" is equal to 12, expected 3
```

Comment 10

[Cameron McCormack (:heycam)]

(I just forgot to add the actual assignments to had_hierarchy_error...)

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8891606 [details]
Bug 1384275 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162718/diff/1-2/

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8891607 [details]
Bug 1384275 - Reftest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162720/diff/1-2/

Comment 13

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c71953028a0c929ed32fc404a8050990231b34c7

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891606 [details]
Bug 1384275 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place.

https://reviewboard.mozilla.org/r/162718/#review168078

Nice find!

::: servo/components/style/stylesheets/rule_parser.rs:83
(Diff revision 1)
> +
> +    /// Returns whether we previously tried to parse a rule that was invalid
> +    /// due to being in the wrong place (e.g. an @import rule was found after
> +    /// a regular style rule).  The state of this flag is reset when this
> +    /// function is called.
> +    pub fn get_and_reset_had_hierarchy_error(&mut self) -> bool {

nit: The rust naming for this would be something like `take_had_hierarchy_error`

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8891607 [details]
Bug 1384275 - Reftest.

https://reviewboard.mozilla.org/r/162720/#review168084

::: layout/reftests/bugs/1384275-1-ref.html:6
(Diff revision 2)
> +<!DOCTYPE html>
> +<style>
> +p { color: red; }
> +@supports (color: green) {
> +  p { color: green; }
> +}

nit: I'd just use `p { color: green }` instead of `@supports` in the ref, but your call.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8891607 [details]
Bug 1384275 - Reftest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162720/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a990e8c471a
Reftest. r=emilio

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a990e8c471a

Comment 19

[Darkspirit]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> STR:
> 1. Load
> http://www.fluentcpp.com/2017/07/25/understandable-statements-run-slower/
> 2. Look at the buttons below the text "Jonathan Boccara's blog" in page's
> left column.
> 3. Scroll down the page until a "Tweet" button is shown at the top of the
> page.
> 
> RESULT:
> When Stylo is disabled, the buttons are rendered as images of a hamburger
> menu button, a magnifying glass, and the Twitter logo respectively. When
> Stylo is enabled, the buttons are rendered as unrecognized Unicode
> characters.

Verified fixed in Nightly 56 x64 20170730100307 @ Debian Testing
(by attachment 8891606 [details] = https://github.com/servo/servo/pull/17918)