border-radius on <img> tag doesn't always load initially

VERIFIED FIXED in Firefox 68

Status

()

defect
VERIFIED FIXED
3 months ago
3 months ago

People

(Reporter: teuneboon, Assigned: emilio)

Tracking

(Regression, {regression, reproducible})

68 Branch
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox67 unaffected, firefox67.0.1 unaffected, firefox68+ verified, firefox69+ verified)

Details

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Made an image with a border-radius of 50% like so: <img src="https://www.gravatar.com/avatar/9916c685fb33f3692cca815527058d34?s=32&amp;d=mm" style="border-radius: 50%;">
  2. Load the page this image is on

Actual results:

Image is shown, but doesn't have a border radius, it's just a normal square. Once I try to use inspect element on it, it suddenly has a border radius.

It's seemingly randomly reproducible on: https://jsfiddle.net/jzotx46v/1/. Sometimes it initially seems to load with border radius there, and sometimes it's just a square(pressing the "Run" button resets it to being square for me). It's always reproducible if I make a new tab "about:blank" and use the developer tools to just inject the problematic HTML in there.

Expected results:

Image should have a border radius

I can reproduce the issue on Nightly69.0a1 Windows10.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=94682e23ba118dcfab3843a3280618b2e1686ecb&tochange=fdc7b66d76a4ded62d0292bef4de0ffb090d9587

Regressed by: Bug 1395964

:emilio
Your patch seems to cause the regression. Can you please look into this?

Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Layout: Images, Video, and HTML Frames
Ever confirmed: true
Flags: needinfo?(emilio)
Product: Firefox → Core
Regressed by: 1395964

[Tracking Requested - why for this release]: Somewhat bad regression.

Assignee: nobody → emilio

This is really an issue with bug 1367906.

Flags: needinfo?(emilio)
Blocks: 1554755

This is just a super-low-risk work-around for bug 1554755 that we can uplift to
beta, I'll follow-up in a bit in there with a fix and a test.

The reason my patch breaks this is because my patch stopped making images
reframe.

The reframe was of course a very expensive way of invalidating that bit ;)

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0765f0fa7560
Work-around broken border-radius caching. r=mattwoodrow

Comment on attachment 9067853 [details]
Bug 1554664 - Work-around broken border-radius caching. r=mstange,mattwoodrow

Beta/Release Uplift Approval Request

  • User impact if declined: Some images won't get clipped by border-radius after loading.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: see comment 0, basically just open data:text/html,<img src="https://www.gravatar.com/avatar/9916c685fb33f3692cca815527058d34?s=32&amp;d=mm" style="border-radius: 50%;">
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Ultra-low-risk patch intentionally, to be able to uplift to beta. An optimization introduced by bug 1395964 uncovered a pre-existing, londstanding bug (which I filed and proposed a patch for, see bug 1554755).

The patch just literally makes us re-compute the border radius next time we're asked for it after an image load. So I think it's basically zero-risk.

  • String changes made/needed: none
Attachment #9067853 - Flags: approval-mozilla-beta?
Flags: qe-verify+

A quick quote from IRC, just to better explain what's going on:

00:30 <mattwoodrow> emilio: I don't really understand the correlation between image frames getting a new image, and border-radius
00:32 <@emilio> mattwoodrow: we initially (before the load) lay out the image with 0x0 size, which makes GetBorderRadii compute to zero (since all the radii are 50%)
00:32 <@emilio> mattwoodrow: the image load changes the size of the image, but not the style, which means that the mMayHaveRoundedCorners bit gets out of sync
00:32 <mattwoodrow> emilio: Ah right, so it's ultimately the size change on the frame that matters, image load is just a cause of that?
00:33 <mattwoodrow> Can other frame types have similar issues?
00:33 <@emilio> mattwoodrow: right. The image load used to cause a reframe itself before my patch.
00:33 <@emilio> mattwoodrow: yeah, this is independent of the frame type, see bug 1554755

Comment on attachment 9067853 [details]
Bug 1554664 - Work-around broken border-radius caching. r=mstange,mattwoodrow

regression fix, approved for 68.0b6

Attachment #9067853 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

I have reproduced this issue using Firefox 69.0a1 (2019.05.27) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 69.0a1 (2019.05.29) Win 10 x64, Ubuntu 16.04 x64 and macOS 10.13.6, waiting for fix in beta 68.0b6.

I can confirm this issue is fixed, I verified using Firefox 68.0b6 on Win 10 x64, Ubuntu 16.04 x64 and macOS 10.13.6.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.