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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 + fixed
firefox49 + fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

Attachments

(6 files, 9 obsolete files)

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
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)
Attached file translate test case (obsolete) —
Attached file translate.html (obsolete) —
Attachment #8743120 - Attachment is obsolete: true
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.
Attached file translation test case (obsolete) —
Attachment #8743121 - Attachment is obsolete: true
Attached file translation test case reference (obsolete) —
Attachment #8744323 - Flags: review?(jfkthame)
Attachment #8744324 - Flags: review?(jfkthame)
Attachment #8744370 - Flags: review?(jfkthame)
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 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)
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
(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)
Yes, that sounds like a good approach.
Flags: needinfo?(jfkthame)
(Make that "enum class nsDisplayListBuilderMode" for better type checking.)
Blocks: 1267209
Attachment #8744323 - Flags: review?(jfkthame) → review+
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 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+
Attachment #8744370 - Flags: review?(jfkthame) → review+
Comment on attachment 8744370 [details]
MozReview Request: Bug 1265715 - Part 3. Use nsLayoutUtils::PaintFrame in ClipBackgroundByText;

https://reviewboard.mozilla.org/r/48501/#review45527
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 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)
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 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+
Target Milestone: mozilla48 → mozilla49
[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?
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
See Also: → 1267697
Attachment #8745929 - Attachment is obsolete: true
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?
All the patches, from Part 1 to Part 5, need to be uplifted.
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?
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.
Attachment #8748321 - Flags: review+
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?
Tracking, recent regression
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+
You need to log in before you can comment on or make changes to this bug.