Mismatched parenthesis in some CSS functions do not prevent parsing of subsequent CSS properties

RESOLVED FIXED in mozilla25

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Max Vujovic, Assigned: Max Vujovic)

Tracking

unspecified
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 779837 [details]
Test

Please see attached test for an example. The test fails in FF nightly.

Affected properties include 'transform', 'font-variant-alternates', and 'filter' (when bug 895182 lands).

We need to call SkipUntil("(") in ParseFunctionInternals.
(Assignee)

Comment 1

4 years ago
Created attachment 780690 [details] [diff] [review]
Patch v1

This patch passes mochitests locally.
Attachment #780690 - Flags: review?
(Assignee)

Comment 2

4 years ago
Comment on attachment 780690 [details] [diff] [review]
Patch v1

Requesting review from heycam.
Attachment #780690 - Flags: review? → review?(cam)
Comment on attachment 780690 [details] [diff] [review]
Patch v1

Review of attachment 780690 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Can you change the comment above CSSParserImpl::ParseFunctionInternals to mention that it consumes the ')'?  And also fix its two typos ("meant" and "called") while you're there.  r=me with comments addressed.

::: layout/style/test/test_css_function_mismatched_parenthesis.html
@@ +6,5 @@
> +This test verifies that:
> +(1) Mismatched parenthesis in a CSS function prevent parsing of subsequent CSS
> +properties.
> +(2) Properly matched parenthesis do not prevent parsing of subsequent CSS
> +properties.

parentheses
Attachment #780690 - Flags: review?(cam) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 782760 [details] [diff] [review]
Patch v2
Attachment #780690 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Comment on attachment 780690 [details] [diff] [review]
Patch v1

Review of attachment 780690 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review!

Updated the comment and fixed the typos.

::: layout/style/test/test_css_function_mismatched_parenthesis.html
@@ +6,5 @@
> +This test verifies that:
> +(1) Mismatched parenthesis in a CSS function prevent parsing of subsequent CSS
> +properties.
> +(2) Properly matched parenthesis do not prevent parsing of subsequent CSS
> +properties.

Done.
Attachment #780690 - Attachment is obsolete: false
(Assignee)

Comment 6

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a67e7f011777
The code part of the patch looks great.  I'm a little concerned about the test -- I think it could be easier to understand and more maintainable if it were all run using script rather than having big style and HTML blocks.  That is, you could have a function something like:

function check_parens(declaration, parens_are_balanced) {
  var elt = document.getElementById("...");
  elt.setAttribute("style",
                   "background-color: " +
                     (parens_are_balanced ? "red" : "green") +
                     "; " + declaration + "; background-color: " +
                     (parens_are_balanced ? "green" : "red");
  is(elt.style.getPropertyValue("background-color"), "green",
     "parenthesis balancing within " + declaration);
}

and then have all the tests simply be calls to this function, such as:
  check_parens("transform: scale()", true);
  check_parens("transform: scale(,)", true);
  check_parens("transform: scale(", false);
  check_parens("transform: scale(,", false);

This also avoids going through style computation, which is unnecessary here, and thus should be faster.
(Assignee)

Comment 8

4 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> The code part of the patch looks great.  I'm a little concerned about the
> test -- I think it could be easier to understand and more maintainable if it
> were all run using script rather than having big style and HTML blocks. 
> That is, you could have a function something like:
> 
> function check_parens(declaration, parens_are_balanced) {
>   var elt = document.getElementById("...");
>   elt.setAttribute("style",
>                    "background-color: " +
>                      (parens_are_balanced ? "red" : "green") +
>                      "; " + declaration + "; background-color: " +
>                      (parens_are_balanced ? "green" : "red");
>   is(elt.style.getPropertyValue("background-color"), "green",
>      "parenthesis balancing within " + declaration);
> }
> 
> and then have all the tests simply be calls to this function, such as:
>   check_parens("transform: scale()", true);
>   check_parens("transform: scale(,)", true);
>   check_parens("transform: scale(", false);
>   check_parens("transform: scale(,", false);
> 
> This also avoids going through style computation, which is unnecessary here,
> and thus should be faster.

I like that function! I tried to avoid script to make the test easier to read, but I'm not sure it helped. This way is definitely more maintainable. I'll change the test. Thanks!
(Assignee)

Comment 9

4 years ago
Created attachment 782818 [details] [diff] [review]
Patch v3

New patch with dbaron's feedback.
Attachment #780690 - Attachment is obsolete: true
Attachment #782760 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=10b2c210a7b5

Push new try job with Mochitests only. I didn't change any code since the last patch, only the test.
Great; once you're happy with the try results, set the checkin-needed keyword.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 12

4 years ago
(In reply to Cameron McCormack (:heycam) from comment #11)
> Great; once you're happy with the try results, set the checkin-needed
> keyword.

Thanks! Try results look good with only unrelated failures; ready for checkin.
https://hg.mozilla.org/integration/mozilla-inbound/rev/80be1fb00dc5
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/80be1fb00dc5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.