Closed Bug 1341703 Opened 4 years ago Closed 3 years ago

stylo: border-image support with url() seems to be broken

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Every single test in layout/reftests/border-image/reftest-stylo.list that uses url() for the image is marked "fails", because the border-image is not actually shown/loaded/whatever afaict...
Priority: -- → P2
CJ, can you have a look at this?
Assignee: nobody → cku
Priority: P2 → P1
Sure
nsCSSRendering::PaintBorderWithStyleBorder(...)
{

   if (aStyleBorder.IsBorderImageLoaded()) {
     // this patch calls nsImageRenderer::PrepareImage, which will trigger full decode.
    ..
    retrun
  }
  
  // else...
  // With stylo enable, we will go into this path, nsImageRenderer::PrepareImage is not called in this path at all.
}

I am  fixing it
Duplicate of this bug: 1356494
Attachment #8858710 - Flags: review?(manishearth)
Attachment #8858753 - Flags: review?(manishearth)
Comment on attachment 8858710 [details]
Bug 1341703 - Part 1. Make sure that the URL stylo parse for border-image-source is propagated to Gecko computed style.

https://reviewboard.mozilla.org/r/130726/#review133314
Attachment #8858710 - Flags: review?(manishearth) → review+
Comment on attachment 8858753 [details]
Bug 1341703 - Part 4. Re-enable bordering reftests.

https://reviewboard.mozilla.org/r/130788/#review133316
Attachment #8858753 - Flags: review?(manishearth) → review+
Blocks: 1357060
Comment on attachment 8858709 [details]
Bug 1341703 - Part 3. Ensure full decode be triggered after style-image was downloaded,

https://reviewboard.mozilla.org/r/130724/#review133370

::: layout/painting/nsCSSRendering.cpp:852
(Diff revision 2)
>    // decoding enabled.
>    if (aStyleBorder.mBorderImageSource.GetType() != eStyleImageType_Null) {
>      result = DrawResult::NOT_READY;
>    }
>  
> +  // Make sure the border image is actually decoding.

How can we be decoding it if it's not even loaded?

This needs much more documentation to describe what's actually going on here and why this call makes any sense at all.
Comment on attachment 8858710 [details]
Bug 1341703 - Part 1. Make sure that the URL stylo parse for border-image-source is propagated to Gecko computed style.

https://reviewboard.mozilla.org/r/130726/#review133372

::: commit-message-72cf6:1
(Diff revision 3)
> +Bug 1341703 - Part 2. Pass true to set_border_image_source

This commit message should describe the _why_ of the change, not the _what_.  The "what" is obvious from the diff, but the "why" is completely unclear.
Attachment #8858710 - Flags: review-
Comment on attachment 8858752 [details]
Bug 1341703 - Part 3. Change the latest parameter of CreateBorderImageRenderer from 'DrawResult *' to 'DrawResult &'.

https://reviewboard.mozilla.org/r/130786/#review133376

Why this change?  We generally want outparams to be obviously outparams...
What is the actual difference in computed styles between what stylo and gecko produce here that necessitates changes to Gecko's drawing code?  Why does this difference exist, and why is working around it in the drawing code, rather than eliminating the difference, the right solution?
Flags: needinfo?(cku)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #19)
> What is the actual difference in computed styles between what stylo and
> gecko produce here that necessitates changes to Gecko's drawing code?  Why
> does this difference exist, and why is working around it in the drawing
> code, rather than eliminating the difference, the right solution?
Take solid-image-1.html as an example(in reftests/border-image), the correct rendering result is we should see a green border.
I can see black border in both enable or disable stylo(load this page without reload). The chance of seeing black border on stylo build is a bit higher.
I also try to use nightly install version on my MBP, the rendering result is not correct as well. So we have Part 1. This patch is not stylo only fix.
> I can see black border in both enable or disable stylo(load this page without reload)

That's... not what I see.  I see a black border with stylo enabled, but a green one with stylo disabled, or in a build without MOZ_STYLO defined.

Maybe there's a race here, that my builds are all winning and yours is losing?  But my point was that code comments or commit message or something need to explain what is actually being changed and why.

Also, I am pretty sure we have code to handle this so it _should_ work.  See the AssociateRequestToFrame calls in nsFrame::DidSetStyleContext and the bits in ImageLoader::OnFrameComplete that should lead to a repaint of the frame, which will land back in the cssrendering code, but now with IsBorderImageLoaded() true.  Which part of that is not happening with stylo?
Comment on attachment 8858709 [details]
Bug 1341703 - Part 3. Ensure full decode be triggered after style-image was downloaded,

https://reviewboard.mozilla.org/r/130724/#review133370

> How can we be decoding it if it's not even loaded?
> 
> This needs much more documentation to describe what's actually going on here and why this call makes any sense at all.

The name of this function,nsStyleImage::StartDecoding, is kind of misleading.
1.
Take background-image paiting path as an example, here is how background painting
* nsCSSRendering::PaintStyleImageLayerWithSC be called first to paint all layers
* nsCSSRendering::PrepareImageLayer: paint each image layer 
* nsImageRenderer::PrepareImage()
  In this function
  if (!mImage->IsComplete()) {
    // Make sure the image is actually decoding.
    bool frameComplete = mImage->StartDecoding();
  StartDecoding is not called after download, I will explain in #2

StartDecoding is always called in this path. What I am saying so far is I try to align the behavior between border-image and background/mask-image painting.

2.
Back to nsStyleImage::StartDecoding, this function will eventually call RasterImage/VectorImage::StartDecodingWithResult to do the real thing[1][2]. 
Take RasterImage::StartDecodingWithResult as an example, like you said, this image is not loaded yet, so we early return at [3]. The key point here is we set mWantFullDecode as true here, so after image downloaded we will trigger full decode.

Maybe rename it to ShouldTriggerFullDecodeAfterLoaded? mstange may give a better suggestion

[1] http://searchfox.org/mozilla-central/source/image/RasterImage.cpp#1124
[2] http://searchfox.org/mozilla-central/source/image/VectorImage.cpp#1055
[3] http://searchfox.org/mozilla-central/source/image/RasterImage.cpp#1131
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #22)
> > I can see black border in both enable or disable stylo(load this page without reload)
> 
> That's... not what I see.  I see a black border with stylo enabled, but a
> green one with stylo disabled, or in a build without MOZ_STYLO defined.
I ask three colleagues to do this test on no-stylo nightly build
one said he always see black(macos release version)
one said he always see green, but after  he create a new profile, he see black border.(macos release version)
one said he always see green.(macos release version)
mine is linux debug version
Flags: needinfo?(cku)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #22)
> Maybe there's a race here, that my builds are all winning and yours is
> losing?  But my point was that code comments or commit message or something
> need to explain what is actually being changed and why.
Understand, I will fix what you said
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #22)
> Also, I am pretty sure we have code to handle this so it _should_ work.  See
> the AssociateRequestToFrame calls in nsFrame::DidSetStyleContext and the
> bits in ImageLoader::OnFrameComplete that should lead to a repaint of the
> frame, which will land back in the cssrendering code, but now with
> IsBorderImageLoaded() true.  Which part of that is not happening with stylo?
Hope #2 in comment 23 have explained your question. (For a style image, ImageLoader::OnFrameComplete will not be called if we do not call nsStyleImage::StartDecoding beforehand).
(In reply to C.J. Ku[:cjku](UTC+8) from comment #24)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #22)
> > > I can see black border in both enable or disable stylo(load this page without reload)
> > 
> > That's... not what I see.  I see a black border with stylo enabled, but a
> > green one with stylo disabled, or in a build without MOZ_STYLO defined.
> I ask three colleagues to do this test on no-stylo nightly build
> one said he always see black(macos release version)
> one said he always see green, but after  he create a new profile, he see
> black border.(macos release version)
> one said he always see green.(macos release version)
> mine is linux debug version

I reread the chatting log in skype, update more correct testing result
From TY
Ting-Yu Lin: Nightly load 第一次黑, refresh 變綠
Translation: using nightly, see black border at the first load, refresh then see green
This is what I expect.

From Tommy Kuo
Tommy Kuo [:KuoE0]: 我沒有 refresh 就綠欸
Translation: always green
His testing result is just like yours.

Astley Chen: 我nightly:green, beta:black
Translation: nightly:green, beta:black
Astley Chen: @CJ Ku 我剛用clean profile後,就是黑的了
Translation: @CJ Ku By using clean profile, see black
I don't know why.....
So the problem is that nothing signals to imagelib that decoding even needs to start?  Why does it get started at all in some cases, then?  I guess only when the image manages to somehow load before we need to paint it, so then we request decode?

I did manage to reproduce the incorrect painting without stylo with this testcase:

  <div style="border: 100px solid black; border-image: url(https://software.hixie.ch/utilities/js/live-dom-viewer/delayed-image)"></div>

where the image load has a 2-second delay built in.  So I agree that there is a non-stylo problem here, and stylo is just tickling it, perhaps due to timing changes in when it starts image loads...  I wonder whether we can have such a delayed-load reftest...
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #28)
> where the image load has a 2-second delay built in.  So I agree that there
> is a non-stylo problem here, and stylo is just tickling it, perhaps due to
> timing changes in when it starts image loads...  I wonder whether we can
> have such a delayed-load reftest...
I think we can, since we pass sync-decode flag in reftest harness. I will try to add this test.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #28)
> So the problem is that nothing signals to imagelib that decoding even needs
> to start?  Why does it get started at all in some cases, then?  I guess only
Such as why we don't need to do it in nsImageFrame paining? To be honest, I don't know yet. But I will look into it.
Attachment #8858752 - Flags: review?(mstange)
Comment on attachment 8858752 [details]
Bug 1341703 - Part 3. Change the latest parameter of CreateBorderImageRenderer from 'DrawResult *' to 'DrawResult &'.

https://reviewboard.mozilla.org/r/130786/#review133376

ok, I think I should handle DrawResult thing in bug 1351440 at once. Please ignore this patch
Attachment #8858752 - Attachment is obsolete: true
Attachment #8859012 - Flags: review?(cam)
Comment on attachment 8858710 [details]
Bug 1341703 - Part 1. Make sure that the URL stylo parse for border-image-source is propagated to Gecko computed style.

https://reviewboard.mozilla.org/r/130726/#review133372

> This commit message should describe the _why_ of the change, not the _what_.  The "what" is obvious from the diff, but the "why" is completely unclear.

I think the new commit msg should explain why I did this change
Comment on attachment 8859012 [details]
Bug 1341703 - Part 3. Handle nsStyleBorder::CalcDifference off main thread.

https://reviewboard.mozilla.org/r/131030/#review133594

r=me with the NeutralChange block restored.

::: commit-message-c0393:7
(Diff revision 2)
> +The drawback of this change is we may paint a border-image, once, even before it
> +was actaully loaded, which is a waste, because we have nothing to draw yet.

Yeah, I guess it's fine, if we're not doing this for background or mask too.

(If we did want to handle this a bit better, at least for when stylo is in use, we could check whether the nsStyleImageRequest object has resolved its imgRequestProxy object yet, since if its mRequestProxy is null, then we definitely don't have an image to paint, and that's a check we can do off the main thread.  It also would work well with the way nsStyleImageRequest is resolved after restyling, back in the main thread, since if we just restyled from border-image:none to border-image:url(something), and we're just about to start loading that URL, then we could avoid repainting.  But I would wouldn't bother doing this unless we know this repainting is a problem.)

::: layout/style/nsStyleStruct.cpp
(Diff revision 2)
> -  // mBorderImage* fields are checked only when border image was
> -  // actualy loaded. But we need to return neutral change even when
> -  // they are not actually used.
> -  if (mBorderImageSource  != aNewData.mBorderImageSource  ||
> -      mBorderImageRepeatH != aNewData.mBorderImageRepeatH ||
> -      mBorderImageRepeatV != aNewData.mBorderImageRepeatV ||
> -      mBorderImageSlice   != aNewData.mBorderImageSlice   ||
> -      mBorderImageFill    != aNewData.mBorderImageFill    ||
> -      mBorderImageWidth   != aNewData.mBorderImageWidth) {
> -    return nsChangeHint_NeutralChange;
> -  }

I don't think it's correct to remove this with the changed condition above, since if both the old and new mBorderImageSource values have an eStyleImageType_Null value, then we still need to indicate to the caller if these other properties have changed values.

So please restore this block and update its comment to say that we check these fields above "only when border-image is not 'none'".
Attachment #8859012 - Flags: review?(cam) → review+
Comment on attachment 8858709 [details]
Bug 1341703 - Part 3. Ensure full decode be triggered after style-image was downloaded,

https://reviewboard.mozilla.org/r/130724/#review133370

> The name of this function,nsStyleImage::StartDecoding, is kind of misleading.
> 1.
> Take background-image paiting path as an example, here is how background painting
> * nsCSSRendering::PaintStyleImageLayerWithSC be called first to paint all layers
> * nsCSSRendering::PrepareImageLayer: paint each image layer 
> * nsImageRenderer::PrepareImage()
>   In this function
>   if (!mImage->IsComplete()) {
>     // Make sure the image is actually decoding.
>     bool frameComplete = mImage->StartDecoding();
>   StartDecoding is not called after download, I will explain in #2
> 
> StartDecoding is always called in this path. What I am saying so far is I try to align the behavior between border-image and background/mask-image painting.
> 
> 2.
> Back to nsStyleImage::StartDecoding, this function will eventually call RasterImage/VectorImage::StartDecodingWithResult to do the real thing[1][2]. 
> Take RasterImage::StartDecodingWithResult as an example, like you said, this image is not loaded yet, so we early return at [3]. The key point here is we set mWantFullDecode as true here, so after image downloaded we will trigger full decode.
> 
> Maybe rename it to ShouldTriggerFullDecodeAfterLoaded? mstange may give a better suggestion
> 
> [1] http://searchfox.org/mozilla-central/source/image/RasterImage.cpp#1124
> [2] http://searchfox.org/mozilla-central/source/image/VectorImage.cpp#1055
> [3] http://searchfox.org/mozilla-central/source/image/RasterImage.cpp#1131

In the new version, I didn't manually call StartDecoding, but changing the condition of creating border renderer, which have the same result. And add more detailed reason of why this change is needed in the commit message.
Comment on attachment 8858710 [details]
Bug 1341703 - Part 1. Make sure that the URL stylo parse for border-image-source is propagated to Gecko computed style.

https://reviewboard.mozilla.org/r/130726/#review134174

::: commit-message-72cf6:1
(Diff revision 5)
> +Bug 1341703 - Part 1. Pass nsStyleImage's URL value of a border-image from stylo to gecko

Might be worth saying that we want to pass the URL in addition to the image or something?

::: servo/components/style/properties/gecko.mako.rs:915
(Diff revision 5)
>              Gecko_SetNullImageValue(&mut self.gecko.mBorderImageSource);
>          }
>  
>          if let Some(image) = v.0 {
>              // TODO: We need to make border-image-source match with background-image
>              // until then we are setting with_url to false

This comment no longer matches the code.  What should it really say?
Comment on attachment 8859012 [details]
Bug 1341703 - Part 3. Handle nsStyleBorder::CalcDifference off main thread.

https://reviewboard.mozilla.org/r/131030/#review134176

::: commit-message-c0393:8
(Diff revision 3)
> +Image loading status can be accessed in main thread only, so we can not use
> +nsStyleImage::IsLoaded in nsStyleBorder::CalcDifference, which might be executed
> +on a background thread.
> +
> +The drawback of this change is we may paint a border-image, once, even before it
> +was actaully loaded, which is a waste, because we have nothing to draw yet.

"actually"
Comment on attachment 8858710 [details]
Bug 1341703 - Part 1. Make sure that the URL stylo parse for border-image-source is propagated to Gecko computed style.

https://reviewboard.mozilla.org/r/130726/#review134174

> Might be worth saying that we want to pass the URL in addition to the image or something?

Hmm, I think the following comment might describe what this patch did more precisely:
Part 1: Pass URL fo border-image-source, that get from stylo, into nsStyleBorder::mBorderImageSource in gecko.

> This comment no longer matches the code.  What should it really say?

It can be removed.
Hi Nazim,
Regarding to the comment at [1] that you made in #13989:
  // TODO: We need to make border-image-source match with background-image
  // until then we are setting with_url to false
I am going to pass with_url as true in the next line, may I know is there anything I should do before remove this comment?

[1] http://searchfox.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs#916
Flags: needinfo?(canaltinova)
> Pass URL fo border-image-source, that get from stylo, into nsStyleBorder::mBorderImageSource in gecko.

Maybe:

  Make sure that the URL stylo parses for border-image-source is propagated to Gecko computed style.

or something?  Not sure whether that matches what you're trying to say....
Hi C.J.,
If it's implemented, we can just remove this comment. Also I think we can remove this with_url parameter completely from this method[1] since other images already passing true to this[2]. 

[1] https://dxr.mozilla.org/servo/rev/2f8d9013a09b7b6dfad253a2438b9810d6d077dc/components/style/gecko/conversions.rs#104
[2] https://dxr.mozilla.org/servo/rev/2f8d9013a09b7b6dfad253a2438b9810d6d077dc/components/style/properties/gecko.mako.rs#2462,2467
Flags: needinfo?(canaltinova)
(In reply to Nazım Can Altınova [:canaltinova] from comment #49)
> Hi C.J.,
> If it's implemented, we can just remove this comment. Also I think we can
> remove this with_url parameter completely from this method[1] since other
> images already passing true to this[2]. 
> 
> [1]
> https://dxr.mozilla.org/servo/rev/2f8d9013a09b7b6dfad253a2438b9810d6d077dc/
> components/style/gecko/conversions.rs#104
> [2]
> https://dxr.mozilla.org/servo/rev/2f8d9013a09b7b6dfad253a2438b9810d6d077dc/
> components/style/properties/gecko.mako.rs#2462,2467
Thanks! Will do
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #48)
> > Pass URL fo border-image-source, that get from stylo, into nsStyleBorder::mBorderImageSource in gecko.
> 
> Maybe:
> 
>   Make sure that the URL stylo parses for border-image-source is propagated
Can't be better, thanks, Boris
Attachment #8859627 - Flags: review?(canaltinova)
Comment on attachment 8859627 [details]
Bug 1341703 - Part 2. Remove the second parameter(with_url) of nsStyleImage::set.

https://reviewboard.mozilla.org/r/131648/#review134386

Thanks!
Attachment #8859627 - Flags: review?(canaltinova) → review+
Attachment #8858709 - Flags: review?(mstange)
Move part 3 to Bug 1359844 so that we can land part 4 and 5 indepedently. And remove Part 1/ 2, since they are going to be a git pull.
Blocks: 1359844
Attachment #8858710 - Attachment is obsolete: true
Attachment #8859627 - Attachment is obsolete: true
Attachment #8858709 - Attachment is obsolete: true
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fe2bbc74895
Part 3. Handle nsStyleBorder::CalcDifference off main thread. r=heycam
https://hg.mozilla.org/integration/autoland/rev/66fbc3cef3e9
Part 4. Re-enable bordering reftests. r=manishearth
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f492ae7e816d
Re-skip a frequently failing reftest on stylo a=bustage
You need to log in before you can comment on or make changes to this bug.