1057674 - CSS filters break site layout when the filter value is empty and the closing semicolon is missing

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Trevor Rowbotham [:rowbot]]

Attachment: filters enabled.png

If you visit [1], with layout.css.filters.enabled = true, the layout of the site is completely broken.  Setting layout.css.filters.enabled = false fixes the problem.  The cause of this appears to be that the one of the stylesheets contains filter properties with empty values AND the filter property is not ended with a semicolon. An empty value with a closing semicolon does not cause a problem.

The offending rules appear to be on lines 293, 297, and 307 of the main-rollup.css file at [1].  Removing those lines in the Style Editor fixes the problem.

I'm not sure how common it is to find the filter property with an empty value and no closing semicolon out there in the wild.  So I'm not sure if this is something worth fixing in the parser or if it should just be turned into a tech evangelism bug.

[1] https://forum.myrcon.com/

Comment 1

[Trevor Rowbotham [:rowbot]]

Attachment: filters disabled.png

Comment 2

[Boris Zbarsky [:bzbarsky]]

> So I'm not sure if this is something worth fixing in the parser

This needs to be fixed in the parser, for sure.

The relevant rules look like this:

  .cke_buttonarrow{filter:}

As far as I can tell, CSSParserImpl::ParseSingleFilter is just buggy.  It gets the first non-whitespace token, and if that's not a url and filters are enabled and it's not a function, then it returns false... without ungetting the token.  Which means in this case it consumes the '}' and then we see we have an invalid value and try to find end-of-declaration, which we never do (because the '}' got consumes).

Max, it looks like you have blame for this code?

Comment 3

[Max Vujovic]

Thanks for the report, Trevor, and thanks for pinpointing the problem, Boris. I'll post a fix for this.

Comment 4

[Max Vujovic]

Attachment: Patch

The site is fixed when this patch is applied.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm not convinced the test is necessary -- if you enable the pref, I think you should get a failure in test_property_syntax_errors.html (from the check_empty_value_rejected function).

(It's worth testing if you get any other errors in the layout/style mochitests with the pref enabled.)

If you don't get an error from that test with the pref enabled, I'd like to know why that test isn't working.

Comment 6

[Max Vujovic]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #5)
> I'm not convinced the test is necessary -- if you enable the pref, I think
> you should get a failure in test_property_syntax_errors.html (from the
> check_empty_value_rejected function).

You're exactly right. I get the expected failures in test_property_syntax_errors.html without the code change, and they're fixed by the code change. Thanks for pointing that test out- I'll remove mine.

> 
> (It's worth testing if you get any other errors in the layout/style
> mochitests with the pref enabled.)

Looks like I get some other errors. I've filed bug 1057900 to track them.

> 
> If you don't get an error from that test with the pref enabled, I'd like to
> know why that test isn't working.

Comment 7

[Max Vujovic]

Attachment: Patch v2

Same code change, minus the new test, since test_property_syntax_errors.html already covers this bug when CSS filters is enabled.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8478038 [details] [diff] [review]
Patch v2

r=dbaron

Comment 9

[Max Vujovic]

Attachment: Patch v3

Change commit message to specify dbaron as reviewer.

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/236906d22acf

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=40ebd30fadf8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/236906d22acf