Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.