Closed Bug 1347409 Opened 3 years ago Closed 3 years ago

SVG paint server without fallback value should be treated as no fallback

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [stylo])

Attachments

(3 files, 2 obsolete files)

Currently, if you do
> <p style="fill: url(#a)"></p>
the computed value of fill would be "url(#a) rgba(0, 0, 0, 0)", however, in this case, it should actually be "url(#a) none" or just "url(#a)".

This is because nsStyleSVGPaint always stores a color for the fallback, and SetSVGPaint (in nsRuleNode.cpp) does the computation.

I think a reasonable solution would be to split eStyleSVGPaintType_Server value into eStyleSVGPaintType_Server and eStyleSVGPaintType_ServerWithoutFallback, and use the latter for the cases where fallback is none.

test_bug_1293164 in layout/style/test/test_computed_style.html can be simplified after fixing this.

(Servo side would also need similiar fix.)
Priority: -- → P4
That's not really going to work as context-fill and context-stroke can have optional fallback. On top off that we don't really distinguish between no fallback and the none keyword. I think we need a new enum (nothing, none, color) to track what kind of fallback we have.
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attached patch 1347409.txtSplinter Review
Allows gecko to distinguish between

url()
url() none
url() blue

We serialise what we are given now. Previously there was no way to distinguish the first two cases, in fact they both became url() rgba(0, 0, 0, 0)

Currently url() and url() none are treated identically but bug 1351243 suggests that context-fill without a fallback colour should be treated as if it were context-fill black. This patch would allow that to be supported fairly simply.
Attachment #8858821 - Attachment is obsolete: true
Attachment #8859028 - Flags: review?(cam)
Attached patch attempt at servo patch (obsolete) — Splinter Review
Not really sure if this is

a) how to support url() none (is that supported currently in stylo?)
b) sufficient except for the known limitation of a)
c) if it compiles given that the gecko patch with the enum in has to land. Can I tie my gecko and server trees together locally somehow?
d) how to land it after the gecko patch lands.

Without what I've done so far stylo's url() color support would disappear though.
Attachment #8859030 - Flags: feedback?(cam)
Comment on attachment 8859028 [details] [diff] [review]
1347409.txt

Review of attachment 8859028 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following addressed (and assuming you use that comment as the commit message).

::: layout/style/nsComputedDOMStyle.cpp
@@ +5740,5 @@
> +        }
> +        valueList->AppendCSSValue(val.forget());
> +        valueList->AppendCSSValue(fallback.forget());
> +        return valueList.forget();
> +      }      

Nit: trailing space.

@@ +5759,5 @@
> +        }
> +        valueList->AppendCSSValue(val.forget());
> +        valueList->AppendCSSValue(fallback.forget());
> +        return valueList.forget();
> +      }      

Nit: trailing space.

It might be nice to factor out the serialization of the fallback into a separate function, since the code is the same here as for URLs above.

::: layout/style/nsStyleStruct.h
@@ +3370,4 @@
>    eStyleSVGPaintType_ContextStroke
>  };
>  
> +enum nsStyleSVGFallbackType {

Maybe we should make this (and nsStyleSVGPaint) |: uint8_t|, just to avoid wasting a bit of space in nsStyleSVGPaint.

::: layout/svg/nsSVGUtils.cpp
@@ +1458,5 @@
>    const nsStyleSVGPaint &paint = aStyleContext->StyleSVG()->*aFillOrStroke;
>    nsStyleContext *styleIfVisited = aStyleContext->GetStyleIfVisited();
> +  nscolor color;
> +  switch (paint.Type()) {
> +  case eStyleSVGPaintType_Server:

Nit: cases should be indented one level.

@@ +1555,5 @@
>    }
>  
> +  if (style->mFill.GetFallbackType() == eStyleSVGFallbackType_None) {
> +    return DrawResult::SUCCESS;
> +  }

Nit: I'd put a blank line below this.

@@ +1634,5 @@
>    }
>  
> +  if (style->mStroke.GetFallbackType() == eStyleSVGFallbackType_None) {
> +    return DrawResult::SUCCESS;
> +  }

