bug(MatProgressSpinner): Small Spinners with diameter <= 10 don’t show

paulferaud commented 4 years ago

Reproduction

Create a spinner with diameter of 10: https://stackblitz.com/edit/angular-uwttkp?file=src/app/progress-spinner-configurable-example.html

Expected Behavior

See a small spinner

Actual Behavior

Can’t see the spinner

Notes

The spinner does not show because the svg has this element:

<circle cx="50%" cy="50%" r="0" style="animation-name: mat-progress-spinner-stroke-rotate-10; stroke-dasharray: 0px; stroke-width: 10%;" class="ng-star-inserted"></circle>

Notice the r="0". I believe that’s the issue.

It’s set from here: https://github.com/angular/components/blob/fa96c8a8be13da0ebb5c0096f50210c8eadac043/src/material/progress-spinner/progress-spinner.ts#L234 I believe using the BASE_STROKE_WIDTH constant here is wrong?

Environment

Angular: 9.0
CDK/Material: 9.0
Browser(s): Google Chrome Version 81.0.4044.141
Operating System: Chrome OS

AlexLangdon commented 4 years ago

@devversion I can try fixing this, if no one is working on it.

theLeftTenant commented 3 years ago

Any word on a fix? @AlexLangdon some issue with the PR that it wouldn't be accepted?

I have a couple production applications that use a simple loading component built with a mat-spinner inline before "Loading..." or other caller supplied text. The spinner auto-sizes after view init to the offsetHeight of the element ref. This means the spinners are often too small to load because of this issue.

AlexLangdon commented 3 years ago

Any word on a fix? @AlexLangdon some issue with the PR that it wouldn't be accepted?

I have a couple production applications that use a simple loading component built with a mat-spinner inline before "Loading..." or other caller supplied text. The spinner auto-sizes after view init to the offsetHeight of the element ref. This means the spinners are often too small to load because of this issue.

@theLeftTenant This was a while ago, but from what I recall, there was a "correct" way of calculating the spinner radius by subtracting stroke width as opposed to a BASE_STROKE_WIDTH of 10, which would address the problem of not showing spinners of diameter <= 10 (see diff).

The concern at the time was that although this fix is correct it would also affect the radius calculation for all spinners, breaking some people's screenshot tests which depend on the current approach of calculating the radius.

There is a thread of discussion on this https://github.com/angular/components/pull/20062#pullrequestreview-456034861

I did raise another PR which would only change the logic for small spinners of diameter <= 10, though it is not as elegant as the first solution and I am not sure it was reviewed.

angular / components