26.18 KB, image/png
20.24 KB, patch
|Details | Diff | Splinter Review|
56.73 KB, image/png
75.14 KB, image/png
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
[Tracking Requested - why for this release]: regression
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
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
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?
(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.
Thanks for the assessment mats. I'm marking 32 as won't fix based on comment 8.
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
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)?
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
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12) > Backed out from beta for bustage. I'll take a look...
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
Thanks! What are your thoughts on b2g32?
(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.
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 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.
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