Closed Bug 372063 Opened 17 years ago Closed 16 years ago

clip: rect(auto auto auto auto); is not equivalent to clip: auto

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: sharparrow1, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files, 7 obsolete files)

Testcase #2
2.25 KB, text/html
Details
patch
17.86 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Attached file Testcase (obsolete) — 
Testcase attached.  The spec states very clearly that auto values correspond to the border box; therefore, the red box should get clipped.  However, the clip rect is getting ignored.  The DOM Inspector shows that the computed value is auto, which is wrong.
Hmm, looking at the code a bit, this is clearly a style system problem; mClipFlags isn't getting set to the right value.
Assignee: nobody → dbaron
Component: Layout: R & A Pos → Style System (CSS)
QA Contact: layout.r-and-a-pos → ian
Attached file Testcase #2 — 
You're right about the bug, but you're testcase is wrong, it would show
FAIL also when the bug is fixed.
Attachment #256746 - Attachment is obsolete: true
Patch coming up...
Assignee: dbaron → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #3)
> Patch coming up...

What exactly did you mean by "coming up"?
(In reply to comment #4)
> What exactly did you mean by "coming up"?

I meant: I have a fix in my tree and will attach a patch for review soon.
It took longer than I anticipated. Sorry.
Attached patch Patch rev. 1 (obsolete) — — Splinter Review 
We currently use 'inherit' style rect coordinates internally to encode
'clip:inherit'.  I extended this idea by using 'top:auto' + 'left:inherit'
to encode 'clip:auto'.
Attachment #258822 - Flags: superreview?(dbaron)
Attachment #258822 - Flags: review?(dbaron)
Attached patch Reftest rev. 1 (obsolete) — — Splinter Review 

        Attachment #258834 -
        Flags: superreview?(dbaron)

        Attachment #258834 -
        Flags: review?(dbaron)

    
    

    
  
(In reply to comment #5)
> (In reply to comment #4)
> > What exactly did you mean by "coming up"?
> 
> I meant: I have a fix in my tree and will attach a patch for review soon.
> It took longer than I anticipated. Sorry.

It's not a problem; I was just making sure you didn't forget to post the patch or something like that.

    
    

    
  
Comment on attachment 258822 [details] [diff] [review]
Patch rev. 1

This representation solution seems too hacky.  Instead, could you use eCSSUnit_Null on all 4 sides to represent 'auto', and eCSSUnit_Auto on a side to represent that that side was auto?

Could you also remove the relevant line from gBadComputed in layout/style/test/test_value_computation.html and make sure all the mochitests in layout/style/test pass?  (To do that, cd <objdir>/_tests/testing/mochitest; perl ./runtests.pl; click on the directory name "layout/style/test" in the list so you just show those tests, and then click "Run Tests" at the top.)

        Attachment #258822 -
        Flags: superreview?(dbaron)

        Attachment #258822 -
        Flags: superreview-

        Attachment #258822 -
        Flags: review?(dbaron)

        Attachment #258822 -
        Flags: review-

    
    

    
  
Comment on attachment 258834 [details] [diff] [review]
Reftest rev. 1

r+sr=dbaron, although it would be good to add an equivalent to test4 with an explicit clip:auto that tests that it's not clipped, and another variant with no clip at all, also testing that it's not clipped.  (PASS and FAIL would need to be switched in the test, of course.)

        Attachment #258834 -
        Flags: superreview?(dbaron)

        Attachment #258834 -
        Flags: superreview+

        Attachment #258834 -
        Flags: review?(dbaron)

        Attachment #258834 -
        Flags: review+

    
    

    
  
(In reply to comment #9)
> (From update of attachment 258822 [details] [diff] [review])
> This representation solution seems too hacky.  Instead, could you use
> eCSSUnit_Null on all 4 sides to represent 'auto'

A rect with all sides set to eCSSUnit_Null represents "not specified".
Using it for clip:auto also results in these assertions:

###!!! ASSERTION: oops: 'mTempData.HasPropertyBit(aPropID)', file nsCSSParser.cpp, line 3437
###!!! ASSERTION: Valueless rect while computing size: 'val->HasValue()', file nsCSSDataBlock.cpp, line 652
###!!! ASSERTION: Valueless rect while compressing: 'val->HasValue()', file nsCSSDataBlock.cpp, line 753

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 2 (obsolete)
        — 
          — Splinter Review
      

    


  
I chose eCSSUnit_None on all sides to encode clip:auto.
This patch also includes the reftest, with the additional
two tests you wanted.

        Attachment #258822 -
        Attachment is obsolete: true

        Attachment #258834 -
        Attachment is obsolete: true

        Attachment #263910 -
        Flags: superreview?(dbaron)

        Attachment #263910 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 263910 [details] [diff] [review]
Patch rev. 2

There a couple of -moz-image-region mochitests that fails with this...

        Attachment #263910 -
        Flags: superreview?(dbaron)

        Attachment #263910 -
        Flags: review?(dbaron)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 3 (obsolete)
        — 
          — Splinter Review
      

    


  
Contains fix + mochitest corrections + reftest.
It passes the "layout/style/test" mochitests.
As a bonus -moz-image-region now passes the 'initial' tests as well.

        Attachment #263910 -
        Attachment is obsolete: true

        Attachment #263956 -
        Flags: superreview?(dbaron)

        Attachment #263956 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 263956 [details] [diff] [review]
Patch rev. 3

>+  if (eCSSUnit_Null    == displayData.mClip.mTop.GetUnit() ||

You don't want this null test; it'll break clip specified in a less specific rule in some cases.

>+  if (eCSSUnit_Null == listData.mImageRegion.mTop.GetUnit()) {
>+    // Nothing to do - optimization to have this first.
>+  }

Don't do this -- if we wanted this as an optimization we should do it everywhere and measure the costs and benefits -- so it should be a different bug.

I still need to look at nsRuleNode changes for clip.

Should at least get a new bug filed on auto values within rect() for -moz-image-region.

    
    

    
  
(In reply to comment #15)
> (From update of attachment 263956 [details] [diff] [review])
> >+  if (eCSSUnit_Null    == displayData.mClip.mTop.GetUnit() ||
> 
> You don't want this null test; it'll break clip specified in a less specific
> rule in some cases.

And what you were trying to solve is already solved by nsStyleDisplay's constructor (and copy-constructor).

I should probably figure out how to make mochitests to catch mistakes like that...

    
    

    
  
(In reply to comment #15)
> You don't want this null test; it'll break clip specified in a less specific
> rule in some cases.

To whoever writes them, don't forget to include at least one of these in the final reftests.  :-)

    
    

    
  
Comment on attachment 263956 [details] [diff] [review]
Patch rev. 3

>-    display->mClipFlags &= ~NS_STYLE_CLIP_TYPE_MASK;

You can't really remove this, since if there's a start struct (because you've already gotten style and cached it in the rule tree for a less specific rule), you might have clip flags set.

>-    if (fullAuto) {
>-      display->mClipFlags |= NS_STYLE_CLIP_AUTO;
>-    }
>-    else {
>-      display->mClipFlags |= NS_STYLE_CLIP_RECT;
>     }
>+    display->mClipFlags |= NS_STYLE_CLIP_RECT;

But the better solution than not removing it would be to change this line to an assignment (= rather than |=) and move it up to the beginning of the block that it's in.  Doing this will also fix some bugs when you currently have rect(10px,auto,30px,10px) in a less specific rule and rect(10px,30px,30px,10px) in a more specific rule -- we could leave NS_STYLE_CLIP_RIGHT_AUTO in mClipFlags.

I think there are enough changes here that I'd like to look at a new patch, but this mostly looks good.  (And, you're right, eCSSUnit_Null was a bad suggestion -- I was thinking of nsStyleCoord (and nsStyleUnit_Null) rather than nsCSSValue!)

It would probably be good to have testcases for these potential mistakes -- and perhaps even testcases to catch potential similar mistakes on other properties...

        Attachment #263956 -
        Flags: superreview?(dbaron)

        Attachment #263956 -
        Flags: superreview-

        Attachment #263956 -
        Flags: review?(dbaron)

        Attachment #263956 -
        Flags: review-
QA Contact: ian → style-system

    
    

    
  
I just independently rewrite the patch for this based on the failing test, so taking bug.
Assignee: mats.palmgren → dbaron
Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Here's my patch.  I think a new unit is better (they're cheap; we have plenty of integers left).

I pulled in the eCSSUnit_Null optimizations from Mats's patch, and also the nsComputedDOMStyle change that I'd missed (but probably would have noticed quickly once I tested it).

        Attachment #263956 -
        Attachment is obsolete: true

        Attachment #354615 -
        Flags: superreview?(bzbarsky)

        Attachment #354615 -
        Flags: review?(bzbarsky)

    
    

    
  
And I'll land Mats's reftest as well.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Oops, missed a few nsCSSValue changes for adding the unit.

        Attachment #354615 -
        Attachment is obsolete: true

        Attachment #354618 -
        Flags: superreview?(bzbarsky)

        Attachment #354618 -
        Flags: review?(bzbarsky)

        Attachment #354615 -
        Flags: superreview?(bzbarsky)

        Attachment #354615 -
        Flags: review?(bzbarsky)

        Attachment #354618 -
        Flags: superreview?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
er, really this time

        Attachment #354618 -
        Attachment is obsolete: true

        Attachment #354619 -
        Flags: superreview?(bzbarsky)

        Attachment #354619 -
        Flags: review?(bzbarsky)

        Attachment #354619 -
        Flags: superreview?(bzbarsky)

        Attachment #354619 -
        Flags: superreview+

        Attachment #354619 -
        Flags: review?(bzbarsky)

        Attachment #354619 -
        Flags: review+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/4e418f9c2b0f
http://hg.mozilla.org/mozilla-central/rev/5e3e645e59be
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

    
    

    
      
  

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: