Closed Bug 1325865 Opened 7 years ago Closed 7 years ago

SVG: Percentage length values in mask elements are calculated incorrectly

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: paul.lebeau, Assigned: u459114)

References

Details

(Keywords: testcase)

Attachments

(5 files)

test case
323 bytes, image/svg+xml
Details
Bug 1325865 - Part 1. Remove double transform.
59 bytes, text/x-review-board-request
longsonr
: review+
Details
Bug 1325865 - Part 2. Reftest.
59 bytes, text/x-review-board-request
longsonr
: review+
Details
patch for aurora(1)
2.94 KB, patch
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review
patch for aurora(2)
2.35 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

Try the following test SVG - fiddle here: https://jsfiddle.net/ypw9soxL/

<svg width="100%">
  <defs>
    <mask id="myMask">
      <svg x="50%" overflow="visible" id="foo">
        <polygon fill="green" points="0 38 56 0 -56 0"/>
      </svg>
    </mask>
  </defs>

  <svg x="50%" overflow="visible">
    <polygon fill="black" points="0 38 56 0 -56 0" />
  </svg>
  <use xlink:href="#foo"/>

  <rect mask="url(#myMask)" width="100%" height="100%" fill="red"/>
</svg>



Actual results:

The x="50%" in the embedded SVG in the mask seems to be calculated incorrectly.  I am expecting the triangle in the mask to coincide with the the other references. However it is coming out at a value equivalent to 100%.




Expected results:

In Chrome and IE, the position of the three triangles coincide.
Component: Untriaged → SVG
Keywords: testcase
Product: Firefox → Core
I imagine bug 1127507 has changed things here. That's what the regression range found.
Assignee: nobody → cku
Attached image test case — 

    
    

    
  
https://hg.mozilla.org/mozilla-central/file/9953c71eb9ba/layout/svg/nsSVGContainerFrame.cpp#l272

This line increase x-translation, so the x-position of red rectangle looks not right.

    
    

    
  
(In reply to C.J. Ku[:cjku](UTC+8) from comment #4)
> https://hg.mozilla.org/mozilla-central/file/9953c71eb9ba/layout/svg/
> nsSVGContainerFrame.cpp#l272
> 
> This line increase x-translation, so the x-position of red rectangle looks
> not right.
or https://hg.mozilla.org/mozilla-central/file/7ecf2330d1d8/layout/svg/nsSVGMaskFrame.cpp#l265
We apply the transform of this inner svg frame twice.

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8826551 -
        Flags: review?(longsonr)

        Attachment #8826552 -
        Flags: review?(longsonr)

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8826551 -
        Flags: review?(longsonr)

        Attachment #8826552 -
        Flags: review?(longsonr)

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8826551 -
        Flags: review?(longsonr)

        Attachment #8826552 -
        Flags: review?(longsonr)

    
  
See Also:  → 537623

    
    

    
  
Comment on attachment 8826552 [details]
Bug 1325865 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/104512/#review105356

::: layout/reftests/svg/mask-contains-inner-svg-01-ref.svg:1
(Diff revision 4)
> +<?xml version="1.0"?>

usually we try to generate something which is all lime and then compare to that fixed reference. It seems to me that ought to be possible here.

    
    

    
  
Comment on attachment 8826551 [details]
Bug 1325865 - Part 1. Remove double transform.

https://reviewboard.mozilla.org/r/104510/#review105376

I'm really not convinced this is right. Surely we should modify nsSVGMaskFrame::GetMaskForMaskedFrame to apply eUserSpaceToParent only instead, no? jwatt could confirm.

    
    

    
  
Jonathan, thoughts on the patch and comment 15?
Flags: needinfo?(jwatt)

    
    

    
  
Comment on attachment 8826551 [details]
Bug 1325865 - Part 1. Remove double transform.

https://reviewboard.mozilla.org/r/104510/#review105376

How about the second version of this patch?

    
    

    
  
Maybe my reviewboad skills are failing but that looks just like the first version.

    
    

    
  
(In reply to Robert Longson from comment #18)
> Maybe my reviewboad skills are failing but that looks just like the first
> version.

Yes, but is different with the last version.
https://reviewboard.mozilla.org/r/104510/diff/2-4/

In the first or second version, I modified nsSVGMaskFrame::GetMaskForMaskedFrame to apply eUserSpaceToParent only, like you said.

I thought we can either 
1. Apply eAllTransforms from the container side(#4 version), or
2. Apply eUserSpaceToParent  from container frames and apply eUserSpaceToParent at leaf frame(a frame contains no child)(#1 and #2 version).

    
    

    
  
(In reply to C.J. Ku[:cjku](UTC+8) from comment #19)
> (In reply to Robert Longson from comment #18)
> > Maybe my reviewboad skills are failing but that looks just like the first
> > version.
> 
> Yes, but is different with the last version.
> https://reviewboard.mozilla.org/r/104510/diff/2-4/
> 
> In the first or second version, I modified
> nsSVGMaskFrame::GetMaskForMaskedFrame to apply eUserSpaceToParent only, like
> you said.
> 
> I thought we can either 
> 1. Apply eAllTransforms from the container side(#4 version), or
> 2. Apply eUserSpaceToParent  from container frames and apply
> eUserSpaceToParent at leaf frame(a frame contains no child)(#1 and #2
> version).

Hmm..no, I think you are right. If we have a container frame(<svg> or <g>) contains a another container frame, #1 is still broken. I will modify the patch and add more test case.

One last question, I do not quite understand commnet #14. Does that means I should change fill color from blue to lime? (I don't think so). Pls enlighten me(such as an example), thanks

    
    

    
  
The idea is that if we can, we make reftests that produce a completely lime result if they pass and display some (or all) red if not. We then compare them to pass.svg. 

If that turns out too difficult we can stick with the reftest you've already created.

    
    

    
  
Comment on attachment 8826551 [details]
Bug 1325865 - Part 1. Remove double transform.

https://reviewboard.mozilla.org/r/104510/#review105436

nSVGContainerFrame should stay as it is. The real fix is elsewhere.

        Attachment #8826551 -
        Flags: review?(longsonr) → review-

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8826552 [details]
Bug 1325865 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/104512/#review105444

        Attachment #8826552 -
        Flags: review?(longsonr) → review+

    
    

    
  
Comment on attachment 8826551 [details]
Bug 1325865 - Part 1. Remove double transform.

https://reviewboard.mozilla.org/r/104510/#review105446

        Attachment #8826551 -
        Flags: review?(longsonr) → review+

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c24e94982ff
Part 1. Remove double transform. r=longsonr+218550
https://hg.mozilla.org/integration/autoland/rev/14d1bd983274
Part 2. Reftest. r=longsonr+218550

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8c24e94982ff
https://hg.mozilla.org/mozilla-central/rev/14d1bd983274
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

    
    

    
  
Please request Aurora approval on this when you get a chance.

          status-firefox52:
          --- → affected
Flags: needinfo?(cku)
Flags: in-testsuite+

    
    

    
  


  
Flags: needinfo?(cku)

    
    

    
  
keep ni
Flags: needinfo?(cku)

    
    

    
  


  
Flags: needinfo?(cku)

    
  

        Attachment #8827384 -
        Attachment description: patch for aurora → patch for aurora(1)

    
    

    
  
Comment on attachment 8827384 [details] [diff] [review]
patch for aurora(1)

Approval Request Comment
[Feature/Bug causing the regression]:bug 1127507
[User impact if declined]:The position of svg mask will be wrong if this mask contains any containers element which have non zero x/y attribute.   
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:had verify the rendering result by try and manual testing.
[String changes made/needed]:nan

        Attachment #8827384 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8827384 [details] [diff] [review]
patch for aurora(1)

svg transform fix for aurora52

        Attachment #8827384 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
The documentation for nsISVGChildFrame::PaintSVG says that aTransform is the transform to the element's user space. nsSVGInnerSVGFrame::PaintSVG clearly requires that to be the case since aTransform is the transform that is applied directly before applying x,y,w,h clipping. So the change to nsSVGMaskFrame.cpp seems absolutely correct to me.

The change to SVGGeometryFrame seems completely bogus though. Elements that get an SVGGeometryFrame will always return true when IsSVGElement() is called on them. More to the point though, these elements will always return the identity matrix when PrependLocalTransformsTo is called passing eChildToUserSpace since things like 'path' etc. do not have children-only transforms like a viewBox.
Flags: needinfo?(jwatt)

    
    

    
  
(In reply to Jonathan Watt [:jwatt] from comment #37)
> The change to SVGGeometryFrame seems completely bogus though.

Yeah, I can confirm that the reftests still pass with that change removed. I'll file a bug and stick my patch on it.
Depends on: 1335610

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: