fix(material/progress-spinner): progress-spinners with diameter less than or equal to 10 do not show #20062

AlexLangdon · 2020-07-21T22:15:51Z

As @paulferaud pointed out in #19469, using BASE_STROKE_WIDTH when calculating the spinner circle radius is causing issues, such as the progress spinner becoming invisible if the diameter is less than or equal to 10.

The proposed solution is to use the current stroke width instead of the BASE_STROKE_WIDTH constant for calculating the spinner radius.

This one change will also stop the spinner circle extending beyond its parent mat-progress-spinner dimensions for custom stroke widths (see screenshot and stackblitz below), which I presume is an improvement.

https://stackblitz.com/edit/components-issue-jsceqa?file=src%2Fapp%2Fexample-component.html

Before commit:

After commit:

Fixes #19469

…than or equal to 10 do not show Fixes a bug in the Angular Material ‘progress-spinner’ component where progress-spinners with a diameter less than or equal to 10 do not show. This is because the spinner radius calculation uses a constant stroke width of 10 instead of the current stroke width. Fixes angular#19469

AlexLangdon · 2020-07-21T22:26:29Z

src/material/progress-spinner/progress-spinner.ts

- * Base reference stroke width of the spinner.
- * @docs-private
- */
-const BASE_STROKE_WIDTH = 10;


The intention behind using BASE_STROKE_WIDTH constant isn't yet apparent to me, given it is not used anywhere else. Maybe it worked for a spinner with a default diameter and stroke width?

Decided to keep BASE_STROKE_WIDTH for use in calculating the base circle diameter.

crisbeto

If I remember correctly, we need the base stroke, because it's hardcoded to the default animation that's inside the CSS and changing it to something different will break it. See:

I think the underlying problem here is that the dynamically-inserted style tags with the new animation are keyed only on the diameter, not the circumference which is what determines how the animation looks and accounts both for the diameter and the stroke. I don't remember whether we did it this way because the logic breaks down at some point, or we just didn't think that it was important. It wouldn't hurt changing it and testing it out though.

AlexLangdon · 2020-07-22T22:49:34Z

If I remember correctly, we need the base stroke, because it's hardcoded to the default animation that's inside the CSS and changing it to something different will break it. See:

I think the underlying problem here is that the dynamically-inserted style tags with the new animation are keyed only on the diameter, not the circumference which is what determines how the animation looks and accounts both for the diameter and the stroke. I don't remember whether we did it this way because the logic breaks down at some point, or we just didn't think that it was important. It wouldn't hurt changing it and testing it out though.

Thanks for pointing that out, hadn't checked that case earlier. I will see if I can find the cause of this animation issue.

AlexLangdon · 2020-07-27T16:35:09Z

@crisbeto

Based on your suggestion I found an issue with using floating point circumference values in animation names. The animation-name property does not support periods (https://developer.mozilla.org/en-US/docs/Web/CSS/animation-name), so just appending the circumference to the end of the animation name e.g. “animation-name-90.6” would be invalid.

It should be noted that this is also a problem in the current code, if a floating point diameter is given for the spinner - see https://stackblitz.com/edit/components-issue-dxxhqv?file=src%2Fapp%2Fexample-component.html

To get around this, I have replaced “.” characters with “_” in the circle diameter appended to the animation name e.g. “animation-name-90_6” instead of “animation-name-90.6”.

This should solve the animation issue mentioned above, along with allowing floating point numbers to be used for the stroke width and diameter, which I assume is preferable. The fixes mentioned in my first comment regarding diameter <= 10 and the circle element extending beyond its parents are also maintained.

Happy to hear feedback on if/how float input values should be handled here.

crisbeto

Something to keep in mind: even though the changes to the circle sizing are correct, they might make the PR harder to land, because they'll break people's screenshot tests.

crisbeto · 2020-07-27T18:33:11Z

src/material/progress-spinner/progress-spinner.ts

+  /** Returns a string that is used for setting the spinner animation-name CSS property */
+  get _spinnerAnimationLabel() {
+    // Period '.' character cannot be included in an animation-name
+    return this._circleDiameter.toString().replace('.', '_');


This will be called on every change detection run, but it only changes when the diameter is changed. It might be better if we used a regular property that gets updated once per input change.

Yes, missed this one. Updated the PR with the corrections.

…than or equal to 10 do not show Binds the spinner rotate animation name to the circle element diameter, thereby fixing broken spinner animations introduced in the previous commit. Allows float point values to be used for custom diameter and stroke width. Fixes angular#19469

…than or equal to 10 do not show Correcting circle diameter getter usage and public api definition. Fixes angular#19469

AlexLangdon · 2020-07-29T00:49:35Z

Something to keep in mind: even though the changes to the circle sizing are correct, they might make the PR harder to land, because they'll break people's screenshot tests.

Would it be preferable to split this into separate PRs? Keep this PR just for supporting diameters of <= 10 along with float values, which should not break many screenshot tests as it is adding support for a larger range of inputs. I can open a separate issue with another PR to correct the circle radius calculation relative to current stroke width.

crisbeto · 2020-07-29T06:40:31Z

Yes, it would be easier to get it merged in as separate PRs.

AlexLangdon · 2020-08-13T00:05:00Z

Opened PRs #20193 and #20192, which fix spinner issues with diameter <= 10 (#19469) and floating point diameters respectively (#20158), and shouldn't add breaking changes.

Right now, I can’t see a neat alternative that corrects the spinner radius calculation (#20159) but does not fix both of the above issues or potentially break screenshot tests as a result.

Closing this PR in favour of an updated version #20289.

angular-automatic-lock-bot · 2020-09-13T00:09:55Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

AlexLangdon requested review from crisbeto and jelbourn as code owners July 21, 2020 22:15

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 21, 2020

AlexLangdon commented Jul 21, 2020

View reviewed changes

crisbeto reviewed Jul 22, 2020

View reviewed changes

AlexLangdon force-pushed the small-progress-spinner-fix branch from 2b14d9a to d6b0818 Compare July 27, 2020 15:49

AlexLangdon force-pushed the small-progress-spinner-fix branch from d6b0818 to 3ba2335 Compare July 27, 2020 16:52

crisbeto reviewed Jul 27, 2020

View reviewed changes

AlexLangdon force-pushed the small-progress-spinner-fix branch from 3ba2335 to 2e3197c Compare July 29, 2020 00:10

AlexLangdon and others added 2 commits July 29, 2020 01:26

Merge branch 'master' into small-progress-spinner-fix

3eff69b

fix(material/progress-spinner): progress-spinners with diameter less …

2746978

…than or equal to 10 do not show Correcting circle diameter getter usage and public api definition. Fixes angular#19469

AlexLangdon mentioned this pull request Aug 2, 2020

bug(MatProgressSpinner): Progress spinner circle expands beyond the specified diameter #20159

Closed

AlexLangdon mentioned this pull request Aug 13, 2020

fix(material/progress-spinner): spinner circle expands beyond the specified diameter #20289

Closed

AlexLangdon closed this Aug 13, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(material/progress-spinner): progress-spinners with diameter less than or equal to 10 do not show #20062

fix(material/progress-spinner): progress-spinners with diameter less than or equal to 10 do not show #20062

AlexLangdon commented Jul 21, 2020 •

edited

AlexLangdon Jul 21, 2020

AlexLangdon Aug 4, 2020

crisbeto left a comment

AlexLangdon commented Jul 22, 2020

AlexLangdon commented Jul 27, 2020

crisbeto left a comment

crisbeto Jul 27, 2020

AlexLangdon Jul 29, 2020

AlexLangdon commented Jul 29, 2020 •

edited

crisbeto commented Jul 29, 2020

AlexLangdon commented Aug 13, 2020

angular-automatic-lock-bot bot commented Sep 13, 2020

fix(material/progress-spinner): progress-spinners with diameter less than or equal to 10 do not show #20062

fix(material/progress-spinner): progress-spinners with diameter less than or equal to 10 do not show #20062

Conversation

AlexLangdon commented Jul 21, 2020 • edited

AlexLangdon Jul 21, 2020

Choose a reason for hiding this comment

AlexLangdon Aug 4, 2020

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

AlexLangdon commented Jul 22, 2020

AlexLangdon commented Jul 27, 2020

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto Jul 27, 2020

Choose a reason for hiding this comment

AlexLangdon Jul 29, 2020

Choose a reason for hiding this comment

AlexLangdon commented Jul 29, 2020 • edited

crisbeto commented Jul 29, 2020

AlexLangdon commented Aug 13, 2020

angular-automatic-lock-bot bot commented Sep 13, 2020

AlexLangdon commented Jul 21, 2020 •

edited

AlexLangdon commented Jul 29, 2020 •

edited