1575186 - Shape Path Editor does not work for circle basic shape without coordinates

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Razvan Caliman [:rcaliman]]

Attachment: clip-path-circle-no-coordinates.mp4

If the circle() basic shape is used without coordinates, the Shape Path Editor fails to run and errors are thrown in the browser console.

First identified on the profile pictures of people listed on http://thewebahead.net/

Steps to reproduce:

• Run this in the address bar of a new tab:

data:text/html,<style>div {clip-path: circle(); width: 300px; height: 300px; background: black;}</style><div>

• Open DevTools Inspector and inspect the <div> element
• In the Rules panel, click the icon to trigger the Shape Path Editor on to the clip-path: circle() declaration.

Expected result
The Shape Path Editor shows up on the page over the black circle and allows the user to edit the shape.

Actual result
The Shape Path Editor does not show up. Errors are thrown in the browser console about missing coords. An infinite loop is triggered without a chance to stop it.

Comment 1

[Razvan Caliman [:rcaliman]]

Mozregression points to this range of which Bug 1559796 is the likely cause for the regression.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:rcaliman, could you try to find a regression range in using for example mozregression?

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1575186 - Ensure Shape Path Editor can be toggled for basic shapes with default values. r=pbro

The patch for Bug 1559796 has changed the computed value for the default circle() basic shape (no coordinates). It now reflects the approach used with default ellipse() basic shape (no coordinates), thus addressing the issue with inconsistency raised in Bug 1521508.

The change means the Shape Path Editor's parsing of circle() and ellipse() need to account for the missing "closest-side" default radius. This patch addresses this need and introduces a test to check whether the Shape Path Editor successfully triggers for basic shapes with default values.

Comment 4

[Razvan Caliman [:rcaliman]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #2)

:rcaliman, could you try to find a regression range in using for example mozregression?

See Comment #1. Found Bug 1559796 to be the regression.

Comment 5

[Razvan Caliman [:rcaliman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7809c1efe436fbb87839c4532a288c01965dcf07

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79741214531f
Ensure Shape Path Editor can be toggled for basic shapes with default values. r=pbro

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/79741214531f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. We have one beta left before RC next week.

Comment 9

[Razvan Caliman [:rcaliman]]

Comment on attachment 9086650 [details]
Bug 1575186 - Ensure Shape Path Editor can be toggled for basic shapes with default values. r=pbro

Beta/Release Uplift Approval Request

• User impact if declined: Developers are unable to use the Shape Path Editor with circle() paths due to a breaking change that occurred a while ago at the platform level in Bug 1559796.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk: fixes a regression with parsing default circle() shape values in the Shape Path editor. No other shape values or functionality changed.
  Introduces an automated test to catch any failures like these in the future.
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9086650 [details]
Bug 1575186 - Ensure Shape Path Editor can be toggled for basic shapes with default values. r=pbro

Fixes a devtools regression in Fx69. Approved for 69.0b16.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef23f491d904