Closed Bug 1387986 Opened 7 years ago Closed 7 years ago

stylo: Some distance calculation and interpolated results are not correct for stroke-dasharray in test_transitions_per_property.html

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: boris, Assigned: mantaroh)

References

Details

Attachments

(2 files)

Servo PR, #18168
41 bytes, text/x-github-pull-request
Details | Review
Bug 1387986 - Enable stroke-* test of test_transitions_per_property.html on Servo.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
There are many failures of stroke-dasharray in test_transitions_per_property.html:

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | dasharray-valued property stroke-dasharray: interpolation of dasharray - got "3", expected "6"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 3 from start '3' to quarter '6' should be quarter distance 0 from start '3' to end '15px'
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '6' to end '15px' should be three quarters distance 0 from start '3' to end '15px'
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | dasharray-valued property stroke-dasharray: interpolation of dasharray - got "8, 8px, 7, 4", expected "8, 9, 7, 7"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from start '6,8px,4,4' to quarter '8,9,7,7' should be quarter distance 0 from start '6,8px,4,4' to end '14, 12,16,16px'
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '8,9,7,7' to end '14, 12,16,16px' should be three quarters distance 0 from start '6,8px,4,4' to end '14, 12,16,16px'
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | dasharray-valued property stroke-dasharray: non-interpolability of mixed units - got "5.75, 14, 6, 10, 10.25, 5, 8.25, 14.5, 3.5, 8, 12.75, 7.75", expected "2, 50%, 6, 10"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | dasharray-valued property stroke-dasharray: interpolation of dasharray - got "5.8125, 14, 5, 8, 9.1875, 5, 6.6875, 11.375, 4.125, 8, 10.0625, 6.3125", expected "3, 45%, 5, 8"
Some of those failures will be fixed by bug 1369614. I hope all of them, but am not quite sure.
Depends on: 1369614
Mantaroh, would you mind running this test with your patch and seeing what happens there?  If your patch does not fix these failures completely, we need to investigate the reason before landing your patch.
Flags: needinfo?(mantaroh)
(In reply to Boris Chiou [:boris] from comment #0)
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | dasharray-valued property stroke-dasharray: non-interpolability of mixed
> units - got "5.75, 14, 6, 10, 10.25, 5, 8.25, 14.5, 3.5, 8, 12.75, 7.75",
> expected "2, 50%, 6, 10"
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | dasharray-valued property stroke-dasharray: interpolation of dasharray -
> got "5.8125, 14, 5, 8, 9.1875, 5, 6.6875, 11.375, 4.125, 8, 10.0625,
> 6.3125", expected "3, 45%, 5, 8"

Oops, these seem to be the same issue for shadow list interpolation.
(In reply to Boris Chiou [:boris] from comment #0)
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | dasharray-valued property stroke-dasharray: non-interpolability of mixed
> units - got "5.75, 14, 6, 10, 10.25, 5, 8.25, 14.5, 3.5, 8, 12.75, 7.75",
> expected "2, 50%, 6, 10"
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | dasharray-valued property stroke-dasharray: interpolation of dasharray -
> got "5.8125, 14, 5, 8, 9.1875, 5, 6.6875, 11.375, 4.125, 8, 10.0625,
> 6.3125", expected "3, 45%, 5, 8"

These two failures seem to have been already fixed. I don't see the failures any more.
As far as I can tell rest of failures in comment 0 should be fixed by bug 1369614.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Mantaroh, would you mind running this test with your patch and seeing what
> happens there?  If your patch does not fix these failures completely, we
> need to investigate the reason before landing your patch.

Bug 1369614 will use the ComputeSquaredDistance derive for new type. However test failures still remains.

https://pastebin.mozilla.org/9029892

39070 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 3 from start '3' to quarter '6' should be quarter distance 0 from start '3' to end '15px' 
39071 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '6' to end '15px' should be three quarters distance 0 from start '3' to end '15px' 
39075 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from start '6,8px,4,4' to quarter '8,9,7,7' should be quarter distance 0 from start '6,8px,4,4' to end '14, 12,16,16px' 
39076 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '8,9,7,7' to end '14, 12,16,16px' should be three quarters distance 0 from start '6,8px,4,4' to end '14, 12,16,16px'
Flags: needinfo?(mantaroh)
So what's your plan about the distance for stroke-dasharray?  It seems to me that all of test cases for stroke-dasharray distance don't work fine. That means your patch does not work at least for distance calculation.   Are you going to defer it to another bug? Or should we fix it regardless of your patch?
Priority: -- → P2
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> So what's your plan about the distance for stroke-dasharray?  It seems to me
> that all of test cases for stroke-dasharray distance don't work fine. That
> means your patch does not work at least for distance calculation.   Are you
> going to defer it to another bug? Or should we fix it regardless of your
> patch?

I'll take it. Bug 1369614 will land soon.
Assignee: nobody → mantaroh
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > Mantaroh, would you mind running this test with your patch and seeing what
> > happens there?  If your patch does not fix these failures completely, we
> > need to investigate the reason before landing your patch.
> 
> Bug 1369614 will use the ComputeSquaredDistance derive for new type. However
> test failures still remains.
> 
> https://pastebin.mozilla.org/9029892
> 
> 39070 INFO TEST-UNEXPECTED-FAIL |
> layout/style/test/test_transitions_per_property.html | property
> 'stroke-dasharray': distance 3 from start '3' to quarter '6' should be
> quarter distance 0 from start '3' to end '15px' 
> 39071 INFO TEST-UNEXPECTED-FAIL |
> layout/style/test/test_transitions_per_property.html | property
> 'stroke-dasharray': distance 0 from quarter '6' to end '15px' should be
> three quarters distance 0 from start '3' to end '15px' 
> 39075 INFO TEST-UNEXPECTED-FAIL |
> layout/style/test/test_transitions_per_property.html | property
> 'stroke-dasharray': distance 0 from start '6,8px,4,4' to quarter '8,9,7,7'
> should be quarter distance 0 from start '6,8px,4,4' to end '14, 12,16,16px' 
> 39076 INFO TEST-UNEXPECTED-FAIL |
> layout/style/test/test_transitions_per_property.html | property
> 'stroke-dasharray': distance 0 from quarter '8,9,7,7' to end '14,
> 12,16,16px' should be three quarters distance 0 from start '6,8px,4,4' to
> end '14, 12,16,16px'

Umm,, wait.
On my local environment on today, test has passed. I'll confirm on try server:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc43578c704c1a9a6427430108a12e3ddcec792

If test has passed, I'll remove this skip code on this bug.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> On my local environment on today, test has passed. I'll confirm on try

Sorry, this is my mistake. An environment is wrong. Test result is:

40004 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 3 from start '3' to quarter '6' should be quarter distance 0 from start '3' to end '15px'
    check_distance@layout/style/test/test_transitions_per_property.html:1111:3
    test_dasharray_transition@layout/style/test/test_transitions_per_property.html:1761:3
    @layout/style/test/test_transitions_per_property.html:1082:5
40005 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '6' to end '15px' should be three quarters distance 0 from start '3' to end '15px'
    check_distance@layout/style/test/test_transitions_per_property.html:1112:3
    test_dasharray_transition@layout/style/test/test_transitions_per_property.html:1761:3
    @layout/style/test/test_transitions_per_property.html:1082:5
40006 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from start '6,8px,4,4' to quarter '8,9,7,7' should be quarter distance 0 from start '6,8px,4,4' to end '14, 12,16,16px'
    check_distance@layout/style/test/test_transitions_per_property.html:1111:3
    test_dasharray_transition@layout/style/test/test_transitions_per_property.html:1772:3
    @layout/style/test/test_transitions_per_property.html:1082:5
40007 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property 'stroke-dasharray': distance 0 from quarter '8,9,7,7' to end '14, 12,16,16px' should be three quarters distance 0 from start '6,8px,4,4' to end '14, 12,16,16px'
    check_distance@layout/style/test/test_transitions_per_property.html:1112:3
    test_dasharray_transition@layout/style/test/test_transitions_per_property.html:1772:3
    @layout/style/test/test_transitions_per_property.html:1082:5
Buffered messages finished

We should implement ComputeSquaredDistance for SvgLengthOrPercentageOrNumber in order to calculate distance between unitless value and others. Current ComputeSquaredDistance derive will not consider unitless length. So We need to this calculation.
Bug 1369614 changed SVGLength<LengthOrPercentageOrNumber> to SvgLength<SvgLengthOrPercentageOrNumber>.
This SvgLengthOrPercentageOrNumber is enum <LengthOrPercentageType, NumberType>. So ComputeSquaredDistance dervice can't calculate between LengthOrPercentageType and NumberType.

Actually, this parameter(i.e. LengthOrpercentageType and NumberType) will be LengthOrPercentage or NonNegativeLengthOrPercentage or Number or NonNegativeNumber. So servo will need to convert to NumberOrPercentage like interpolate imlementation.

WIP: https://hg.mozilla.org/try/rev/331e2670c91bd6d7f56a719d34e7afb4da0069c0
Attached file Servo PR, #18168 — 

    
    

    
  
Mantaroh, this PR has been merged. Do we need to remove skip code from test_transitions_per_property.html?
Flags: needinfo?(mantaroh)

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #13)
> Mantaroh, this PR has been merged. Do we need to remove skip code from
> test_transitions_per_property.html?

Yes. We can remove skip code of stroke-* from this test. Test result of it is success on my environment.

Push to try:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d8c14df94bf845aa9405487e9e2919951cde5d9

I'll fix it in this bug.
Flags: needinfo?(mantaroh)
Keywords: leave-open

    
    

    
  
Comment on attachment 8899681 [details]
Bug 1387986 - Enable stroke-* test of test_transitions_per_property.html on Servo.

https://reviewboard.mozilla.org/r/170994/#review176164

        Attachment #8899681 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f5b6eedb70c9
Enable stroke-* test of test_transitions_per_property.html on Servo. r=birtles

    
    

    
  
I think it's time to resolve this bug, right?
Flags: needinfo?(mantaroh)

    
    

    
  
Yes, that's right.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(mantaroh)
Resolution: --- → FIXED

    
    

    
  
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: