If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 33, Firefox OS v2.1

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cthedot, Assigned: mats)

Tracking

({regression})

Trunk
mozilla35
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32+ wontfix, firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, firefox-esr31 unaffected, b2g-v2.0 affected, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
Created attachment 8489107 [details]
Capture.PNG

Comment 2

3 years ago
[Tracking Requested - why for this release]: regression
Status: UNCONFIRMED → NEW
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
Ever confirmed: true
Keywords: regression

Updated

3 years ago
Component: Untriaged → Layout
Product: Firefox → Core

Comment 3

3 years ago
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
Blocks: 613659

Updated

3 years ago
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 32 Branch → Trunk
(Assignee)

Updated

3 years ago
Assignee: nobody → mats
tracking-firefox32: --- → +
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
(Assignee)

Comment 4

3 years ago
Created attachment 8490092 [details] [diff] [review]
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
Attachment #8490092 - Flags: review?(roc)
Attachment #8490092 - Flags: review?(roc) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb22d1e4d640
Flags: in-testsuite+
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?
Flags: needinfo?(mats)
https://hg.mozilla.org/mozilla-central/rev/fb22d1e4d640
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 8

3 years ago
(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.
Flags: needinfo?(mats)
Thanks for the assessment mats. I'm marking 32 as won't fix based on comment 8.
status-firefox32: affected → wontfix
(Assignee)

Comment 10

3 years ago
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
Attachment #8490092 - Flags: approval-mozilla-beta?
Attachment #8490092 - Flags: approval-mozilla-aurora?
Attachment #8490092 - Flags: approval-mozilla-beta?
Attachment #8490092 - Flags: approval-mozilla-beta+
Attachment #8490092 - Flags: approval-mozilla-aurora?
Attachment #8490092 - Flags: approval-mozilla-aurora+
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)?
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox33: affected → fixed
status-firefox34: affected → fixed
Flags: needinfo?(mats)
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
status-firefox33: fixed → affected
Keywords: branch-patch-needed
(Assignee)

Comment 13

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

I'll take a look...
Flags: needinfo?(mats)
(Assignee)

Comment 14

3 years ago
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
status-firefox33: affected → fixed
Thanks! What are your thoughts on b2g32?
Keywords: branch-patch-needed
(Assignee)

Comment 16

3 years ago
(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.
b2g32 is still open for approved patches, it'll just need to go through the approval process as usual.
(Assignee)

Comment 18

3 years ago
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.
Attachment #8490092 - Flags: approval-mozilla-b2g32?
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.
Attachment #8490092 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32-
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
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Created attachment 8531891 [details]
2014-12-04-01-39-08_Flame22.png
Created attachment 8531894 [details]
2014-12-04-14-37-13_Flame21.png
You need to log in before you can comment on or make changes to this bug.