Closed Bug 897094 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mvujovic, Assigned: mvujovic)

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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.
Attached patch Patch v1 (obsolete) — Splinter Review
This patch passes mochitests locally.
Attachment #780690 - Flags: review?
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+
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #780690 - Attachment is obsolete: true
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
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.
(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!
Attached patch Patch v3Splinter Review
New patch with dbaron's feedback.
Attachment #780690 - Attachment is obsolete: true
Attachment #782760 - Attachment is obsolete: true
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.
Keywords: checkin-needed
(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/mozilla-central/rev/80be1fb00dc5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.