Closed Bug 1057900 Opened 5 years ago Closed 5 years ago

test_value_cloning.html and test_value_computation.html fail with CSS filters enabled

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mvujovic, Assigned: mvujovic)

References

Details

Attachments

(1 file)

6.15 KB, patch
dbaron
: review+
mstange
: review+
mvujovic
: checkin+
Details | Diff | Splinter Review
If you run:
./mach mochitest-plain --setpref=layout.css.filters.enabled=true

You get the following failures:

248009 WARNING The following tests failed:
248010 INFO TEST-UNEXPECTED-ERROR | /tests/layout/style/test/test_value_cloning.html | Assertion count 12 is greater than expected range 0-0 assertions.
TEST-INFO expected OK
248011 INFO TEST-UNEXPECTED-ERROR | /tests/layout/style/test/test_value_computation.html | Assertion count 13 is greater than expected range 9-9 assertions.
TEST-INFO expected OK
248012 INFO SUITE-END | took 1110s

I'll be looking into this. I don't remember seeing any mochitest failures when CSS filter parsing was landed some months ago, so this might be interaction with new code- we'll see.
Assignee: nobody → mvujovic
Blocks: 1057180
The assertions are happening on cases like "blur(calc(10%))". We're parsing calc values for CSS filter lengths, but we're not resolving them yet. I'll work on that.
(In reply to Max Vujovic from comment #1)
> The assertions are happening on cases like "blur(calc(10%))".

I mean "blur(calc(1px))"- percentage values are correctly rejected at parse time, since blur should only take a <length>.
Attached patch PatchSplinter Review
r? dbaron for nsRuleNode changes.
r? mstange for rendering reftests.

Before this patch, a calc() argument in CSS blur() would be stored and then not handled in nsCSSFilterInstance.cpp. This resulted in an "unexpected unit" assertion in nsCSSFilterInstance::SetAttributesForBlur, which expected only eStyleUnit_Coord (not eStyleUnit_Calc). Now, calc() is resolved to a coord in nsRuleNode, using the same flags as the blur radius for box/drop shadows [1].

I added two rendering tests, one for a postive calc value and one for a negative calc value because I thought it'd be nice to know that the rendering was correct in addition to the parsing tests.

[1]: http://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?from=nsRuleNode.cpp#4060
Attachment #8478680 - Flags: review?(mstange)
Attachment #8478680 - Flags: review?(dbaron)
Comment on attachment 8478680 [details] [diff] [review]
Patch

r=dbaron on nsRuleNode changes
Attachment #8478680 - Flags: review?(dbaron) → review+
Attachment #8478680 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/efd173e4cc23
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.