Closed
Bug 1265715
Opened 8 years ago
Closed 8 years ago
background-clip:text and transform don't interact correctly (number icons disappear in http://giphy.com/create/gifcaption )
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: u459114, Assigned: u459114)
References
Details
Attachments
(6 files, 9 obsolete files)
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
1.20 KB,
patch
|
u459114
:
review+
|
Details | Diff | Splinter Review |
Visit http://giphy.com/create/gifcaption in Nightly(background-clip:text and -webkit-text-fill-color supported), the number icons on the left-top corner of each gif sprite disappear. We do not handle transform well with background-clip:text transform: translate(-50%, -50%) skewX(-10deg)
skewX(-10deg) is not relative. translate is the reason why we can't see number glyphs.
Summary: Number icons disappear in http://giphy.com/create/gifcaption → background-clip:text and transform don't interact correctly (number icons disappear in http://giphy.com/create/gifcaption)
Attachment #8743120 -
Attachment is obsolete: true
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Assignee | ||
Comment 10•8 years ago
|
||
More complex then I though, so it takes one more extra day to find out solution. Still need construct several reftests before asking for review.
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8743121 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8744323 -
Flags: review?(jfkthame)
Attachment #8744324 -
Flags: review?(jfkthame)
Attachment #8744370 -
Flags: review?(jfkthame)
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Assignee | ||
Comment 19•8 years ago
|
||
Hi Jonathan, There are two changes in these patches. 1. ClipBackgroundByText now uses nsLayoutUtils::PaintFrame. To be able to use that function, we need Part 1. 2. Fix transform bug. The code which really fix wrong rendering result on http://giphy.com/create/gifcaption is located in Part 2. nsFrame.cpp if (!aBuilder->IsForGenerateGlyphPath()) { nsDisplayTransform *transformItem = new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList, dirtyRect); resultList.AppendNewToTop(transformItem); } The reason of preventing creating this nsDisplayTransform is we already have that transform information apply into aCtx of nsDisplayBackgroundImage::PaintInternal, so we should not create one more again.
Comment 20•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; https://reviewboard.mozilla.org/r/48463/#review45447 ::: layout/base/nsLayoutUtils.h:1073 (Diff revision 4) > static nsresult PaintFrame(nsRenderingContext* aRenderingContext, nsIFrame* aFrame, > const nsRegion& aDirtyRegion, nscolor aBackstop, > - uint32_t aFlags = 0); > + uint32_t aFlags = 0, > + uint32_t aBuilderFlag = 0); Can we use nsDisplayListBuilder::Mode for the new parameter here, instead of passing a uint32_t? The pattern here makes it very easy for someone to pass a flag in the wrong parameter, and the compiler will have no idea there's a mismatch. Also, looks like it should be called aBuilderMode, not aBuilderFlag; it's not actually a 'flags' word but a single mode value. You should also add the new parameter to the comment above. If it uses the specific nsDisplayListBuilder::Mode type, it won't need much extra documentation; just something like @param aBuilderMode Passed through to the display-list builder. (Even better if we also converted the existing flags parameter to an enum class, but that's probably a large enough change to deserve its own separate bug.)
Attachment #8744323 -
Flags: review?(jfkthame)
Assignee | ||
Comment 21•8 years ago
|
||
https://reviewboard.mozilla.org/r/48463/#review45447 > Can we use nsDisplayListBuilder::Mode for the new parameter here, instead of passing a uint32_t? The pattern here makes it very easy for someone to pass a flag in the wrong parameter, and the compiler will have no idea there's a mismatch. > > Also, looks like it should be called aBuilderMode, not aBuilderFlag; it's not actually a 'flags' word but a single mode value. > > You should also add the new parameter to the comment above. If it uses the specific nsDisplayListBuilder::Mode type, it won't need much extra documentation; just something like > > @param aBuilderMode Passed through to the display-list builder. > > (Even better if we also converted the existing flags parameter to an enum class, but that's probably a large enough change to deserve its own separate bug.) By doing so, we need in include "nsDisplayList.h" in nsLayoutUtils.h, since there is no way to forward delcare an in-class enum class nsDisplayListBuilder { enum Mode { } } I hesiate a bit to do that(include "nsDisplayList.h" in nsLayoutUtils.h). OK with all the other suggestion
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to C.J. Ku[:cjku] from comment #21) > https://reviewboard.mozilla.org/r/48463/#review45447 > > > Can we use nsDisplayListBuilder::Mode for the new parameter here, instead of passing a uint32_t? The pattern here makes it very easy for someone to pass a flag in the wrong parameter, and the compiler will have no idea there's a mismatch. > > > > Also, looks like it should be called aBuilderMode, not aBuilderFlag; it's not actually a 'flags' word but a single mode value. > > > > You should also add the new parameter to the comment above. If it uses the specific nsDisplayListBuilder::Mode type, it won't need much extra documentation; just something like > > > > @param aBuilderMode Passed through to the display-list builder. > > > > (Even better if we also converted the existing flags parameter to an enum class, but that's probably a large enough change to deserve its own separate bug.) > > By doing so, we need in include "nsDisplayList.h" in nsLayoutUtils.h, since > there is no way to forward delcare an in-class enum > class nsDisplayListBuilder { > enum Mode { > } > } > > I hesiate a bit to do that(include "nsDisplayList.h" in nsLayoutUtils.h). OK > with all the other suggestion How about pull Mode out of nsDisplayListBuilder, /* in nsDisplayList.h */ enum nsDisplayListBuilderMode { } class nsDisplayListBuilder { } Then, we can forward delare "enum nsDisplayListBuilderMode" in nsLayoutUtils.h I can have a separate patch to fix that.
Flags: needinfo?(jfkthame)
Comment 24•8 years ago
|
||
(Make that "enum class nsDisplayListBuilderMode" for better type checking.)
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Updated•8 years ago
|
Attachment #8744323 -
Flags: review?(jfkthame) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; https://reviewboard.mozilla.org/r/48463/#review45523 ::: layout/base/nsDisplayList.h:155 (Diff revision 5) > > nsIFrame* mFrame; > AnimatedGeometryRoot* mParentAGR; > }; > > +enum nsDisplayListBuilderMode : uint8_t { Please make this "enum class", so that the values don't end up being available unprefixed. This will require prefixing them with "nsDisplayListBuilderMode::" wherever they're used, but given the various enums we have in the codebase with similar-sounding (but not necessarily identical) values, that's a Good Thing. r=me with that change.
Comment 31•8 years ago
|
||
Comment on attachment 8744324 [details] MozReview Request: Bug 1265715 - Part 2. Add nsDisplayListBuilderMode parameter into nsLayoutUtils::PaintFrame; https://reviewboard.mozilla.org/r/48465/#review45525 ::: layout/base/nsLayoutUtils.h:44 (Diff revision 5) > class nsIAtom; > class nsIScrollableFrame; > class nsIDOMEvent; > class nsRegion; > class nsDisplayListBuilder; > +enum nsDisplayListBuilderMode : uint8_t; This will become "enum class" to match the requested change in patch 1. ::: layout/base/nsLayoutUtils.h:1088 (Diff revision 5) > */ > static nsresult PaintFrame(nsRenderingContext* aRenderingContext, nsIFrame* aFrame, > const nsRegion& aDirtyRegion, nscolor aBackstop, > - uint32_t aFlags = 0); > + nsDisplayListBuilderMode aBuilderMode, > + uint32_t aFlags = 0 > + ); We don't normally put the closing paren on its own line.
Attachment #8744324 -
Flags: review?(jfkthame) → review+
Updated•8 years ago
|
Attachment #8744370 -
Flags: review?(jfkthame) → review+
Comment 32•8 years ago
|
||
Comment on attachment 8744370 [details] MozReview Request: Bug 1265715 - Part 3. Use nsLayoutUtils::PaintFrame in ClipBackgroundByText; https://reviewboard.mozilla.org/r/48501/#review45527
Comment 33•8 years ago
|
||
Comment on attachment 8744873 [details] MozReview Request: Bug 1265715 - Part 4. Fix transform problem; https://reviewboard.mozilla.org/r/48709/#review45533
Attachment #8744873 -
Flags: review?(jfkthame) → review+
Comment 34•8 years ago
|
||
Comment on attachment 8744874 [details] MozReview Request: Bug 1265715 - Part 5. bg-clip:text transform reftest; https://reviewboard.mozilla.org/r/48711/#review45553 ::: layout/reftests/backgrounds/background-clip-text-1f.html:19 (Diff revision 1) > + font-size: 100px; > + font-family: serif; > + position: absolute; > + left: 100px; > + top: 100px; > + transform: translate(-100px, -100px); To make this test transforms a little bit harder, how about nesting them? I propose adding something like body { transform: scale(1, 0.5); } to the testcase, and then compensating for that in the transform here: transform: translate(-100px, 50px) scale(1, 2); which should give the same result AFAICT. ::: layout/reftests/backgrounds/reftest.list:173 (Diff revision 1) > > pref(layout.css.background-clip-text.enabled,true) fuzzy-if(winWidget,1,44) == background-clip-text-1a.html background-clip-text-1-ref.html > pref(layout.css.background-clip-text.enabled,true) fuzzy-if(winWidget,1,44) == background-clip-text-1b.html background-clip-text-1-ref.html > pref(layout.css.background-clip-text.enabled,true) fuzzy-if(winWidget,1,44) == background-clip-text-1c.html background-clip-text-1-ref.html > pref(layout.css.background-clip-text.enabled,true) fuzzy-if(winWidget,1,44) == background-clip-text-1d.html background-clip-text-1-ref.html > +pref(layout.css.background-clip-text.enabled,true) fuzzy-if(winWidget,1,44) == background-clip-text-1f.html background-clip-text-1-ref.html What happened to test 1e? :) It seems confusing to have a gap in the naming sequence.
Attachment #8744874 -
Flags: review?(jfkthame)
Assignee | ||
Comment 35•8 years ago
|
||
https://reviewboard.mozilla.org/r/48711/#review45553 > To make this test transforms a little bit harder, how about nesting them? I propose adding something like > > body { transform: scale(1, 0.5); } > > to the testcase, and then compensating for that in the transform here: > > transform: translate(-100px, 50px) scale(1, 2); > > which should give the same result AFAICT. Sure. I had tried. Pass > What happened to test 1e? :) It seems confusing to have a gap in the naming sequence. ....d follow by e.. I should already learned it from the sound of music... will correct it
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment 41•8 years ago
|
||
Comment on attachment 8744874 [details] MozReview Request: Bug 1265715 - Part 5. bg-clip:text transform reftest; https://reviewboard.mozilla.org/r/48711/#review45569 Great to see this happening! :)
Attachment #8744874 -
Flags: review?(jfkthame) → review+
Comment 42•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a267a1cee49 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf77eabb455e https://hg.mozilla.org/integration/mozilla-inbound/rev/14410850187e https://hg.mozilla.org/integration/mozilla-inbound/rev/d29d98a86d25 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3785624cfc
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a267a1cee49 https://hg.mozilla.org/mozilla-central/rev/bf77eabb455e https://hg.mozilla.org/mozilla-central/rev/14410850187e https://hg.mozilla.org/mozilla-central/rev/d29d98a86d25 https://hg.mozilla.org/mozilla-central/rev/0a3785624cfc
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Target Milestone: mozilla48 → mozilla49
Comment 44•8 years ago
|
||
[Tracking Requested - why for this release]: This is a bug in the newly-implemented 'background-clip:text' feature (bug 759568). Although it's currently disabled for release builds, IIUC, we should still include this fix in the same version as the initial landing rather than leaving developers with a somewhat-broken feature until the next train. CJ, will you be requesting uplift for the patches here?
status-firefox48:
--- → affected
tracking-firefox48:
--- → ?
Assignee | ||
Comment 45•8 years ago
|
||
I think so, the plan I heard from Astley is we want to turn on bg-clip:text and webkit-prefix in 48(release version). So, any blocker of bug 1264905 need to be uplift before feature shipping. So, yes, will do
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Attachment #8745929 -
Attachment is obsolete: true
Assignee | ||
Comment 52•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; Approval Request Comment [Feature/regressing bug #]: bug 759568. This bug exists after we enable backgroun-clip:text [User impact if declined]: background rendering result is over-transformed. [Describe test coverage new/current, TreeHerder]: has a reftest for this change. [Risks and why]: Low risk. It only change how we treat transform while rendering background-clip:text. [String/UUID change made/needed]:n/a
Attachment #8744323 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 53•8 years ago
|
||
All the patches, from Part 1 to Part 5, need to be uplifted.
Assignee | ||
Comment 54•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; Wait. I think I might do something wrong in this patch.
Attachment #8744323 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 55•8 years ago
|
||
Too bad, dirty region in Part 3 is not correct: nsLayoutUtils::PaintFrame(aContext, aFrame, aFrame->GetRect(), NS_RGB(255, 255, 255), Where correct region should be: nsLayoutUtils::PaintFrame(aContext, aFrame, nsRect(nsPoint(0, 0), aFrame->GetSize()), NS_RGB(255, 255, 255), My test cases can not help me to catch this fault is a problem, need one more test case here. Will update a patch tomorrow after more test.
Assignee | ||
Comment 56•8 years ago
|
||
Attachment #8748321 -
Flags: review+
Assignee | ||
Comment 57•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f6e41e3250980ad72367d2e4f123f3c7bc0cbe Bug 1265715 - followup - Correct dirty region; r=me
Assignee | ||
Comment 58•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; [Feature/regressing bug #]: bug 759568. This bug exists after we enable backgroun-clip:text [User impact if declined]: background rendering result is over-transformed. [Describe test coverage new/current, TreeHerder]: has a reftest for this change. [Risks and why]: Low risk. It only change how we treat transform while rendering background-clip:text. [String/UUID change made/needed]:n/a Include Part 1 ~ Part 6, all six patches are required.
Attachment #8744323 -
Flags: approval-mozilla-aurora?
Comment 59•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9f6e41e3250
Comment 61•8 years ago
|
||
Comment on attachment 8744323 [details] MozReview Request: Bug 1265715 - Part 1. Pull Mode out of nsDisplayListBuilder; Fix for recent regression, adds a test, please uplift all 6 patches to aurora.
Attachment #8744323 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 62•8 years ago
|
||
landed on aurora remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/b97d28503652 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/2bc7b2a907e1 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/759e79571fee remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/8a24bdbdb8a4 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/90495a003e1d remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/307b5eca8726
You need to log in
before you can comment on or make changes to this bug.
Description
•