Fix clip-path failure in test_transition_per_property.html

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
After fixing the assertion-failure in bug 1269990, I notice all the clipPathTests are failed after turning on clip-path-shapes.enabled preference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=215e730f317b&selectedJob=25359476

File this bug to trace and fix this mochitest failure.
Comment hidden (spam)
(Assignee)

Comment 2

2 years ago
We should serialize "circle(500px at 500px 500px) border-box" as "circle(500px at 500px 500px) border-box", but in reality we serialize it as "circle(500px at calc(500px + 0%) calc(500px + 0%)).
Logic in AdjustEdgeOffsetPairForBasicShape lead this result.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: clipPathTests test cases in test_transition_per_property.html are all failed after turning on clip-path-shapes.enabled → serialization of basic-shape:position is not correct
(Assignee)

Updated

2 years ago
Assignee: nobody → cku
(Assignee)

Comment 4

2 years ago
xidorn, to verify this change(if you want), plz import a patch written by dholbert in bug 1269990
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1269990&attachment=8753950

And test by 
./mach test layout/style/test/test_transitions_per_property.html
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: serialization of basic-shape:position is not correct → Fix clip-path failure in test_transition_per_property.html
(Assignee)

Comment 8

2 years ago
We can turn on transition test for both shape-outside & clip-path after this bug fixed.
(Assignee)

Updated

2 years ago
Attachment #8780149 - Flags: review?(xidorn+moz)
Attachment #8780213 - Flags: review?(xidorn+moz)
(Assignee)

Comment 9

2 years ago
Change in part 1 may trigger assertion in nsCSSValue::AppendBasicShapePositionToString. Rework again.
(Assignee)

Updated

2 years ago
Attachment #8780214 - Flags: review?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780214 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8780149 - Flags: review?(xidorn+moz)
Attachment #8780213 - Flags: review?(xidorn+moz)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8780149 [details]
Bug 1293590 - Part 1. Fix basic-shape position serialization.

https://reviewboard.mozilla.org/r/70928/#review68666

Per discussion with CJ on IRC, I think the result is correct per spec. The testcase needs to be fixed, so clear this review request.
Attachment #8780149 - Flags: review?(xidorn+moz)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8780213 [details]
Bug 1293590 - Part 2. Insert missing reference-box in test cases.

https://reviewboard.mozilla.org/r/70946/#review68668

This would need fix as well.
Attachment #8780213 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780149 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8780213 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8780486 - Flags: review?(xidorn+moz)
Attachment #8780487 - Flags: review?(xidorn+moz)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8780486 [details]
Bug 1293590 - Part 2. Fix expected value of basic-shape position.

https://reviewboard.mozilla.org/r/71190/#review68702

::: layout/style/test/test_transitions_per_property.html:1428
(Diff revision 1)
> -  // Split arguments at commas or whitspeaces.
> -  funcParameters = matches[1].split(/\s*,\s*|\s+/);
> +  // Remove the last character in matches[1] if it is ')' to prevent one
> +  // additional empty item put in funcParameters.
> +  if (matches[1].slice(-1) == ')') {
> +    matches[1] = matches[1].slice(0, -1);
> +  }
> +  // Split arguments at commas or whitspeaces or patentheses or plus.
> +  funcParameters = matches[1].split(/\s*,\s*|\s*\+\s*|\s*\(\s*|\s*\)\s*|\s+/);

Let's don't make this test more complicated and counterintuitive... Could you merge function parameters together as a string, and do a straightforward string comparison for this?

That should be in a separate patch.
Attachment #8780486 - Flags: review?(xidorn+moz)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8780487 [details]
Bug 1293590 - Part 3. Insert missing reference-box in test cases.

https://reviewboard.mozilla.org/r/71192/#review68704
Attachment #8780487 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780515 - Flags: review?(xidorn+moz)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8780515 [details]
Bug 1293590 - Part 1. Merge function parameters into a single string.

https://reviewboard.mozilla.org/r/71222/#review68720

Looks good, thanks!
Attachment #8780515 - Flags: review?(xidorn+moz) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8780486 [details]
Bug 1293590 - Part 2. Fix expected value of basic-shape position.

https://reviewboard.mozilla.org/r/71190/#review68722
Attachment #8780486 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f185aaeeff45b6f6ca9aaf81d63bce802d1f636
Bug 1293590 - Part 1. Merge function parameters into a single string. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/8079ced4885628b62afa130c5a250cca41444d8d
Bug 1293590 - Part 2. Fix expected value of basic-shape position.r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/6accce4abaadad6c8de1de4da908132aad1f196e
Bug 1293590 - Part 3. Insert missing reference-box in test cases. r=xidorn

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f185aaeeff4
https://hg.mozilla.org/mozilla-central/rev/8079ced48856
https://hg.mozilla.org/mozilla-central/rev/6accce4abaad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Blocks: 1324179
You need to log in before you can comment on or make changes to this bug.