Closed Bug 1265343 Opened 4 years ago Closed 2 years ago

stylo: Implement 'shape-image-threshold'

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: sebo, Assigned: TYLin)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, DevAdvocacy)

Attachments

(3 files, 1 obsolete file)

The shape-image-threshold property should be implemented as defined in https://drafts.csswg.org/css-shapes/#shape-margin-property.

Sebastian
Depends on: shape-outside
Keywords: dev-doc-needed
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Flags: platform-rel?
platform-rel: --- → ?
platform-rel: ? → ---
Depends on: 435426
Priority: -- → P3
Whiteboard: [DevRel:P1]
I notice this was marked as depending on bug 435426.  I suppose this dependency makes some sense given the example in https://drafts.csswg.org/css-shapes/#shapes-from-image -- however, if other browsers have shipped shape-image-threshold without shipping 'attr(src url)', I don't think we should wait either.  (On the other hand, if other browsers haven't, but have instead shipped shape-outside without shipping shape-image-threshold, then this shouldn't block shipping shape-outside.)
I tested Chrome and Opera 47 right now and both support shape-image-threshold without supporting 'attr(src url)'. According to MDN[1] shape-image-threshold is also supported by Safari, though none of the browsers currently supports 'attr()' on other properties than 'content', and also only the one-value syntax[2].

Disregarding the notes above, I believe Mozilla's decision should not depend on the decision of other browsers in this case. I think this property is already useful without the support of 'attr(src url)' for shape-outside. AND, Astley already removed the dependency from shape-outside in bug 1098939 comment 6, so it's just logical to remove it here, too.

Having said that, it would still be great if 'attr(src url)' (for other properties than 'content') would be pushed, as it opens up a lot of ways for interaction between HTML and CSS.

Sebastian

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/shape-image-threshold
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/attr
No longer depends on: 435426
Assignee: nobody → tlin
Status: NEW → ASSIGNED
No longer depends on: shape-outside
Blocks: 1404222
Add 'stylo' keyword to the bug title. Because 'shape-image-threshold' property itself doesn't render anything, it's considered done after adding support to the style system. The rendering part of shape-outside: <image> will be done in bug 1404222.
Summary: Implement 'shape-image-threshold' → stylo: Implement 'shape-image-threshold'
Comment on attachment 8932687 [details]
Bug 1265343 Part 2 - Fix tests after adding 'shape-image-threshold' to style system.

https://reviewboard.mozilla.org/r/203554/#review209226

::: testing/web-platform/meta/css/css-shapes/shape-outside/values/shape-image-threshold-003.html.ini:3
(Diff revision 1)
>    [shape-image-threshold is not inherited and defaults to 0]
>      expected: FAIL

I thought this one should pass, but I got `assert_equals: expected (object) null but got (string) ""`.

Shouldn `getPropertyValue()` return `null` if it not set?

https://treeherder.mozilla.org/#/jobs?repo=try&author=tlin@mozilla.com&selectedJob=148196932
Comment on attachment 8932686 [details]
Bug 1265343 Part 1 - Add shape-image-threshold to style system.

https://reviewboard.mozilla.org/r/203552/#review209664

::: layout/style/nsStyleStruct.cpp:3792
(Diff revision 1)
>      if (aNewData.mFloat != StyleFloat::None) {
>        // If we are floating, and our shape-outside property changes, our
>        // descendants are not impacted, but our ancestor and siblings are.

This and the other comment below should be updated to mention shape-image-threshold too.

::: layout/style/nsStyleStruct.cpp:3806
(Diff revision 1)
>        // shape-outside changed, but we don't need to reflow because we're not
>        // floating.

Update this comment to mention shape-image-threadhsold too
Attachment #8932686 - Flags: review?(cam) → review+
Comment on attachment 8932688 [details]
style: Support 'shape-image-threshold' property

https://reviewboard.mozilla.org/r/203556/#review209668

::: servo/components/style/properties/gecko.mako.rs:3102
(Diff revision 1)
> -                          shape-outside contain touch-action""" %>
> +                          shape-image-threshold shape-outside
> +                          contain touch-action""" %>

I think we can remove all of these changes to gecko.mako.rs, since the property is defined with the Opacity type, and the glue functions will get generated automatically for that type.  (At least I think it should -- that's how it works for the 'opacity' property.)
Attachment #8932688 - Flags: review?(cam) → review+
Comment on attachment 8932687 [details]
Bug 1265343 Part 2 - Fix tests after adding 'shape-image-threshold' to style system.

https://reviewboard.mozilla.org/r/203554/#review209672
Attachment #8932687 - Flags: review?(cam) → review+
Comment on attachment 8932688 [details]
style: Support 'shape-image-threshold' property

https://reviewboard.mozilla.org/r/203556/#review209668

> I think we can remove all of these changes to gecko.mako.rs, since the property is defined with the Opacity type, and the glue functions will get generated automatically for that type.  (At least I think it should -- that's how it works for the 'opacity' property.)

Neat! All the changes to gecko.mako.rs are not needed.
Attached file Servo PR #19436
Attachment #8932688 - Attachment is obsolete: true
Comment on attachment 8932687 [details]
Bug 1265343 Part 2 - Fix tests after adding 'shape-image-threshold' to style system.

https://reviewboard.mozilla.org/r/203554/#review209226

> I thought this one should pass, but I got `assert_equals: expected (object) null but got (string) ""`.
> 
> Shouldn `getPropertyValue()` return `null` if it not set?
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&author=tlin@mozilla.com&selectedJob=148196932

This test is fixed by upstream. https://github.com/w3c/web-platform-tests/pull/8517
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/929e3111144c
Part 1 - Add shape-image-threshold to style system. r=heycam
https://hg.mozilla.org/integration/autoland/rev/853a847067ad
Part 2 - Fix tests after adding 'shape-image-threshold' to style system. r=heycam
https://hg.mozilla.org/mozilla-central/rev/929e3111144c
https://hg.mozilla.org/mozilla-central/rev/853a847067ad
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I was going to write a page about this, but then I found out that it had already been done:

https://developer.mozilla.org/en-US/docs/Web/CSS/shape-image-threshold

So I've just added a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/59#CSS

Let me know if that is OK. Thanks!
Flags: needinfo?(aethanyc)
We shouldn't add this to the 59 release note because this property is still behind the pref "layout.css.shape-outside.enabled".
Flags: needinfo?(aethanyc) → needinfo?(cmills)
Hi Chris, 
Yeah, there are many parts and pieces for CSS Shapes level 1, including: shape-outside, shape-margin, shape-image-threshold (just off the top of my head). They will all ship together, not one at a time, since they are needed together. Work is still underway. There was a hope that perhaps CSS Shapes would make it for Firefox 60, but it definitely won't happen for 59. And it might not make 60 — instead sliding to 61 or whenever it's ready. It would be great for all the CSS Shapes docs to be all shiny and happy. Also — for the docs for the Shape Path Editor to be ready. We'll be shipping the Shape Path Editor at the same time.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) (Away, needinfo me if you need anything) from comment #21)
> We shouldn't add this to the 59 release note because this property is still
> behind the pref "layout.css.shape-outside.enabled".

OK, thanks. I've removed the note from the rel notes, and:

* Added an entry in our experimental features page: https://developer.mozilla.org/en-US/Firefox/Experimental_features
* Updated the browser support info to include the flag information: https://github.com/mdn/browser-compat-data/pull/899
Flags: needinfo?(cmills)
(In reply to Jen Simmons [:jensimmons] from comment #22)
> Hi Chris, 
> Yeah, there are many parts and pieces for CSS Shapes level 1, including:
> shape-outside, shape-margin, shape-image-threshold (just off the top of my
> head). They will all ship together, not one at a time, since they are needed
> together. Work is still underway. There was a hope that perhaps CSS Shapes
> would make it for Firefox 60, but it definitely won't happen for 59. And it
> might not make 60 — instead sliding to 61 or whenever it's ready. It would
> be great for all the CSS Shapes docs to be all shiny and happy. Also — for
> the docs for the Shape Path Editor to be ready. We'll be shipping the Shape
> Path Editor at the same time.

Cool, thanks for all the info Jen; I've contacted you over mail for followup.
You need to log in before you can comment on or make changes to this bug.