Nit: and here.
Attachment #8859028 - Flags: review?(cam) → review+
Comment on attachment 8859030 [details] [diff] [review]
attempt at servo patch

Review of attachment 8859030 [details] [diff] [review]:
-----------------------------------------------------------------

::: components/style/properties/gecko.mako.rs
@@ +448,4 @@
>          }
>  
>          if let Some(fallback) = fallback {
> +            paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_Color;

I think instead of this, we should extend the specified and computed SVGPaint types in Servo (in components/style/values/{specified,computed}/mod.rs) so that the fallback is an enum type that allows distinguishing none from the presence of a colour.  Then we can set mFallbackType to eStyleSVGFallbackType_None when appropriate.

(We could extend Gecko_nsStyleSVGPaint_SetURLValue with arguments to indicate the fallback type/value, but given we're poking in the nsStyleSVGPaint object directly here anyway, for the non-URL types, we may as well do all that poking here.)
Attachment #8859030 - Flags: feedback?(cam)
(In reply to Robert Longson from comment #4)
> a) how to support url() none (is that supported currently in stylo?)

"No", per my comment just now, but we should add support for that.

> b) sufficient except for the known limitation of a)

I think so.

> c) if it compiles given that the gecko patch with the enum in has to land.
> Can I tie my gecko and server trees together locally somehow?

If you make your local changes to the files under servo/ in your mozilla-central checkout, and you have |ac_add_options --enable-stylo| in your mozconfig, it should all compile together.  (I usually do that, and use my separate Servo checkout when it's time to put those patches up in a PR.)

> d) how to land it after the gecko patch lands.

The current procedure for changes that involve Gecko and Servo are to do all the review here in Bugzilla, then when you've got your r+es, open a Servo PR with just the servo/ changes (re-jigged to not apply to files under servo/!), mention in the PR that it's already been reviewed by whoever and link to the Bugzilla bug.  Someone will say "@bors-servo delegate+" in the PR, which means you then have control over when the patch will be tested and merged (you type "@bors-servo r=whoever" in the PR).  After the hour or so of CI testing, it should merge if there are no Servo test failures, and then you can land the Gecko change ASAP.  Unfortunately we don't have anything better at the moment, so if you don't think you'll be around to click the button to land the Gecko patch in an hour after r+ing the Servo PR, better to wait until you will have time.  Or you could ask someone on #servo to help you do that.

> Without what I've done so far stylo's url() color support would disappear
> though.

Yeah, I think I noticed some test failures just earlier today due to not supporting the fallback properly.  (It turns out we don't enable stylo for SVG documents at the moment -- see bug 1355762.)
Also, when doing a try run for changes that affect stylo, please include "-p linux64-stylo" in your try syntax.  That might reveal some test passes (maybe unlikely until bug 1355762 is fixed), but if there are, you should update the annotations for those in the Gecko patch.
Blocks: 1352258
Keywords: leave-open
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff1f94e5767
part 1 - introduce nsStyleSVGFallbackType so that we can begin to distinguish the type of paintserver fallback r=cam
Comment on attachment 8859030 [details] [diff] [review]
attempt at servo patch

I think I'd like to land this as-is and then implement comment 7 in a follow-up bug so as not to block progress on bug 1352258.

I've landed just enough of this bug so that the servo patch should be landable now.
Attachment #8859030 - Flags: review?(cam)
Comment on attachment 8859030 [details] [diff] [review]
attempt at servo patch

Review of attachment 8859030 [details] [diff] [review]:
-----------------------------------------------------------------

Doing that in the followup sounds fine.  Please do file it though.
Attachment #8859030 - Flags: review?(cam) → review+
fix servo patch so that it compiles. Some kind of include was missing.
Attachment #8859030 - Attachment is obsolete: true
Blocks: 1360935
Attachment #8863235 - Flags: checkin+
Keywords: leave-open
OS: Unspecified → All
Hardware: Unspecified → All
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08735c334ad1
part 2 - serialise fill and stroke fallback properly r=cam
https://hg.mozilla.org/mozilla-central/rev/08735c334ad1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.