Closed Bug 1418224 Opened 7 years ago Closed 7 years ago

stylo: support shape-outside: <image>

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Bug 1418224 Part 1 - Change StyleShapeSource::SetURL's return type to void.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1418224 Part 2 - Extract ShapeInfo::CreateBasicShape().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1418224 Part 3 - Add shape-outside: <image> support to style system.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1418224 Part 4 - Fix tests after adding shape-outside: <image> to style system.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR #19415
41 bytes, text/x-github-pull-request
Details | Review 
This bug for adding shape-outside: <image> support to the style system.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78691cba3a8a555fb1624620aab8e4579849c5eb

Latest patch set has fixed wpt8 failure (fetch-request-css-images.https.html).
Comment on attachment 8931279 [details]
Bug 1418224 Part 1 - Change StyleShapeSource::SetURL's return type to void.

https://reviewboard.mozilla.org/r/201208/#review207966
Attachment #8931279 - Flags: review?(cam) → review+
Comment on attachment 8931280 [details]
Bug 1418224 Part 2 - Extract ShapeInfo::CreateBasicShape().

https://reviewboard.mozilla.org/r/201210/#review207970
Attachment #8931280 - Flags: review?(cam) → review+
Comment on attachment 8931281 [details]
Bug 1418224 Part 3 - Add shape-outside: <image> support to style system.

https://reviewboard.mozilla.org/r/201212/#review207972

::: layout/style/nsCSSParser.cpp:16454
(Diff revision 1)
> -  if (ParseSingleTokenVariant(aValue, VARIANT_HUO, nullptr)) {
> -    // 'inherit', 'initial', 'unset', 'none', and <image> url must be alone.
> +  CSSParseResult result =
> +    ParseVariant(aValue, VARIANT_HUO | VARIANT_IMAGE, nullptr);

VARIANT_IMAGE already includes VARIANT_URL and VARIANT_NONE so I think you can just write "VARIANT_IMAGE | VARIANT_INHERIT".

::: layout/style/nsRuleNode.cpp:6420
(Diff revision 1)
>      case eCSSUnit_URL: {
>        display->mShapeOutside = StyleShapeSource();
>        display->mShapeOutside.SetURL(shapeOutsideValue->GetURLStructValue());
>        break;
>      }

Given the changes to nsFloatManager.cpp, where it asserts on URL types, should we be doing something else here?

::: layout/style/nsStyleStruct.cpp:1119
(Diff revision 1)
> +
> +void
>  StyleShapeSource::SetBasicShape(UniquePtr<StyleBasicShape> aBasicShape,
>                                  StyleGeometryBox aReferenceBox)
>  {
> -  NS_ASSERTION(aBasicShape, "expected pointer");
> +  MOZ_ASSERT(aBasicShape, "Expect a valid pointer!");

I think we usually prefer no message for null pointer checks, since they're so common, so just "MOZ_ASSERT(aBasicShape);".

::: layout/style/nsStyleStruct.cpp:3723
(Diff revision 1)
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
> +
> +  if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
> +    const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
> +    if (shapeImage && shapeImage->GetType() == eStyleImageType_Image) {

Don't need the GetType() check, since ResolveImage() will do that.
Comment on attachment 8931282 [details]
Bug 1418224 Part 4 - Fix tests after adding shape-outside: <image> to style system.

https://reviewboard.mozilla.org/r/201214/#review207974

::: commit-message-d619d:3
(Diff revision 2)
> +Bug 1418224 Part 4 - Fix tests after adding shape-outside: <image> to style system.
> +
> +Skip URL tests in property_database.js because shape-outside will resolve

Do you mean test_transitions_per_property.html instead of property_database.js?

::: commit-message-d619d:4
(Diff revision 2)
> +URL fragments (i.e. #a), so the computed value of URL fragment will have
> +document URL as a prefix, which won't match.

You could make some URL tests that have an absolute URL instead.

::: layout/style/test/property_database.js:6450
(Diff revision 2)
> +    invalid_values: basicShapeSVGBoxValues.concat(
> +      basicShapeInvalidValues,
> +      invalidGradientAndElementValues
> +    ),

Nit: make this

  invalid_values: [].concat(
    basicShapeSVGBoxValues,
    basicShapeInvalidValues,
    invalidGradientAndElementValues
  )

to be consistent with unbalanced_values?  Or replace the [] below with basicShapeUnbalancedValues.

::: testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini:6
(Diff revision 2)
> +      if stylo: PASS
> +      FAIL

A single

  if not stylo: FAIL

should also work.
Attachment #8931282 - Flags: review?(cam) → review+
Comment on attachment 8931283 [details]
style: Support shape-image: <image>

https://reviewboard.mozilla.org/r/202376/#review207976

r=me with that.

::: servo/components/style/gecko/conversions.rs:609
(Diff revision 2)
> -    impl<'a, ReferenceBox> From<&'a StyleShapeSource> for ShapeSource<BasicShape, ReferenceBox, ComputedUrl>
> +    impl<'a> From<&'a StyleShapeSource> for ClippingShape
> -    where
> -        ReferenceBox: From<StyleGeometryBox>,
>      {

Can we factor out the body of these two From impls into a helper function that conditionally fails with URL / Image values?

::: servo/components/style/properties/gecko.mako.rs:5028
(Diff revision 2)
> +            ShapeSource::ImageOrUrl(_) => {
> +                unimplemented!("Unknown property!");
> +            }

I think you can do:

  <% raise Exception("Unknown property") %>

to make this a compile time error.
Attachment #8931283 - Flags: review?(cam) → review+
Comment on attachment 8931281 [details]
Bug 1418224 Part 3 - Add shape-outside: <image> support to style system.

https://reviewboard.mozilla.org/r/201212/#review207972

> Given the changes to nsFloatManager.cpp, where it asserts on URL types, should we be doing something else here?

Yes. We can delete `case eCSSUnit_URL` to let it fall into the `MOZ_ASSERT_UNREACHABLE` below.
Comment on attachment 8931283 [details]
style: Support shape-image: <image>

https://reviewboard.mozilla.org/r/202376/#review207976

> Can we factor out the body of these two From impls into a helper function that conditionally fails with URL / Image values?

I had thought of that, but `ShapeSource::ImageOrUrl` will be instantiated to two different types in each method, `ComputedUrl` in `ClippingShape` and `Image` in `FloatAreaShape`. 

Unless we separate `Image` and `Url` into two fields in `enum ShapeSource`, I don't see a way to write a helper function for this. Is there any Rust trick for this?

> I think you can do:
> 
>   <% raise Exception("Unknown property") %>
> 
> to make this a compile time error.

This is nice!
Comment on attachment 8931283 [details]
style: Support shape-image: <image>

https://reviewboard.mozilla.org/r/202376/#review207976

> I had thought of that, but `ShapeSource::ImageOrUrl` will be instantiated to two different types in each method, `ComputedUrl` in `ClippingShape` and `Image` in `FloatAreaShape`. 
> 
> Unless we separate `Image` and `Url` into two fields in `enum ShapeSource`, I don't see a way to write a helper function for this. Is there any Rust trick for this?

Maybe you could have a helper function that just handles the conversion for the None/Box/Shape values, and that function would be generic over the ImageOrUrl type, so e.g.:

  fn convert<BasicShape, ReferenceBox, ImageOrUrl>(source: &StyleShapeSource) -> Option<ShapeSource<BasicShape, ReferenceBox, ImageUrl>> {
    match source.mType {
      StyleShapeSource::None => ...,
      StyleShapeSource::Box => ...,
      StyleShapeSource::Shape => ...,
      _ => None,
    }
  }

And the Url/Image values you can handle separately in the two From impls.  You shouldn't need the BasicShape and ReferenceBox type arguments either, if they're the same for ClippingShape and FloatAreaShape.

Or there is probably something you could do by having a trait that both ClippingShape and FloatAreaShape implement...
Comment on attachment 8931281 [details]
Bug 1418224 Part 3 - Add shape-outside: <image> support to style system.

https://reviewboard.mozilla.org/r/201212/#review208020

r=me with those previous comments addressed.
Attachment #8931281 - Flags: review?(cam) → review+
I've added a helper function to handle values other than Url/Image values per to comment 17.  which might need a re-review.
Flags: needinfo?(cam)
Looks good, thanks!
Flags: needinfo?(cam)
Attached file Servo PR #19376 (obsolete) — 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8931283 -
        Attachment is obsolete: true

    
    

    
  
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e86ff6b95ae
Part 1 - Change StyleShapeSource::SetURL's return type to void. r=heycam
https://hg.mozilla.org/integration/autoland/rev/faad7f275749
Part 2 - Extract ShapeInfo::CreateBasicShape(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/0d58d9fed90d
Part 3 - Add shape-outside: <image> support to style system. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3ef8715cb8d7
Part 4 - Fix tests after adding shape-outside: <image> to style system. r=heycam

        Attachment #8931923 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached file
          
        Servo PR #19415
        — 
      

    


  

    
    

    
  
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d25c240574f5
Part 1 - Change StyleShapeSource::SetURL's return type to void. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3936d5282bb1
Part 2 - Extract ShapeInfo::CreateBasicShape(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/0db4809caa0a
Part 3 - Add shape-outside: <image> support to style system. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e065b6fd2c7f
Part 4 - Fix tests after adding shape-outside: <image> to style system. r=heycam

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: