Closed Bug 1293590 Opened 8 years ago Closed 8 years ago

Fix clip-path failure in test_transition_per_property.html

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Bug 1293590 - Part 2. Fix expected value of basic-shape position.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1293590 - Part 3. Insert missing reference-box in test cases.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1293590 - Part 1. Merge function parameters into a single string.
58 bytes, text/x-review-board-request
xidorn
: review+
Details 
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.
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.
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: nobody → cku
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
Summary: serialization of basic-shape:position is not correct → Fix clip-path failure in test_transition_per_property.html
We can turn on transition test for both shape-outside & clip-path after this bug fixed.
Attachment #8780149 - Flags: review?(xidorn+moz)
Attachment #8780213 - Flags: review?(xidorn+moz)
Change in part 1 may trigger assertion in nsCSSValue::AppendBasicShapePositionToString. Rework again.
Attachment #8780214 - Flags: review?(cam)
Attachment #8780214 - Attachment is obsolete: true
Attachment #8780149 - Flags: review?(xidorn+moz)
Attachment #8780213 - Flags: review?(xidorn+moz)
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 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)
Attachment #8780149 - Attachment is obsolete: true
Attachment #8780213 - Attachment is obsolete: true
Attachment #8780486 - Flags: review?(xidorn+moz)
Attachment #8780487 - Flags: review?(xidorn+moz)
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 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+
Attachment #8780515 - Flags: review?(xidorn+moz)
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 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+
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
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: