Closed Bug 378217 Opened 17 years ago Closed 16 years ago

implement css3 'border-image' property

Categories

(Core :: Graphics, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: asmith16, Assigned: robarnold)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Keywords: css3, Whiteboard: [parity-webkit])

Attachments

(3 files, 30 obsolete files)

165.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
16.50 KB, text/plain
Details
6.22 KB, patch
robarnold
: review+
Details | Diff | Splinter Review
A description is available here: http://www.w3.org/TR/css3-background/#the-border-image

I am starting to try and figure out how this might be done.
Keywords: css3
Attached patch what i have so far (not working) (obsolete) — Splinter Review
I thought I should post what I've done so far. The patch is so horible I can't ask anyone to look at it but if anyone will, any comments (especially from an architecture standpoint) would be appreciated. Sorry it's taking so long, I've never done this kind of work before.

The parser part is mostly working, the renderer I barely touched. My plan is to get the border to show as quickly as I can and fix the ugly parts one by one after that. I don't think there's any way I can do it properly the first time.
Hey, this is awesome -- I'm glad you did the parser part first, because the renderer was very much up in the air until like.. last thursday.  Let's talk and figure out how to do the rendering; I think your best bet is to make sure the parser is solid and make sure you can get the right data down to nsCSSRendering::PaintBorder, and then let's talk about how to do the rendering (which I think will be much easier).
Attached patch partly working (obsolete) — Splinter Review
This is a version with many small improvements. I think there may still be a hack or two in it, but overall i don't expect to change anything significant.

I posted (at least i think i did) a message on the www-style mailing list to clarify the specification, there are too many ways to interpret how the rendering is to be done. Until that's worked out I don't know what I can do - maybe figure out how to make the resized images look more smooth.
Attachment #266836 - Attachment is obsolete: true
>+    imgIRequest *req = aPresContext->LoadImage(aBorderStyle.mBorderImage,
>+                                               aForFrame);

Won't this kill off any existing background image for the frame?  That doesn't seem like a good idea...  You may need to make that code deal with multiple images per frame...

The packing in nsStyleBorder could be better, no?

Does this implementation follow the "except table elements with border-collapse:collapse" thing?

You probably need to add tests for this stuff, including serialization and parsing tests.  See the tests in layout/style/test, in particular the property_database.js file there.  I suspect that once you add some values of this property to the property database and run those tests you'll find that it's not serialized correctly; I don't see any changes to nsCSSDeclaration in that patch, esp. to nsCSSDeclaration::AppendCSSValueToString.  The rendering tests should probably be done using reftest and such once we have answers on the rendering model.

I also don't see a computed style implementation in the patch...
Attached patch mostly working (obsolete) — Splinter Review
In this version:
- added nsPresContext::LoadBorderImage() (comment #4)
- improved the packing in nsStyleBorder
- renders all 3 types of border-images (stretch, repeat, round) correctly if I understood what fantasai said on the www-style mailing list

Things that still need fixing:
- I added border-image to property_database.js and now 17 tests fail. I have no idea what they're trying to do in the first place, so it's very hard for me to fix. If someone's willing to help, I'll put the logs up on the web.
- I added nsComputedDOMStyle::GetBorderImage() but (not surprisingly) it's never called. Solving this may help with the tests, but again, I don't know when and how that stuff is supposed to be used.

Things that may need fixing:
- Should I use -moz-border-image instead of border-image? There are two CSS3 properties in nsIDOMNSCSS2Properties without the prefix.
- The border image isn't drawn when the table has border-collapse:collapse, but I couldn't figure out what that's supposed to do, so I can't say for sure it's not broken. Does someone know a page that explains border-collapse:collapse well?
- I use nsLayoutUtils::DrawImage() for drawing every tile. Which means each tile is resized separately. This may be good for 'round', which works magically, but it's not good for 'repeat' where each tile will end up being exactly the same size. Strangely I can't find a function in the tree that will take an nsRect and return it resized.
- Another problem with nsLayoutUtils::DrawImage() is that it doesn't do a smooth resize, so it looks very ugly in many cases. I don't know what other options there are.

This is what the rendering looks like: http://littlesvr.ca/border-image/firefox-border-image-01.png

Comments please?
Attachment #268104 - Attachment is obsolete: true
You should use -moz-border-image, as the spec isn't quite stable and Safari is using -webkit-.

Our border-collapse code is a mess and needs rewriting (bug 203686). Don't worry about border-image not working there: it's not practical for you to try to fix it.
The tests that use property_database.js test the following things:

1)  Serialization of the property (e.g. getting foo.style.propName) gives a
    string that parses to the same thing as the original value of the property.
2)  Setting the property to the computed value doesn't change the computed
    value.

If they're failing, that means that your changes to nsCSSDeclaration and/or nsComputedDOMStyle are wrong.

In particular, you need to make the former (see also comment 4) and add your property to the list of properties that get computed at the end of nsComputedDOMStyle.cpp.  And then you need to make GetBorderImage return whatever is the right thing; just the URI looks pretty wrong to me, no?
Oh, and the resize is not something you should worry about.  We should be using the same resizing algorithm throughout, and the layout code should not need to worry what that is.

You probably want to do what nsCSSRendering::PaintBackgroundWithSC does wrt DrawImage vs DrawTile.  Or something.  Except I guess DrawTile can't resize, eh?  File bugs on better thebes APIs here?
Sorry it's taking me so long to take a look at the rendering portions... some comments:

- "horizontal", not "horisontal" :)

- for when you need to repeat the image, what you have is ok, but it would be much better (and potentially simpler) if you were to create a gfxPattern from the image, and set EXTEND_REPEAT on it.. and then just Fill() the region in a single shot.  EXTEND_REPEAT repeats in both directions though, so you'd have to make sure that your path was bound correctly on the one side.  You can resize as appropriate by setting a matrix on the gfxPattern before calling ctx->SetSource().

Other than that, it looks fine.
Attached patch patch ready for review (obsolete) — Splinter Review
Changes that I remember doing to the patch since the last one:
- all mochitests now pass
- added a couple of reftests
- property name changed to -moz-border-image
- redid the 'repeat' rendering to not use DrawImage
- doesn't crash on windows any more

Tested on Linux and Windows, I don't have access to a Mac.

It's ready for review.
Attachment #269676 - Attachment is obsolete: true
Attached file reftests (obsolete) —
I didn't know how to put these in the patch. A new directory with a few files to go into layout/reftests/.
Andrew, I'd ask "vladimir@pobox" for review and "dbaron@moz" for super-review on the patch...
Attachment #273108 - Attachment mime type: application/octet-stream → application/zip
Though I can tell you right now that NS_FOR_CSS_SIDES is there for a reason.  You should be using it in new code, not removing it in old code.

Oh, and nsComputedDOMStyle::GetBorderImage leaks on some of the allocation failures...
(In reply to comment #13)
> Though I can tell you right now that NS_FOR_CSS_SIDES is there for a reason. 
> You should be using it in new code, not removing it in old code.

This may well be my bad influence -- I didn't realize that macro existed until I'd already landed the new border chunks.  What's the reason for having it?  I assumed it was just a bit of extra readability...
Readability and making sure that everyone looks at the sides in the same order (e.g. no one's iterating backwards or whatnot).  But mostly readability.
Attached patch patch ready for review (obsolete) — Splinter Review
Fixed the potential memory leak in nsComputedDOMStyle::GetBorderImage().

I won't volunteer to use NS_FOR_CSS_SIDES. For beginners that macro is another nail to step on. Maybe for experts it adds to readability, I wouldn't know. If someone tells me to put it back, I will.
Attachment #273106 - Attachment is obsolete: true
Attachment #273606 - Flags: review?(vladimir)
Maybe I wasn't clear.  I'm telling you to put it back in all the places where you removed it and to use it for the new code.  ;)
Attached patch patch ready for review (obsolete) — Splinter Review
same patch, but using NS_FOR_CSS_SIDES. retested on linux just in case.

vlad, will you review the nsCSSRendering part (and the couple of cosmetic changes in gfx) please?
Attachment #273606 - Attachment is obsolete: true
Attachment #273690 - Flags: review?(vladimir)
Attachment #273606 - Flags: review?(vladimir)
Attachment #273690 - Attachment is patch: true
Attachment #273690 - Attachment mime type: application/octet-stream → text/plain
The change to reftest/README.txt is only relevant to non-DEBUG builds.  Since most developers should be using debug builds, I think it should be in parentheses, and should start with "If you're not using a DEBUG build,".
Attached patch patch ready for review (obsolete) — Splinter Review
Updated the readme. Sorry about the bugmail spam Vlad.
Attachment #273690 - Attachment is obsolete: true
Attachment #273708 - Flags: review?(vladimir)
Attachment #273690 - Flags: review?(vladimir)
Comment on attachment 273708 [details] [diff] [review]
patch ready for review

Looks fine, with a few issues:

> void
>+nsCSSRendering::DrawBorderImageSide(nsPresContext* aPresContext,
>+                                    nsIRenderingContext* aRenderingContext,
>+                                    imgIContainer* aImage,
>+                                    nsRect& aDestRect,
>+                                    const nsRect* aSourceRect,
>+                                    PRUint8 aFillType,
>+                                    PRBool aHorizontal)
>+{
>+  nscoord tileSize; // width or height, depending on aHorizontal
>+  nscoord origDestCoord; // x or y, depending on aHorizontal
>+  nscoord origDestSize; // width or height, depending on aHorizontal
>+  
>+  if (aDestRect.width == 0 || aDestRect.height == 0 ||
>+      aSourceRect->width == 0 || aSourceRect->height == 0) {
>+    return;
>+  }
>+  
>+  if (aFillType == NS_STYLE_BORDER_IMAGE_STRETCH) {
>+    // XXX this resize doesn't look very smooth, needs to be replaced with something
>+    nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+    return;
>+  }
>+  
>+  if(aFillType == NS_STYLE_BORDER_IMAGE_REPEAT) {
>+    nsresult rv;
>+    
>+    nsCOMPtr<gfxIImageFrame> frame;
>+    rv = aImage->GetCurrentFrame(getter_AddRefs(frame));
>+    if (NS_FAILED(rv))
>+      return;
>+    nsCOMPtr<nsIImage> image;
>+    image = do_GetInterface(frame);
>+    if (!image)
>+      return;
>+    
>+    // surface for the whole image
>+    gfxASurface* imageSurface = nsnull;
>+    rv = image->GetSurface(&imageSurface);
>+    if (NS_FAILED(rv))
>+      return;

This leaks -- Get methods always addref, so this should be:

  nsRefPtr<gfxASurface> imageSurface;
  rv = image->GetSurface(getter_AddRefs(imageSurface));
  if (NS_FAILED(rv) || !imageSurface)
    return;



>+    gfxIntSize gfxSourceSize(nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->width),
>+                             nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->height));
>+    
>+    // surface for just the tile i'm interested in drawing
>+    nsRefPtr<gfxASurface> tileSurface = gfxPlatform::GetPlatform()->CreateOffscreenSurface(
>+                                           gfxSourceSize, gfxASurface::ImageFormatARGB32);
>+    
>+    nsRefPtr<gfxContext> tileCtx = new gfxContext(tileSurface);
>+    tileCtx->SetSource(imageSurface, -gfxPoint(nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->x),
>+                                               nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->y)));
>+    tileCtx->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    tileCtx->Paint();
>+    
>+    // to resize the tile to fit in the border exactly
>+    gfxFloat scaleRatio;
>+    if (aHorizontal) {
>+      scaleRatio = (gfxFloat)aSourceRect->height / aDestRect.height;
>+    } else {
>+      scaleRatio = (gfxFloat)aSourceRect->width / aDestRect.width;
>+    }
>+    
>+    // width and height are proportional, so using the same ratio is ok
>+    gfxMatrix scaleMatrix;
>+    scaleMatrix.Scale(scaleRatio, scaleRatio);
>+    
>+    // pattern to scale and repeat the tile
>+    nsRefPtr<gfxPattern> pattern = new gfxPattern(tileSurface);
>+    pattern->SetExtend(gfxPattern::EXTEND_REPEAT);
>+    pattern->SetMatrix(scaleMatrix);
>+    
>+    // where the actual border ends up being rendered
>+    gfxRect gfxDestRect;
>+    gfxDestRect.pos.x = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.x);
>+    gfxDestRect.pos.y = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.y);
>+    gfxDestRect.size.width = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.width);
>+    gfxDestRect.size.height = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.height);
>+    
>+    // offset to make the middle tile centered in the middle of the border
>+    gfxPoint renderOffset(0, 0);
>+    gfxFloat actualTileSize;
>+    int numWholeTiles;
>+    gfxFloat edgeTileSize;
>+    if (aHorizontal) {
>+      actualTileSize = gfxSourceSize.width / scaleRatio;
>+      
>+      numWholeTiles = (int)(gfxDestRect.size.width / actualTileSize);
>+      if (numWholeTiles % 2 == 0)
>+        numWholeTiles--;
>+      
>+      edgeTileSize = (gfxDestRect.size.width - numWholeTiles * actualTileSize) / 2;
>+      
>+      gfxDestRect.pos.x -= (actualTileSize - edgeTileSize);
>+      
>+      renderOffset.x += (actualTileSize - edgeTileSize);
>+    } else {
>+      actualTileSize = gfxSourceSize.height / scaleRatio;
>+      
>+      numWholeTiles = (int)(gfxDestRect.size.height / actualTileSize);
>+      if (numWholeTiles % 2 == 0)
>+        numWholeTiles--;
>+      
>+      edgeTileSize = (gfxDestRect.size.height - numWholeTiles * actualTileSize) / 2;
>+      
>+      gfxDestRect.pos.y -= (actualTileSize - edgeTileSize);
>+      
>+      renderOffset.y += (actualTileSize - edgeTileSize);
>+    }
>+    
>+    // render
>+    nsRefPtr<gfxContext> thebesCtx = (gfxContext*)
>+            aRenderingContext->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);
>+    thebesCtx->Save();
>+    thebesCtx->Translate(gfxDestRect.pos);
>+    thebesCtx->SetPattern(pattern);
>+    thebesCtx->NewPath();
>+    thebesCtx->Rectangle(gfxRect(renderOffset, gfxDestRect.size));
>+    thebesCtx->Fill();
>+    thebesCtx->Translate(-gfxDestRect.pos);

Don't bother with this second Translate -- the transformation matrix will be restored when you call Restore()


>+    thebesCtx->Restore();
>+    
>+  } else if(aFillType == NS_STYLE_BORDER_IMAGE_ROUND) {
>+    if (aHorizontal) {
>+      int numNormalTiles; /* tiles that keep their proportions */
>+      
>+      tileSize = (aDestRect.height * aSourceRect->width) / aSourceRect->height;
>+      tileSize = (nscoord)(aDestRect.width / ceil((float)aDestRect.width / tileSize));
>+      
>+      numNormalTiles = aDestRect.width / tileSize;
>+      
>+      origDestCoord = aDestRect.x;
>+      
>+      origDestSize = aDestRect.width;
>+      aDestRect.width = tileSize;
>+      
>+      // draw the tiles
>+      while(aDestRect.x <= origDestCoord + origDestSize - tileSize) {
>+        nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+        aDestRect.x += tileSize;
>+      }
>+      
>+      // restore original values
>+      aDestRect.x = origDestCoord;
>+      aDestRect.width = origDestSize;
>+    } else {
>+      int numNormalTiles; /* tiles that keep their proportions */
>+      
>+      tileSize = (aDestRect.width * aSourceRect->height) / aSourceRect->width;
>+      tileSize = (nscoord)(aDestRect.height / ceil((float)aDestRect.height / tileSize));
>+      
>+      numNormalTiles = aDestRect.height / tileSize;
>+      
>+      origDestCoord = aDestRect.y;
>+      
>+      origDestSize = aDestRect.height;
>+      aDestRect.height = tileSize;
>+      
>+      // draw the tiles
>+      while(aDestRect.y <= origDestCoord + origDestSize - tileSize) {
>+        nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+        aDestRect.y += tileSize;
>+      }
>+      
>+      // restore original values
>+      aDestRect.y = origDestCoord;
>+      aDestRect.height = origDestSize;
>+    }
>+  }
>+}
>+
>+void

I read over the CSS changes, though I didn't look at them in too much detail -- I assume dbaron looked at that?

Otherwise, looks fine -- if you can attach a new patch with the above things fixed, I can get this checked in for you once the tree reopens (you don't have cvs access, right?  Can't rememeber.)
Attachment #273708 - Flags: review?(vladimir) → review-
Hadn't really looked yet:


In nsCSSParser::ParseBorderImage, you should combine the initial ParseVariant calls -- it takes a mask.

In nsCSSParser::ParseBorderImage, you need to call ExpectEndProperty before returning PR_TRUE.  (Otherwise you will accept values with garbage at the end.)

In nsStyleStruct.h, the comment after mBorderImageSplit should say what types the nsStyleSides have in them, as most of the comments for nsStyleCoord and nsStyleSides values do.

I think mBorderHorizontalFill and mBorderVerticalFill should be named mBorderImage*.  You could abbreviate Horizontal and Vertical to H and V (or X and Y?) if that makes them too long.

Why did you make all the changes to nsStyleStructList.h ?

property_database.js should test some additional invalid_values possibilities, such as too many of the various types of repeated values, and a slash with no numbers (just keywords) following, and a slash with nothing following.

In nsRuleNode, the None and Initial cases should be combined with || rather than separated.

In nsRuleNode, where you call SetCoord, could you comment that passing an uninitialized parentCoord is ok because you're not passing SETCOORD_INHERIT?

+        if (coord.GetUnit() == eStyleUnit_Integer ||
+            coord.GetUnit() == eStyleUnit_Percent) {

You don't need this, since you passed in flags to guarantee it.


The stuff you're doing that messes with styles, transparent, and widths (in both the array and inherit cases) doesn't seem like it's going to work when stuff gets cached in the rule tree.  I think this bit needs to be redesigned, or things will break badly in cases when all properties in the struct are specified, particularly when across different rules in certain orders.  But I need to think about this some more...

+        // have vertcal value
vertical

I also haven't looked at the nsComputedDOMStyle code yet.
(In reply to comment #22)
> or things will break badly in cases when all properties in the struct are
> specified, particularly when across different rules in certain orders.  But I

Er, this is a reset struct, so it doesn't require all properties to be specified to cause problems.  It just requires other border properties vs. border-image to be specified in different cascading orders.
(Whoops, my bad -- I thought you'd already looked this over.  Waiting until these issues get resolved.)
Attached patch patch with many small fixes (obsolete) — Splinter Review
Fixed the problems Vlad mentioned in comment #21, and most of the problems mentioned by dbaron in comment #22.

> Why did you make all the changes to nsStyleStructList.h ?
I couldn't read it otherwise. Is it a problem?

> The stuff you're doing that messes with styles, transparent, and widths (in
> both the array and inherit cases) doesn't seem like it's going to work when
> stuff gets cached in the rule tree.  I think this bit needs to be redesigned,
> or things will break badly in cases when all properties in the struct are
> specified, particularly when across different rules in certain orders.  But I
> need to think about this some more...
Hm.. right. At first I didn't want to deal with the many changes that would be required if I kept the border-widths separate. Then i forgot about it cause it seemed to work. I'll see if I can think of a way. Do you have any suggestions?
Attachment #273708 - Attachment is obsolete: true
(In reply to comment #25)
> > Why did you make all the changes to nsStyleStructList.h ?
> I couldn't read it otherwise. Is it a problem?

I don't think it's particularly readable either way, and I'd rather not mess up the cvs blame.

> > The stuff you're doing that messes with styles, transparent, and widths (in
> > both the array and inherit cases) doesn't seem like it's going to work when
> > stuff gets cached in the rule tree.  I think this bit needs to be redesigned,
> > or things will break badly in cases when all properties in the struct are
> > specified, particularly when across different rules in certain orders.  But I
> > need to think about this some more...
> Hm.. right. At first I didn't want to deal with the many changes that would be
> required if I kept the border-widths separate. Then i forgot about it cause it
> seemed to work. I'll see if I can think of a way. Do you have any suggestions?

There may be a way to make the approach you're using work with some additional changes to the code in nsRuleNode, but I need to look into that...
Attached patch patch using mBorderImageWidth (obsolete) — Splinter Review
dbaron: will this work? I added mHaveBorderImgeWidth and mBorderImageWidth to nsStyleStruct. changed GetBorder() and GetBorderWidth() to return mBorderImageWidth when it's set.

I no longer touch the border style, transparent, or the 'other' border-width.

I expected to need GetActualBorder() and GetActualBorderWidth(), but everything seems to work without them. It should be reasonably easy to replace existing GetBorder() and GetBorderWidth() calls with these, where layout and rendering is not an issue.
The idea seems like it will work, but:
 * you should include the "a" in "Image"
 * you need to add code to set it to false when any of the other values are specified or when a border-image without / is specified, and inherit it when inherit is specified
 * you shouldn't leave the commented-out code
 * you need to audit all callers of GetBorder to figure out which one they want
 * I'd use the opposite names:  GetActualBorder() should include the border image width, GetComputedBorder() should not.  (And I'd replace GetBorder with two newly-named functions so you're sure to change all callers to whichever is right.)
 * I don't see why you're changing protected to public.
 * you need to copy the new member in the copy-constructor

(There might be some other problems, but that's what I saw at a quick skim.  Hopefully the additional computation in GetActualBorder won't make things too slow.)
Also:
 * you should add the new property to the end of nsIDOMNSCSS2Properties
 * The nsComputedDOMStyle code should check your new variable and output or not output the width section (and the code for what it outputs needs to be updated)
 * The nsComputedDOMStyle code should return none as a value, not as a value list.  (See GetContent or GetCounterIncrement for examples.)
 * In general, I'd suggest putting the allocation checks and appends right after the GetROCSSPrimitiveValue calls rather than putting other code in between.
Attached patch patch with more small fixes (obsolete) — Splinter Review
Ok, this should do. I can't test it because the trunk is broken on linux, but I'm pretty sure it's right.

dbaron, will you reviw it please?
Attachment #274623 - Attachment is obsolete: true
Attachment #274776 - Attachment is obsolete: true
Attachment #275814 - Flags: review?
Attached patch patch with more small fixes (obsolete) — Splinter Review
Urmph the previous attachment had some junk in it.
Attachment #275814 - Attachment is obsolete: true
Attachment #275815 - Flags: review?
Attachment #275814 - Flags: review?
Attachment #275815 - Flags: review? → review?(dbaron)
Did you run the style system mochitests?

If the trunk is too broken to test, then the tree needs to be closed.
(In reply to comment #32)
> Did you run the style system mochitests?
> 
I did, and they all pass. It just won't render pages properly, so I can't do the manual testing I'm used to.

That's not caused by this patch because it's the same on a clean build and a nightly I tried. It could be that there's something wrong with my system though I don't recall changing anything before it broke.
Comment on attachment 275815 [details] [diff] [review]
patch with more small fixes

So I missed that there was already a GetComputedBorderWidth.  I think,
given that, the function you're adding called GetComputedBorder should
be called GetRoundedBorder, and there should be a comment explaining
that the process is
 computed ==[rounding]==> rounded ==[including border-image]==> actual
(But I agree that the change in the eCSSUnit_Inherit case in
nsRuleNode::ComputeBorderData is correct; the old code was actually
slightly wrong, and you should be using GetComputedBorderWidth.)

You shouldn't be inserting extra trailing whitespace in
nsCSSRendering::PaintBorder and PaintOutline.


I don't see why you can't just use nsPresContext::LoadImage -- I don't
see why any nsPresContext changes are needed at all.

In nsCSSDeclaration.cpp:

+        if (array->Item(i).GetUnit() == eCSSUnit_Null) {
+          continue;
+        }
+        else if (i == 5) {
+          aResult.AppendLiteral(" /");
+        }

No need for the "else ".

In nsCSSParser.cpp:

+  if (ParseVariant(aErrorCode, mTempData.mMargin.mBorderImage, 
+                   VARIANT_INHERIT | VARIANT_NONE, nsnull)) {
+    return PR_TRUE;
+  }

You need to SetPropertyBit before returning PR_TRUE.  (I'm surprised
this didn't break anything.  Did you not see any assertions (in
DoTransferTempData) or test failures?  If not, why not?)

+  mTempData.mMargin.mBorderImage.SetArrayValue(arr, eCSSUnit_Array);
+  
+  mTempData.SetPropertyBit(eCSSProperty_border_image);

Could you not put a blank line between those two?

nsRuleNode.cpp:

NEED TO DO WHOLE FILE
+      NS_FOR_CSS_SIDES(side) {
+        // an uninitialized parentCoord is ok because I'm not passing SETCOORD_INHERIT
+        if (SetCoord(arr->Item(5 + side), coord, parentCoord, SETCOORD_LENGTH, 
+                     aContext, mPresContext, inherited)) {
+          if (coord.GetUnit() == eStyleUnit_Coord) {
+            border->mBorderImageWidth.side(side) = coord.GetCoordValue();
+            border->mHaveBorderImageWidth = PR_TRUE;
+          }
+        }

It seems like you shouldn't have separate error handling for each side.
I think, instead:
 1. you should move the assignment to mHaveBorderImageWidth out of the
    loop over sides.
 2. assert (NS_ASSERTION) that SetCoord returns true rather than testing 
 3. handle eStyleUnit_Chars by setting the side to 0 (rather than
    letting the cases you don't handle be inconsistent depending on what
    other rules are specified and what other styles get computed first.)
    And you should probably also have an NS_WARNING.


+        // have vertcal value
vertical needs an "i"


There are some blank lines in nsStyleStruct.h where you added extra
trailing whitespace.

nsStyleStruct.cpp:

  You need to initialize the new member variables in the non-copy
  constructor too.  (How did this not cause test failures?)


Also, the first point in comment 29.

And your invalid_values tests in property_database.js all have an extra
"]" in them, and the last three tests need to have numbers in them too.

I think that should be it, though.
Attachment #275815 - Flags: review?(dbaron) → review-
> I don't see why you can't just use nsPresContext::LoadImage

See comment 4.  If you have an animated background and animated border image, you want invalidates from both... LoadImage only keeps track of a single image, though.

Attached patch patch with yet more small fixes (obsolete) — Splinter Review
Fixed everything mentioned in comment #34, except:

- am still not using nsPresContext::LoadImage()

- '2. assert (NS_ASSERTION) that SetCoord returns true rather than testing' Well, I copied the code from the beginning of the function where the actual border-image is dealt with, I don't see why this should be different.

- 'you should add the new property to the end of nsIDOMNSCSS2Properties' Can you clarify please? They are all in alphabetical order.

And some answers:

>You need to SetPropertyBit before returning PR_TRUE.  (I'm surprised
>this didn't break anything.  Did you not see any assertions (in
>DoTransferTempData) or test failures?  If not, why not?)
I couldn't make a debug build for the longest time, and didn't bother when I fixed it. Checked now though, to make sure there are no assertions. Tests always passed unless I said otherwise, I don't know how most of them work so I can't answer that.

>You need to initialize the new member variables in the non-copy
>  constructor too.  (How did this not cause test failures?)
All those values are ignored unless nsCOMPtr mBorderImage is true. nsCOMPtrs are initialised to 0 in the default constructor. But I now added them all to the nsStyleBorder constructor anyway.
Attachment #275815 - Attachment is obsolete: true
Attachment #278043 - Flags: review?(dbaron)
(In reply to comment #36)
> - '2. assert (NS_ASSERTION) that SetCoord returns true rather than testing'
> Well, I copied the code from the beginning of the function where the actual
> border-image is dealt with, I don't see why this should be different.

You should fix it in both cases.  SetCoord will only return false if the value is not one of the cases that it's given in the bitfield.  You should know what the possible cases are, so if it's always going to return true you should be able to assert that rather than test it at runtime.  The assertion also helps catch if you're wrong, since it means that there's another case you need to handle.
 
> - 'you should add the new property to the end of nsIDOMNSCSS2Properties' Can
> you clarify please? They are all in alphabetical order.

The ones that were there when the interface was created were alphabetical; new members are added to the end because it makes binary compatibility easier (although still a pain, but it's good practice for interfaces that people might be using from outside Mozilla).  (Although maybe a few were slipped in to the middle.)

> >You need to initialize the new member variables in the non-copy
> >  constructor too.  (How did this not cause test failures?)
> All those values are ignored unless nsCOMPtr mBorderImage is true. nsCOMPtrs
> are initialised to 0 in the default constructor. But I now added them all to
> the nsStyleBorder constructor anyway.

Oops.  Right, you don't need this, then.  Sorry about that.
Attached patch patch with a couple more fixes (obsolete) — Splinter Review
Fixed everything mentioned in comment #37
Attachment #278043 - Attachment is obsolete: true
Attachment #278076 - Flags: review?(dbaron)
Attachment #278043 - Flags: review?(dbaron)
Blocks: songbird
You probably also need to make some changes here
  http://mxr.mozilla.org/seamonkey/source/layout/base/nsFrameManager.cpp#1146
  // if old context had image and new context does not have the same image, 
  // stop the image load for the frame
Not sure--just happened to run across it and thought it might be relevent.
No longer blocks: 387345
Andrew/David is this something we still want to try for?
I would really like to see this make it.
(In reply to comment #41)
> I would really like to see this make it.
> 

why?
Blocks: 403158
Will this CSS feature make it into Firefox 3? I've tried it on the current beta 3 release, but it does not work (it works in Safari). See temporary link:
http://www.datagrafikk.no/cssdemo/mybubble.htm
Whiteboard: [parity-safari]
Flags: blocking1.9?
Could you inform me of why? It looks like it's almost ready for release, and this is an important CSS feature that would help a lot when designing webpages with different types of border areas (that means a lot of pages). Should FireFox goal not be to handle most of the CSS standards? Right now Safari is way ahead on this area.
It missed the deadline.  See http://dbaron.org/log/2007-10#e20071031b .  We want to ship what we have, then add more.
Flags: blocking1.9? → blocking1.9-
Comment on attachment 278076 [details] [diff] [review]
patch with a couple more fixes

I'm working on merging this to trunk.

>+        NS_ASSERTION(SetCoord(value, coord, parentCoord, SETCOORD_LENGTH, aContext,
>+                              mPresContext, inherited),
>+                     "SetCoord for border-width failed");

This (a change from the previous patch) isn't going to work in non-debug builds.
This merges attachment 278076 [details] [diff] [review] and attachment 273108 [details] to trunk and fixes a bunch of problems I noticed.

It passes mochitest, but the second of the two reftests it adds fails.  I still need to figure out why that reftest is failing, and review the rest of the patch.
Attachment #273108 - Attachment is obsolete: true
Attachment #278076 - Attachment is obsolete: true
Attachment #278076 - Flags: review?(dbaron)
Attached patch patch with more reftests (obsolete) — Splinter Review
This adds a bunch of additional reftests in addition to the ones in attachment 273108 [details].  The patch fails most of the ones I just added; WebKit passes most of them (although not quite all).  So I think there's a good bit more work to be done.  (Though I admit there are probably a few mistakes in my tests as well.)

This still also needs reftests for the image scaling rules and for the stretch/round/repeat keywords.
Attachment #320999 - Attachment is obsolete: true
Attachment #321677 - Flags: review?(dbaron)
Attachment #320999 - Flags: review?(dbaron)
Comment on attachment 321677 [details] [diff] [review]
patch with more reftests

Two things I've noticed recently:

 1) I forgot to add my additional reftests to the reftest.list

 2) The following line

>diff --git a/gfx/thebes/public/gfxImageSurface.h b/gfx/thebes/public/gfxImageSurface.h

>+    nsRefPtr<gfxContext> thebesCtx = (gfxContext*)
>+            aRenderingContext->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);

should just be:

gfxContext *thebesCtx = aRenderingContext->ThebesContext();

(i.e., don't use nsRefPtr, and use ThebesContext rather than GetNativeGraphicData).
Some other known issues:
 * we need to make the border-drawing code draw the center image
   + we need to fix nsDisplayBorder::OptimizeVisibility to not bail for the
     dirty rect inside the padding rect case if mBorderImage is non-null
And (per debugging I just did with Rob Arnold) we need to fix the various setters in nsStyleBorder that check IsVisibleStyle to also check for the presence of a border-image, since a border-image overrides border-style.
Attached patch Added drawing of center image (obsolete) — Splinter Review
Attachment #321677 - Attachment is obsolete: true
Attachment #321677 - Flags: review?(dbaron)
Attached patch patch (obsolete) — Splinter Review
Attachment 322849 [details] [diff] fixed the second half of 50 and the first half of comment 51.

This fixes:
 * the first half of comment 50
 * the second half of comment 51
 * comment 52
 * fixes the parsing code to match a recent change to the spec allowing 1-4 values for the <number>|<percentage> values giving the offsets into the image (and repeating them per usual CSS rules) rather than requiring all 4.

This gets all the reftests passing except for the bug where tests are showing a gap at the end of the tiling.
Attachment #322849 - Attachment is obsolete: true
So the solid-image-2 reftest fails for me on Linux both looking at it at multiple DPI values (96, 120, 144) and using the reftest harness, but it doesn't fail for Rob on Windows...
Comment on attachment 322861 [details] [diff] [review]
patch

>+      if(rectToDrawSource.height != 0 && rectToDrawSource.width != 0 && rectToDraw.width != 0 && rectToDraw.height != 0) {

The rectToDrawSource checks actually need to be for > 0, since it could end up less than 0.  I think the same problem exists for the sides; not sure why there's no code at all to check for that case.

Checking for > 0 does agree with the spec, which says:

# If the sum of the right and left widths is equal to or
# greater than the width of the image, the images for the
# top and bottom edge and the middle part are empty, which
# has the same effect as if a nonempty transparent image
# had been specified for those parts. Analogously for the
# top and bottom values.
Also, the code for drawing the middle needs to use DrawBorderImageSide (potentially with modifications) so that you get the right tiling / stretching / rounding behavior in both directions.
I'm working on some additional tests.

Additional bugs I've noticed:
 * the parser doesn't accept non-integer values for the pixel offsets in the image, but the spec says it should.  (I'm not sure I agree with the spec here.  We should check what WebKit does and maybe propose a change to the spec.)

 * We don't round the border-width overrides that come from the -moz-border-image property the same way we round the border-width property (always down).  I think we should.  I have a test for this.
According to the spec, the border-width override should only happen if the image can be displayed. The current patch does not do this.
Attached patch patch (obsolete) — Splinter Review
This should fix the build errors on Windows with the previous patch; it also adds a few more tests, although there are still a bunch more I'd like to write.
Attachment #322861 - Attachment is obsolete: true
(In reply to comment #59)
> According to the spec, the border-width override should only happen if the
> image can be displayed. The current patch does not do this.

I was just discussing this with Rob.  To fix this, we need something roughly like:
 * when we construct the nsImageLoader in LoadBorderImage, we need to pass the image loader a boolean saying whether the frame requires relayout when the image loads
 * inside nsImageLoader, we need a boolean passed to RedrawDirtyFrame that says this is a redraw for the image loading (rather than for animation)
 * when both booleans are true inside RedrawDirtyFrame, we need to call frame->GetPresContext()->PresShell()->FrameNeedsReflow(STYLE_CHANGE)
 * to avoid a race condition where the image load completes between reflow and repaint, we need to ensure that LoadBorderImage is called the first time we reflow an element with a border image

We can probably have automated tests for this (including the race condition case) using the HTTP server (with js-driven CGI) in mochitest, and we can probably even test the race condition by using things that force flushing of layout.

This may be complex enough that we want to postpone it until after this patch lands.  (I'm curious whether WebKit gets all of this correct.)
marking wanted1.9.1? to get this in the triage queue.  Also set to P2.
Flags: wanted1.9.1?
Priority: -- → P5
Oh, and we also need to update property_database.js to move one of the invalid values (the one with a single image offset rather than four) to the list of valid ones so that test_property_syntax_errors.html doesn't fail.
Assignee: asmith15 → tellrob
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1-
And I got some linker errors on Mac because I didn't include nsStyleStructInlines in nsHTMLStyleSheet, nsImageFrame, nsTablePainter, nsTableFrame, and nsTableCellFrame.  (gcc also gives a "used but never defined" warning.)

I'm thinking we might want to set up a shared hg repo for this rather than passing patches back and forth...
Attached patch patch (mostly gfx fixes) (obsolete) — Splinter Review
I had to rewrite all the drawing code to draw the middle image and I updated it to follow the spec. It currently passes all the reftests on windows (save 1 which is due to a race condition w/reflow and image loading).

There are some pixel alignment/rounding issues still at some zoom sizes, but the integral ones seem ok.

Other notable changes:
* modified nsImageLoader to optionally reflow instead of repaint.
* Border-image loading is started in nsHTMLReflowState::Init because it's called for every reflow
* Added a reftest for repeat
Attachment #323172 - Attachment is obsolete: true
Why are there race conditions?  I wouldn't think there should be any... Or at least onload shouldn't fire until after all the loading is done.
I noticed on the border-image spec page (http://dev.w3.org/csswg/css3-background/#the-border-image) that the image for Figure 6 does not always load when the page is loaded; I have to refresh the page in order for it to show up (other people have seen this behavior on this page too). I suspect that the underlying cause may be the same as the cause for the 1 failing reftest.

Also, my last diff did not properly include the reftests for some unknown reason.
Attached patch border-image patch (obsolete) — Splinter Review
I've cleaned out some unrelated changes that made it into the patch. I'm also moving all the tests into their own patch.
Attachment #324704 - Attachment is obsolete: true
Attached patch border-image tests (obsolete) — Splinter Review
These are the reftests and property tests. Known failures are the second two pixel-rounding tests and solid-image-1 test (which fixes itself when the page is reloaded).
Attached patch border-image patch (obsolete) — Splinter Review
DrawBorderImageSide no longer creates an intermediate surface for each part of the tile when drawing. It uses a matrix now to scale the image. Also, I disabled the nearest neighbor filtering which I accidentally left on in the last patch version.
Attachment #328008 - Attachment is obsolete: true
Attached patch border-image tests (obsolete) — Splinter Review
Disabling the filter in the last patch now causes the repeat-image-1 reftest to fail (as is expected).
Attachment #328009 - Attachment is obsolete: true
Attached patch border-image patch (obsolete) — Splinter Review
Cleaned up some redundant computations in the drawing code and also finally fixed border width overrides. Passes all the reftests in the border-image patch.
Attachment #328046 - Attachment is obsolete: true
Attachment #328782 - Flags: review?
Attachment #328782 - Flags: review? → review?(dbaron)
Just noticed an error introduced recently:  the IID bump in nsIDOMCSS2Properties.idl is on the wrong interface (the frozen nsIDOMCSS2Properties, rather than the unfrozen and actually modified nsIDOMNSCSS2Properties).
Comment on attachment 328782 [details] [diff] [review]
border-image patch

Vlad, could you review the code in nsCSSRendering?
Attachment #328782 - Flags: review?(vladimir)
Comment on attachment 328782 [details] [diff] [review]
border-image patch


>+          if (side == 0 || side == 2)

Use the symbolic constants here (NS_SIDE_TOP, NS_SIDE_BOTTOM)

Here and elsewhere actually; there are a bunch of places where [0] [2] etc. are hardcoded.

One of the things that I don't like is that in DrawBorderImageSide, a temporary surface is created every time for the source rect.  This is kinda crappy, since the same source rect will always be used.  Can we (as a followup patch) reuse the segments, since they're going to remain constant sized?

Other than that, the CSSRendering parts look fine.
Attachment #328782 - Flags: review?(vladimir) → review+
The diff is missing nsStyleStructInlines.h .  Could you rediff with that included?
And the one thing I noticed so far:

+  nscoord GetActualBorderWidth(PRUint8 aSide) const
   {
-    return mActualBorder.side(aSide);
+    if (IsBorderImageLoaded())
+      if (mHaveBorderImageWidth)
+        return mBorderImageWidth.side(aSide);
+      else
+        return mBorder.side(aSide);
+    else
+      return mActualBorder.side(aSide);
   }

I think this should just be "return GetActualBorder().side(aSide);"
Blocks: 444658
Attached patch border-image patch (obsolete) — Splinter Review
Addressed comments from vlad (filed bug 444658) and comments from dbaron
Attachment #328782 - Attachment is obsolete: true
Attachment #328976 - Flags: review?
Attachment #328782 - Flags: review?(dbaron)
Attachment #328976 - Flags: review? → review?(dbaron)
Attached patch review in the form of a diff (obsolete) — Splinter Review
I have this in my tree preparing to land.  Here's a review in the form of a diff, although I haven't gone over everything yet -- mainly just looked over the changes.  I'd like to go over the whole thing again before landing.

I also intend to do two renames of members of nsStyleBorder in addition to this:
  GetRoundedBorder() -> GetComputedBorder()
  mActualBorder -> mComputedBorder
(And there was also quite a bit of merging to do -- which I've done -- mostly with bug 212633, although also with bug 363706.  Your tree is pretty out of date.)
So, when I run full reftests, I crash here:

#4  <signal handler called>
#5  gfxASurface::AddRef (this=0x0)
    at /home/dbaron/builds/mozilla-central/mozilla/gfx/thebes/src/gfxASurface.cpp:68
#6  0x00007f35f979daec in nsThebesImage::GetSurface (
    this=<value optimized out>, aSurface=0x7fff13c6e3f0)
    at /home/dbaron/builds/mozilla-central/mozilla/gfx/src/thebes/nsThebesImage.h:98
#7  0x00007f35fa0db113 in nsCSSRendering::DrawBorderImageSide (
    aThebesContext=0xe9fd00, aDeviceContext=<value optimized out>, 
    aImage=0x2567740, aDestRect=@0x7fff13c6e5a0, aInterSize=@0x7fff13c6e5b0, 
    aSourceRect=@0x7fff13c6e580, aHFillType=0 '\0', aVFillType=0 '\0')
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsCSSRendering.cpp:4385

loading mozilla/layout/reftests/border-image/transparent-image-1.html
... in particular, mSinglePixel for that image is true, so it doesn't have a surface.  (Can you do what you want using GetPattern?)
Attached patch merged patch and tests (obsolete) — Splinter Review
This incorporates attachment 328047 [details] [diff] [review] and attachment 328976 [details] [diff] [review], plus my comments above, and also:
 * Put GetActualBorder in nsStyleStruct.cpp, since it depends on nsStyleStructInlines
 * additional merging with box-shadow
Attachment #328047 - Attachment is obsolete: true
Attachment #328976 - Attachment is obsolete: true
Attachment #329752 - Attachment is obsolete: true
Attachment #328976 - Flags: review?(dbaron)
I don't see the crash when I run the border-image reftests. I should be able to fix the code to use GetPattern which will hopefully fix it.
These are the rest of my review comments (I went through the whole thing).  Once I make sure this compiles and runs, I'll attach the merged patch.

Then all that remains is:
 * Rob reviewing the changes I made in this diff and the previous review comments diff
 * somebody fixing the crash on the reftest with the solid image.  (I'm not sure if there are any performance implications of pattern vs. surface -- whether we should try to use a pattern all the time (can we), or fork at the caller.)
(In reply to comment #85)
>  * somebody fixing the crash on the reftest with the solid image.  (I'm not
> sure if there are any performance implications of pattern vs. surface --
> whether we should try to use a pattern all the time (can we), or fork at the
> caller.)

Using a surface as a source will result in a pattern internally anyway, so should be no perf hit with using a pattern directly (and that would be ideal).
This is just a diff against your last merged patch. It hopefully fixes the crash you were seeing
Attachment #329863 - Flags: review?(vladimir)
Attachment #329752 - Flags: review?(tellrob) → review+
Attachment #329859 - Flags: review?(tellrob) → review+
Has attachment 329859 [details] [diff] [review] and attachment 329863 [details] [diff] [review] merged in.

I'll land this after lunch if the tree is green...
Attachment #329772 - Attachment is obsolete: true
Attachment #329859 - Attachment is obsolete: true
Attachment #329863 - Attachment is obsolete: true
Attachment #329877 - Flags: review+
Attachment #329772 - Flags: review?(dbaron)
These two reftests were failing on mac (paste this into reftest-analyzer).  I marked them as such, but need to do more analysis tomorrow.
No longer blocks: 403158
Depends on: 445810
These are the changes I made to fix the tree last night -- a bunch of methods didn't need to be inlined anymore now that the presence of a border image no longer affects the values in mActualBorder (and we don't need to call RebuildActualBodrer in SetBorderImage).

This is just hg diff -r9b0b2391485c:a8f655e642fb.
Attachment #330134 - Flags: review?(tellrob)
Comment on attachment 330134 [details] [diff] [review]
the changes I made to fix the build

It's after the fact, but oh well it looks good.
Attachment #330134 - Flags: review?(tellrob) → review+
(In reply to comment #89)
> Created an attachment (id=329964) [details]
> reftest failures on mac
> 
> These two reftests were failing on mac (paste this into reftest-analyzer).  I
> marked them as such, but need to do more analysis tomorrow.

I looked at these in GIMP, and it is the test that's showing colors that are off (not the reference).  So I filed bug 445911.

And marking fixed, since this landed yesterday.

(But we could really use some additional reftests.)
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Depends on: 446328
Depends on: 446338
No longer depends on: 446338
We see a slight color variation and reftest failure with this on the SeaMonkey MacOSX (10.4) box now that we switched it to 1.9.1/hg.

The failure is in the div body of multicolor-image-2.html div.one (NOT in the actual borders), which is solid #4a298e on the reference rendering but has 6 of its 9 pixels at #4b2a8e (*+**+**+* where * is off and + is correct).

I fail to find where the test even defines that color for the div content area itself, but I just might not be an expert on that.
(In reply to comment #95)
> I fail to find where the test even defines that color for the div content area
> itself, but I just might not be an expert on that.

The border-image fills the content area too (if it's not transparent in that part of the image).
(In reply to comment #96)
> The border-image fills the content area too (if it's not transparent in that
> part of the image).

Ah, ok, so that's where we get this color from. In that case, this looks to me like at least this box, perhaps all Tiger boxes (as FF has none to test it), fails the test with very similar symptoms to bug 445911, actually. Can we do something about this?
Um, the tests are already marked as known to fail on Mac for bug 445911, so that shouldn't be giving you errors that turn your tinderbox orange.
I don't think _that_ test has been marked failing.
(In reply to comment #99)
> I don't think _that_ test has been marked failing.

Right, I filed bug 448121 ;-)
No longer blocks: 448121
Depends on: 448121
Depends on: 448110
Depends on: 455308
Depends on: 455398
Depends on: 468473
Product: Core → Core Graveyard
Component: GFX → GFX: Thebes
Product: Core Graveyard → Core
Depends on: 497995
Whiteboard: [parity-safari] → [parity-webkit]
Depends on: 676603
You need to log in before you can comment on or make changes to this bug.