Closed Bug 1367327 Opened 6 years ago Closed 6 years ago

stylo: SVG unitless length should not be rounded to absolute length during parsing and computing

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1367327 - stylo: re-enable SVG stroke-width related tetst.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
This bug is filed due to the investigation of the disabled test, layout/reftests/svg/stroke-dasharray-02.svg.

It appears that the only failure in the test is caused by the large scaling in [1]. To be clear, the phenomenon is that a <circle> with large scaling transform is not rendered at all, so the red circles (rendered by [2]) are not covered with lime, which cause the test failure.

Here are a pair of simplified examples:

ex1:
<svg xmlns="http://www.w3.org/2000/svg">
  <rect width="100%" height="100%" fill="lime"/>
    <circle cx="0" cy="0" r=".0008" fill="none" stroke="red" stroke-width=".0001" transform="translate(180, 300) scale(1e5, 1e5)"/>
</svg>


ex2:
<svg xmlns="http://www.w3.org/2000/svg">
  <rect width="100%" height="100%" fill="lime"/>
    <circle cx="200" cy="30" r="30" fill="none" stroke="brown" stroke-width="10" transform="translate(200, 300) scale(1e0, 1e0)"/>
</svg>


In stylo, ex1 can't be rendered at all, whereas ex2 can.
In gecko, both examples can be rendered properly.


[1] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/reftests/svg/stroke-dasharray-02.svg#39-40
[2] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/reftests/svg/stroke-dasharray-02.svg#37-38
Second thoughts, this might be related to precision issues of stroke-width, cause I tried to get the computed value of stroke-width for ex1 in stylo, and I got 0 instead of 0.0001.
Hmmm, this is definitely a parsing/computing issue of stroke-width property. Or, more like a issue for unitless length.

In gecko, we keep the unitless length as it is, and compute/store it as a pure factor number in mStrokeWidth [1]. However, in stylo, we do some rounding stuff (round to absolute length) during parsing [2]. Maybe we should match stylo to gecko by adding the ability for length value to parse unitless number and compute/store as a factor value. Fix the bug summary then.


[1] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/style/nsRuleNode.cpp#962
[2] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/servo/components/style/values/specified/length.rs#622-629
Summary: stylo: SVG elements with large scaling transform is not rendered at all → stylo: unitless length should not be rounded to absolute length during parsing/computing
Assignee: nobody → jeremychen
Priority: -- → P2
Depends on: 1359343
There're 3 SVG properties that accept SVG unitless length, stroke-width, stroke-dasharray, and stroke-dashoffset [1]. In stylo, we compute and store SVG unitless length as a factor number for stroke-dasharray, but not for stroke-width and stroke-dashoffset. So, we shoud fix it.

I wonder maybe we should add some more tests for them. Filed Bug 1367977.


[1] http://searchfox.org/mozilla-central/search?q=symbol:M_8b66591b5a234643ab62b847a70746707a32d04b&redirect=false
Summary: stylo: unitless length should not be rounded to absolute length during parsing/computing → stylo: SVG unitless length should not be rounded to absolute length during parsing and computing
The test is passed on my local, so ask for review while waiting for a positive try result.
Attachment #8871220 - Flags: review?(cam)
Attachment #8871598 - Flags: review?(cam)
Comment on attachment 8871220 [details]
Bug 1367327 - stylo: compute and store SVG unitless length as a factor number.

https://reviewboard.mozilla.org/r/142718/#review146816

::: commit-message-2c8b6:3
(Diff revision 2)
> +Bug 1367327 - stylo: compute and store SVG unitless length as a factor number.
> +
> +There 3 SVG properties that accept SVG unitless length, stroke-width, stroke-dasharray,

There are

::: commit-message-2c8b6:5
(Diff revision 2)
> +Bug 1367327 - stylo: compute and store SVG unitless length as a factor number.
> +
> +There 3 SVG properties that accept SVG unitless length, stroke-width, stroke-dasharray,
> +and stroke-dashoffset. We compute and store SVG unitless length as a factor number for
> +stroke-dasharray, but not for stroke-width and stroke-dashoffset. So, let's fix it.

I don't think we need "So, let's fix it." in the commit message. :-)
Attachment #8871220 - Flags: review?(cam) → review+
Comment on attachment 8871598 [details]
Bug 1367327 - stylo: re-enable SVG stroke-width related tetst.

https://reviewboard.mozilla.org/r/143068/#review146818
Attachment #8871598 - Flags: review?(cam) → review+
Comment on attachment 8871220 [details]
Bug 1367327 - stylo: compute and store SVG unitless length as a factor number.

https://reviewboard.mozilla.org/r/142718/#review146816

> I don't think we need "So, let's fix it." in the commit message. :-)

Yes, you're right. Thanks for the review. :)
Comments addressed!!

Servo PR: https://github.com/servo/servo/pull/17053
Servo side merged: https://hg.mozilla.org/integration/autoland/rev/2e6408a2b2c7
Let's enable the test!!!
Attachment #8871220 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ddd503c088e0 -d 2e6408a2b2c7: rebasing 398426:ddd503c088e0 "Bug 1367327 - stylo: re-enable SVG stroke-width related tetst. r=heycam" (tip)
merging layout/reftests/svg/reftest.list
warning: conflicts while merging layout/reftests/svg/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f7aef97e7b7
stylo: re-enable SVG stroke-width related tetst. r=heycam
https://hg.mozilla.org/mozilla-central/rev/5f7aef97e7b7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1369614
You need to log in before you can comment on or make changes to this bug.