1067088 - CSS: border-radius for :first-letter broken in Firefox 32.0.1 (regression)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[cthedot]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140911151253

Steps to reproduce:

Style a :first-letter with background-color and border-radius: 100%
Seen in Firefox 32.0.1 on Windows 8.1 (64bit). This used to work, not sure if that was FF 30 or 31. Does still work in IE and Chrome. 

See demo: http://jsbin.com/zesuseyebaqo/1/edit


Actual results:

First-letter has a background which is only round at the top, flat the the bottom. 


Expected results:

First-letter should have round background

Comment 1

[cthedot]

Attachment: Capture.PNG

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: regression

Comment 3

[Alice0775 White]

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e798884d04
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140505105036
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75cafc47ff36
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140505105636
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a4e798884d04&tochange=75cafc47ff36

Suspect: Bug 613659

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix+tests

If GetSkipSides().IsEmpty() then just use "aBorderArea" rather
than the joined area of the continuations.

I'm pretty sure this bug only affects ::first-letter.

https://tbpl.mozilla.org/?tree=Try&rev=be257c19fc3c
https://tbpl.mozilla.org/?tree=Try&rev=b8b9c55bb377

Comment 5

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb22d1e4d640

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

We're very likely going to be releasing Firefox 32.0.2 this week. Given that this issue was reported as a 32.0.1 regression, do you think this fix is safe enough to ride along in 32.0.2?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb22d1e4d640

Comment 8

[Mats Palmgren (inactive)]

(In reply to Lawrence Mandel [:lmandel] from comment #6)
> [...] do you think this fix is safe enough to ride along in 32.0.2?

I think the patch is low-to-medium risk, but the benefit is small so it
doesn't seem worth it.  The bug is a paint issue so it's mostly aesthetic -
it doesn't break the layout of the page or make it unusable.  The usage of
a combination of ::first-letter and border-radius and/or box-shadow is
probably rare (I'm guessing, I have no data to back that up).

I think we should take it on Aurora and Beta though.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks for the assessment mats. I'm marking 32 as won't fix based on comment 8.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8490092 [details] [diff] [review]
fix+tests

Approval Request Comment
[Feature/regressing bug #]: 613659
[User impact if declined]: the first letter of a block of text doesn't render as intended when styled with ::first-letter and border-radius and/or box-shadow.  There's a small risk it might be hard to read that letter, but the issue is mostly aesthetic.
[Describe test coverage new/current, TBPL]: new reftests were added that should cover all the possible cases of ::first-letter that hits this code
[Risks and why]: low-to-medium risk - the patch changes are simple, but pretty much all painting of borders/backgrounds/box-shadows etc goes through here and it's been somewhat error prone historically...
[String/UUID change made/needed]: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44eec4673c25
https://hg.mozilla.org/releases/mozilla-beta/rev/af1dbe183e3d

Is this something that needs tracking for B2G v2.0 (b2g32)?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/f5ba94d7170d

https://tbpl.mozilla.org/php/getParsedLog.php?id=48384185&tree=Mozilla-Beta

Comment 13

[Mats Palmgren (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Backed out from beta for bustage.

I'll take a look...

Comment 14

[Mats Palmgren (inactive)]

I made a trivial addition to make it compile on Beta (33) - it passed the relevant reftests
locally so I pushed it:

+  Sides skipSides = aForFrame->GetSkipSides();
   nsRect clipBorderArea =
-    ::BoxDecorationRectForBorder(aForFrame, aBorderArea, &aBorder);
+    ::BoxDecorationRectForBorder(aForFrame, aBorderArea, skipSides, &aBorder);


https://hg.mozilla.org/releases/mozilla-beta/rev/9f2dc7a2df34

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Thanks! What are your thoughts on b2g32?

Comment 16

[Mats Palmgren (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Thanks! What are your thoughts on b2g32?

I wouldn't recommend it for a point release, see comment 8 for my motivation.
This page says 2.0 was code complete 1 Sept:
https://wiki.mozilla.org/Release_Management/B2G_Landing

If no products based on b2g32 are released yet, and there is still time
to bake the patch, then sure let's take it there too.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

b2g32 is still open for approved patches, it'll just need to go through the approval process as usual.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8490092 [details] [diff] [review]
fix+tests

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 613659
User impact if declined: the first letter of a block of text doesn't render as intended when styled with ::first-letter and border-radius and/or box-shadow.  There's a small risk it might be hard to read that letter, but the issue is mostly aesthetic.
Testing completed: new reftests were added that should cover all the possible cases of ::first-letter that hits this code
Risk to taking this patch (and alternatives if risky): low-to-medium risk - the patch changes are simple, but pretty much all painting of borders/backgrounds/box-shadows etc goes through here and it's been somewhat error prone historically...
String or UUID changes made by this patch: none

See also comment 8 and comment 16.

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 8490092 [details] [diff] [review]
fix+tests

Given the bug comments this looks like a minor aesthetic change. The changes that we are taking on 2.0 needs to be a show-stopper for release at this point. This seems to be something we can live with.

Unless I've misunderstood the impact or the experience on the OS is significantly worse over desktop, this should ride the trains. Otherwise, feel free to re-nom.

Comment 20

[Lance（leave from Mozilla）]

This issue has been verified successfully on Flame 2.1, 2.2.
See attachment: 2014-12-04-01-39-08_Flame22.png and 2014-12-04-14-37-13_Flame21.png
Reproducing rate: 0/5

Step:
1.Launch Browser.
2.Go to "http://jsbin.com/zesuseyebaqo/1/edit".

Actual result:
First-letter have round background.

Flame 2.1 version:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141203.034907
FW-Date         Wed Dec  3 03:49:18 EST 2014
Bootloader      L1TC00011880

Flame 2.2 version:
Gaia-Rev        725685831f5336cf007e36d9a812aad689604695
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2c9781c3e9b5
Build-ID        20141203040207
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141203.072513
FW-Date         Wed Dec  3 07:25:25 EST 2014
Bootloader      L1TC00011880

Comment 21

[Lance（leave from Mozilla）]

Attachment: 2014-12-04-01-39-08_Flame22.png

Comment 22

[Lance（leave from Mozilla）]

Attachment: 2014-12-04-14-37-13_Flame21.png