The latest revision of css3-background has made major changes to the spec for border-image. (See URL.) There is backward compatibility for all old pages except those that use the border-width override or query the value of what is now a shorthand property from JS. I've done a first-draft implementation of the changes. In cursory testing, the patch seems to work, but I have to note a whole bunch of caveats: - The patch does not fix up 66 mochitest and 3 reftest failures that appear to be caused by intentional changes to border-image's semantics (specifically, the disappearance of the border-width override mechanism). I will eventually fix this but I would love it if someone else did it for me - I have been having a very hard time concentrating on test writing these days. - Similarly, there are no tests of the new functionality. - There *may* be a bug where an outset border-image does not invalidate enough of the OS window. I wasn't able to get a bad paint but that doesn't mean it can't happen. - I tweaked the CSS syntax a bit; I intend to propose the changes for adoption into the spec. The changes are as follows. The spec's value syntax for border-image-width is inherit | initial | from-image | [ <length> | <percentage> | <number>]{1,4} I used 'auto' instead of 'from-image' -- this is largely for implementation convenience, but I think it's consistent with other uses of 'auto'. The spec permits 'inherit' and 'initial' as the sole value of all four of the new longhand properties, but does not permit these keywords to be used for a single sub-value of the rectangle properties. I do (and similarly 'auto' for b-i-w). There's no other way to set 'inherit' for just one edge, since there are no super-longhand border-image-foo-{top,left,bottom,right} properties as is the case for most other rectangle props, so I think this is also desirable. 'border-image' (now a shorthand) takes the most general form border-image: url(foo.gif) a b c d / e f g h / i j k l keyword keyword ; where a through l are numeric. I permit an optional third slash between l and the first keyword. This introduces no more syntactic ambiguity and makes the notation easier to remember.

> does not permit [inherit and initial] to be used for a single sub-value of > the rectangle properties AFAIK no CSS property or shorthand allows mixing 'inherit' or 'initial' with any other values. Therefore we should not introduce that here. But 'auto' should be able to mix in with the other values. I'll fix that in the spec. Note that the spec currently allows the url(), keywords, and numeric parts to be rearranged; the order is not fixed.

(In reply to comment #1) > > does not permit [inherit and initial] to be used for a single sub-value of > > the rectangle properties > > AFAIK no CSS property or shorthand allows mixing 'inherit' or 'initial' with > any other values. Therefore we should not introduce that here. As I said, I think this is necessary, because there are no super-longhand properties that set just one side, so there is no other way to apply inherit or initial to just one side of one of these rectangle properties. > But 'auto' should be able to mix in with the other values. I'll fix that in the > spec. Does that mean you concur with the substitution of 'auto' for 'from-image'? > Note that the spec currently allows the url(), keywords, and numeric parts to > be rearranged; the order is not fixed. Ugh, really? It's going to be confusing for authors and harder to implement like that.

Addendum: as implemented you can *omit* pieces that you don't need (everything but the source and slice components) but the order is fixed.

(In reply to comment #1) > AFAIK no CSS property or shorthand allows mixing 'inherit' or 'initial' with > any other values. Therefore we should not introduce that here. Agreed.

Comment on attachment 383016 [details] [diff] [review] rev 1: first draft patch for new border-image You need to fix the issues in comment 1. My understanding of the new rules for border-image is that they can now overflow the border box. This means that to get them to paint and repaint correctly you need to: * add to the frame's overflow area during reflow * add to the bounds of the display item review- because of the issues in comment 1 and because of this However, a few notes from going through the rest of the patch somewhat quickly (and not really looking at the rendering code at all yet): dom/interfaces/css/nsIDOMNSCSS2Properties.idl please add new members to the end of the interface. (Though maybe we should give up on that, sort it, and document that it should never be used from C++, which is fine, since it's just syntactic sugar for another interface.) + if (! ParseNonNegativeVariant(result.*(nsCSSRect::sides[index]), local style is no space after ! + if (count == 0 || (!aFromShorthand && ExpectEndProperty() == PR_FALSE)) { Use !ExpectEndProperty() rather than comparison to PR_FALSE. >+ >+ > #define VARIANT_CONTENT (VARIANT_STRING | VARIANT_URL | VARIANT_COUNTER | VARIANT_ATTR | \ > VARIANT_KEYWORD) No need for the extra whitespace. +CSS_PROP_SHORTHAND(-moz-border-image, border_image, MozBorderImage, CSS_PROPERTY_APPLIES_TO_FIRST_LETTER) the APPLIES_TO_FIRST_LETTER only makes sense for longhands; this should just have 0 for the flags. (Maybe that should be documented in nsCSSPropList.h for, actually, all of the current flag values.) Please don't make APPEND_OR_FAIL a macro. You can actually turn your first three uses of it into a static function containing the loop outside of it, and then put the last two into a loop. I really want to encourage making the code size in nsComputedDOMStyle smaller, and allowing macros there isn't going to help. - COMPUTED_STYLE_MAP_ENTRY(border_image, BorderImage), Please comment this out with //// like other shorthands. +#define SETCOORD_INITIAL_FULL 0x4000 How about HUNDRED_PCT instead of FULL? I'm not sure if we can remove aReflowOnLoad -- it seems like border images can still cause overflow, which requires reflow.

(In reply to comment #2) > > Note that the spec currently allows the url(), keywords, and numeric parts to > > be rearranged; the order is not fixed. > > Ugh, really? It's going to be confusing for authors and harder to implement > like that. The parser has a function called ParseChoice that's good at dealing with properties with this type of syntax.

Were you planning to address the review comments here?

(In reply to comment #7) > Were you planning to address the review comments here? Eventually? There's a couple dozen other things that people want me to do first.

How's this coming along? It's been a while since this bug was last active.

I do not anticipate getting back around to this in the near future. Would you like to help?

Sorry, but I can't. I'm merely a web designer. :(

You could still write tests for the new spec. A comprehensive set of tests would make my task (writing the code) enormously easier and therefore speed things up quite a bit. Please use the "reftest" format -- pairs of HTML files which, if everything is working correctly, will be drawn exactly the same. One of each pair uses the feature being tested, the other doesn't.

I've uploaded a testcase that uses the examples found in the spec. It uses two versions: the shorthand property and the individual properties. I hope that helps.

Thanks, but that's not a _comprehensive_ set of tests, nor is it structured as reftests.

Ah, I see. Sorry, but I'm unfamiliar with the way that things work here. I was reading the guide on the Developer Wiki about reftests and it seemed a bit too confusing for me. I won't be able to make a reftest for you.

Try this bit of documentation: http://wiki.csswg.org/test/reftest If it's still confusing, ask me questions. :) Even if you don't write a test, at least I'll get to improve the documentation for the next person.

The guide is very helpful, but I'm having trouble with the reference file. I can't figure out what it would look like or how I would make it. I can't picture in my head how the code would output. I'm not usually one to give up, but I really can't do this. I'm sorry. Regarding the documentation, have you considered using XHTML5? It would be a lot simpler.

Each reference file should look exactly -- pixel-for-pixel -- identical to its corresponding test file, but avoid using the feature that you're testing. Those are the only requirements. Here's a concrete example: suppose you have a 3x3 PNG named "box.png" whose edge pixels are 100% black, and whose center pixel is 100% transparent. Then this is a simple reftest for border-image: <!doctype html> <style> #test { width: 20px; height: 20px; border-width: 10px; border-image: url("box.png") 1 1 1 1 stretch stretch; } </style> <div id="test"></div> and its reference: <!doctype html> <style> #test { width: 20px; height: 20px; border: 10px solid black; } </style> <div id="test"></div>

Input and Jinghua has been reports from users about these issues. http://input.mozilla.com/en-US/search/?q=css3+image&product=firefox

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

(In reply to comment #5) > My understanding of the new rules for border-image is that they can now > overflow the border box. This means that to get them to paint and repaint > correctly you need to: > * add to the frame's overflow area during reflow In nsIFrame::FinishAndStoreOverflow > * add to the bounds of the display item in nsDisplayBorder::GetBounds William points out this is insufficient; I think the problem may be that nsIFrame::CheckInvalidateSizeChange needs to change its border invalidation code as well.

I have taken the old patch and updated it. A lot has changed in the past two years so the patch looks very different now. I have addressed all the comments in the bug which includes fixes to the parser so that properties can appear in any order in the shorthand form, "inherit" can not be mixed. I have also added support for the "fill" keyword in border-image-slice. The code now passes all reftests and mochitests. It is important to note that this patch will break existing behavior of border-image. In particular: - border-image border-width override mechanism is gone (as mentioned earlier). In fact, the values will now be interpreted as border-image-width values (not the same behavior). - Introduction of the "fill" keyword. After the image is sliced, the middle portion of the image is to be discarded unless the "fill" keyword is present. The current behavior is to not discard the middle. There is one addition to the spec which has not been implemented, the "space" keyword for border-image-repeat. It seems like implementation will be similar to background-repeat (https://bugzilla.mozilla.org/show_bug.cgi?id=548372). I think it makes sense to implement this in another bug.

Updated syntax -moz-border-image in mozilla-central to preserve current behavior.

Updated all reftest and mochitests for new -moz-border-image behavior. Removed tests of border-width override mechanism which no longer exists. Added new tests for CSS3 properties of the border-image. Added reftests for invalidating overflow due to border-image-outset.

(In reply to comment #24) > - border-image border-width override mechanism is gone (as mentioned > earlier). In fact, the values will now be interpreted as border-image-width > values (not the same behavior). In what way is this not the same behavior?

The old behavior changes the computed border-width, and thus layout. The new behavior only changes the border-image painting area.

Comment on attachment 548387 [details] [diff] [review] border-image revision 1 Declaration.cpp: Maybe this should be better about serializing to shorter forms when possible? In other words, there's a principle in CSS serialization (suggested by DOM Level 2 Style) that it's best to output the shortest form possible. This should probably be a FOLLOWUP BUG. The serialization code for the 'border' shorthand needs to check that border-image-* are initial values (and -moz-border-*-colors). This is an existing bug, so best handled in a FOLLOWUP BUG. nsCSSPropList.h: Don't prefix the keyword table entries with "nsCSSProps::". nsCSSParser.cpp: If you're putting tw in the modeline, make it 78 rather than 80. Typo fix: "dimentions" -> "dimensions" (multiple times) In ParseGroupedBoxProperty: >+ aValue.Reset(); >+ nsCSSRect& result = aValue.SetRectValue(); just skip the Reset; SetRectValue does Reset inside of it. In ParseBorderImageSlice: >+ borderImageSlice = borderImageSlice->mNext; >+ borderImageSlice->mValue = imageSliceFillValue; Just do "borderImageSlice->mNext->mValue = imageSliceFillValue;" and don't mutate borderImageSlice. In ParseBorderImage: >+ // No empty property. >+ if(CheckEndProperty()) { >+ return PR_FALSE; >+ } If you change the while() loop below into a do-while loop, you won't need this. (Also, if you did, you'd want a space after "if".) >+ // If parsing of <border-image-source> consumed tokens and returned false, >+ // then there was an error while parsing. We check to see if tokens were >+ // consumed by comparing the next token before and after trying to parse. >+ // It is assumed that the token type and identifier is sufficient to >+ // differentiate between the tokens used for <border-image-*> properties. >+ // If the tokens are different, then tokens were consumed. If tokens were >+ // consumed and next token is the same by coincidence, then it is still OK >+ // because all the remaining parsing methods should fail trying to parse it. >+ if (firstTokenType != mToken.mType || firstTokenIdent != mToken.mIdent) { You should assume this will not happen. ParseVariant will only consume tokens when it returns true. >+ // If parsing of <border-image-{slice/width/outset}> consumed token and >+ // returned false, then there was an error while parsing. >+ if (firstTokenType != mToken.mType || firstTokenIdent != mToken.mIdent) { >+ return PR_FALSE; >+ } Likewise, this will never happen. Thus you should also remove this code above: >+ GetToken(PR_TRUE); >+ nsCSSTokenType firstTokenType = mToken.mType; >+ nsAutoString firstTokenIdent = mToken.mIdent; >+ UngetToken(); nsComputedDOMStyle.cpp: Again, use tw=78 rather than tw=80. (Though I wrap code at 78 and comments at 72.) Throughout, where you have: >+ nsROCSSPrimitiveValue* val; declare these as late as you can rather than at the top of the function. In many cases, this means having one instance inside each loop. In DoGetBorderImageSlice, instead of: >+ val->SetIdent(nsCSSProps::ValueToKeywordEnum(border->mBorderImageFill, >+ nsCSSProps::kBorderImageSliceKTable)); you should just do: + val->SetIdent(eCSSKeyword_fill) DoGetBorderImageWidth seems pretty broken -- any of the four values can be auto, so you shouldn't just produce 'auto' if top happens to be 'auto'. It basically needs to be rewritten (and be more like the others). nsRuleNode.cpp: Again, use tw=78 rather than tw=80. (Though I wrap code at 78 and comments at 72.) In a FOLLOWUP PATCH we should fix the numerous tests of eCSSUnit_Initial in SetCoord to coalesce them into a single test of eCSSUnit_Initial containing tests for all the various flags. Could you pad all the constants that you're modifying in the chunk at the top out to 8 digits (i.e., starting from SETCOORD_INITIAL_HUNDRED_PCT) >+ nsStyleCoord parentCoord = parentBorder->mBorderImageSlice.Get(side); >+ nsCSSValue val = borderImageSlice.*(nsCSSRect::sides[side]); Don't do these extra copies. Best fix is to just pass the expressions directly to SetCoord and skip |val| and |parentCoord|. nsStyleStruct.h: >- // mBorder holds the nscoord values for the border widths as they would be if >- // all the border-style values were visible (not hidden or none). This >- // member exists so that when we create structs using the copy >- // constructor during style resolution the new structs will know what the >- // specified values of the border were in case they have more specific rules >- // setting the border style. Note that this isn't quite the CSS specified >- // value, since this has had the enumerated border widths converted to >- // lengths, and all lengths converted to twips. But it's not quite the >- // computed value either. The values are rounded to the nearest device pixel >- // We also use these values when we have a loaded border-image that >- // does not have width overrides. >+ // mBorder holds the nscoord values for the border widths as they >+ // would be if all the border-style values were visible (not hidden >+ // or none). This member exists so that when we create structs >+ // using the copy constructor during style resolution the new >+ // structs will know what the specified values of the border were in >+ // case they have more specific rules setting the border style. >+ // These numbers are also used when computing the border image area. >+ // >+ // Note that this isn't quite the CSS specified value, since this >+ // has had the enumerated border widths converted to lengths, and >+ // all lengths converted to twips. But it's not quite the computed >+ // value either. The values are rounded to the nearest device pixel. The removal of "We also use these values when we have a loaded border-image that does not have width overrides." is good. However, the addition of "These numbers are also used when computing the border image area." seems like a sign that the code is incorrect. Based on my reading of the spec, we should be using mComputedBorder, not mBorder. This needs to be fixed. You should also have a test for this (i.e., that the border-style on a side being hidden hides that part of the border image (by making its computed border-image-width 0) when border-image-width on that side is 1... but that it doesn't for length or auto values of border-image-width). >- // mComputedBorder holds the CSS2.1 computed border-width values. In >- // particular, these widths take into account the border-style for the >- // relevant side and the values are rounded to the nearest device >- // pixel. They are also rounded (which is not part of the definition >- // of computed values). However, they do *not* take into account the >- // presence of border-image. See GetActualBorder above for how to >- // really get the actual border. >+ // mComputedBorder holds the CSS2.1 computed border-width values. >+ // In particular, these widths take into account the border-style >+ // for the relevant side, and the values are rounded to the nearest >+ // device pixel (which is not part of the definition of computed values). I think you should add a note, explicitly, that the presence or absence of a border-image has no effect on the widths. Please rename nsStyleBorder::GetOutset to GetImageOutset. nsStyleStruct.cpp: In the nsStyleBorder copy-constructor, please explicitly initialize mImageTracked to false, #ifdef DEBUG, like it's initialized in the non-copy constructor. (It should be false... i.e., it should NOT be copying.) Likewise, nsStyleBorder::GetOutset should just access mComputedBorder rather than using a borderWidths variable. Also, nsStyleBorder::GetOutset should not have a eStyleUnit_Null case; that shouldn't happen and should just hit the default: case (with assertion), which should remain. Also, in nsStyleBorder::GetOutset, no space after NS_FOR_CSS_SIDES. In nsStyleBorder::GetOutset, could you also add a comment noting that we don't check whether we have an image (which is ok since the initial value yields 0 outset) so that we don't have to reflow to update overflow areas when an image loads. Based on my reading of the spec, GetActualBorder() is basically no longer needed. You should, for now, make it an inline method that just returns mComputedBorder. You need tests to cover this as well. A FOLLOWUP PATCH should remove the method. nsStyleBorder::CalcDifference needs to return NS_STYLE_HINT_REFLOW when mBorderImageOutset changes. nsCSSRendering.cpp: > PRInt32 imgDimension = ((s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM) > ? imageSize.height > : imageSize.width); >+ nscoord borderDimension = ((s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM) >+ ? borderImgArea.height >+ : borderImgArea.width); Please use NS_SIDE_IS_VERTICAL for both of these (which also means switching the ?:), and remove the extra () around both. Don't add an eStyleUnit_Null case for the border-image-slice handling. However, leave the change to an NS_NOTREACHED; it should simply never be null. (If it is, there's something else wrong.) Same for the border-image-width handling. >+ // "If two opposite border-image-width offsets are large enough that they >+ // overlap, their used values are proportionately reduced until they no >+ // longer overlap." >+ if (border.left + border.right > borderImgArea.width) { >+ double scale = borderImgArea.width / double(border.left + border.right); >+ border.left *= scale; >+ border.right *= scale; >+ NS_ASSERTION(border.left + border.right <= borderImgArea.width, >+ "rounding error in width reduction???"); >+ } >+ if (border.top + border.bottom > borderImgArea.height) { >+ double scale = borderImgArea.height / double(border.top + border.bottom); >+ border.top *= scale; >+ border.bottom *= scale; >+ NS_ASSERTION(border.top + border.bottom <= borderImgArea.height, >+ "rounding error in width reduction???"); >+ } This is incorrect given that the spec says: # If two opposite ‘border-image-width’ offsets are large enough # that they overlap, then the used values of all # ‘border-image-width’ offsets are proportionally reduced until # they no longer overlap. In mathematical notation: Given Lwidth as # the width of the border image area, Lheight as its height, and # Wside as the border image width offset for the side side, let f = # min(Lwidth/(Wleft+Wright), Lheight/(Wtop+Wbottom)). If f < 1, # then all W are reduced by multiplying them by f. This means that there should be a single |scale|, not two different ones for horizontal and vertical. >+ // Discard the middle porition unless set to fill. s/porition/portion/ nsPresContext.cpp: As I noted above in comments on nsStyleBorder::GetOutset and if you make the changes I suggest below in nsFrame.cpp, you don't currently need this ACTION_REFLOW_ON_LOAD code at all. If GetOutset actually checked if the image was loaded, you'd need it, but I think it's better this way, since it's faster for a normal case (images normally take some time to load) but it slightly penalizes a silly case (authors setting border-image-outset to non-default values when they don't have a border-image at all or when their border-image doesn't load). nsFrame.cpp: In FinishAndStoreOverflow, as I said above for nsPresContext.cpp, don't bother checking for GetBorderImage() -- just add the outset to the visual overflow whether or not there's a border image. I initially wrote the following comments, but you should ignore them (however, they help to explain my next comment): > In CheckInvalidateSizeChange, you should make two changes: > 1. First, you need to invalidate the *union* of > OldVisualOverflowRect with the new extent of the border, i.e. > aNewDesiredSize outset by border->GetImageOutset(). See, for > example, the code just above dealing with effects. This matters > particularly for cases where the frame gets bigger. > (The later cases don't need to do this because the caller is > responsible for invalidating the difference between the two > border boxes.) > 2. You can return after you do that, since all of the later > invalidates would invalidate a smaller region. > > However, you're adding the outset incorrectly. Rather than inflating > the visual overflow, instead you should inflate the frame's rect, and > union that with the visual overflow. > > >+ } else if (styleBorder->GetBorderImage()) { > >+ // If there is a border-image, then we need to invalidate the overflow > >+ // caused by border-image-outset. > >+ Invalidate(aOverflowAreas.VisualOverflow()); > >This comment isn't sufficient to explain why this invalidation is >needed. MORE HERE... Instead of doing that, you should *remove all* of the changes you've made to nsFrame.cpp and instead change the function ComputeOutlineAndEffectsRect, which affects both FinishAndStoreOverflow and CheckInvalidateSizeChange. This concept of border image outset is very similar to the effects of box-shadow, which is currently handled in ComputeOutlineAndEffectsRect. So you should handle it there. (But, when you do, still apply my first comment under the nsFrame.cpp heading above, and make sure to set *aAnyOutlineOrEffects to PR_TRUE.) r=dbaron with those things fixed -- but please fix only those things, or if you fix more, give detailed rationale for the change and detailed location of where the changed code is so I can review it without re-reviewing the whole patch (which, as you can probably tell from the length of these comments, took more than a day's work). Other things should happen in followup patches. Marking as review- since I want to look at the revisions before it lands. However, additionally, before this lands, we'll also want to get a patch to unprefix these properties. We can do this since the spec is in CR, as long as we contribute tests to the CSS working group simultaneously. I want the unprefixing to land in the SAME RELEASE as this patch. In order to unprefix, I'd also like to look through our test suite and make sure I'm happy with the coverage. (One gap that I noticed is that I don't think we have tests of the border-image slices overlapping, which should also be fixed in a FOLLOWUP BUG.) A few additional notes: This needs documentation of the new features, but also, perhaps most importantly, documentation of the removed feature, which is the width override, i.e., that any old border-image values that had a / in them now have a rather different meaning. (dev-doc-needed) We need to get a FOLLOWUP BUG filed on supporting 'border-image-repeat: space', which is also new, and which this patch does not add support for. One of us should make sure there's a bug filed for all of the "FOLLOWUP" items above (though some of them are pretty simple and could go as additional patches in this bug). None of them need to block landing this or unprefixing these properties, though, except for the one about being confident of our test coverage. And they shouldn't go in this bug after the patch lands.

If we're using those tests to unprefix, I'll need them in W3C format, i.e. in XHTML with appropriate metadata, etc. See http://wiki.csswg.org/test/css2.1/format The <meta http-equiv> lines are not needed.

I have addressed all your comments (thanks for the detailed review). The changes are in rev2to3.patch. (In reply to David Baron [:dbaron] from comment #29) > Comment on attachment 548387 [details] [diff] [review] > border-image revision 1 > > Declaration.cpp: > > Maybe this should be better about serializing to shorter forms when > possible? In other words, there's a principle in CSS serialization > (suggested by DOM Level 2 Style) that it's best to output the shortest > form possible. This should probably be a FOLLOWUP BUG. > > The serialization code for the 'border' shorthand needs to check that > border-image-* are initial values (and -moz-border-*-colors). This is > an existing bug, so best handled in a FOLLOWUP BUG. > Bug needs to be filed. > nsCSSParser.cpp: > In ParseBorderImage: > > >+ // No empty property. > >+ if(CheckEndProperty()) { > >+ return PR_FALSE; > >+ } > > If you change the while() loop below into a do-while loop, you won't > need this. (Also, if you did, you'd want a space after "if".) > I left it as is because converting the while loop into a do-while loop would require that I add a check inside the loop to ensure GetToken(PR_TRUE) succeeded. Also, I thought it was a bit more readable this way. > >+ // If parsing of <border-image-source> consumed tokens and returned > false, > >+ // then there was an error while parsing. We check to see if tokens > were > >+ // consumed by comparing the next token before and after trying to > parse. > >+ // It is assumed that the token type and identifier is sufficient to > >+ // differentiate between the tokens used for <border-image-*> > properties. > >+ // If the tokens are different, then tokens were consumed. If tokens > were > >+ // consumed and next token is the same by coincidence, then it is > still OK > >+ // because all the remaining parsing methods should fail trying to > parse it. > >+ if (firstTokenType != mToken.mType || firstTokenIdent != > mToken.mIdent) { > > You should assume this will not happen. ParseVariant will only > consume tokens when it returns true. > Done. > >+ // If parsing of <border-image-{slice/width/outset}> consumed token > and > >+ // returned false, then there was an error while parsing. > >+ if (firstTokenType != mToken.mType || firstTokenIdent != > mToken.mIdent) { > >+ return PR_FALSE; > >+ } > > Likewise, this will never happen. > Unlike the case above, this one can actually happen. Instead of ParseVariant, I am using ParseBorderImage* functions which can consume tokens and return false because each of those border-image-* properties can consist of many individual values. > Thus you should also remove this code above: > >+ GetToken(PR_TRUE); > >+ nsCSSTokenType firstTokenType = mToken.mType; > >+ nsAutoString firstTokenIdent = mToken.mIdent; > >+ UngetToken(); > > nsRuleNode.cpp: > In a FOLLOWUP PATCH we should fix the numerous tests of > eCSSUnit_Initial in SetCoord to coalesce them into a single test of > eCSSUnit_Initial containing tests for all the various flags. > Done. See initcleanup.patch > nsStyleStruct.h: > > >- // mBorder holds the nscoord values for the border widths as they > would be if > >- // all the border-style values were visible (not hidden or none). This > >- // member exists so that when we create structs using the copy > >- // constructor during style resolution the new structs will know what > the > >- // specified values of the border were in case they have more specific > rules > >- // setting the border style. Note that this isn't quite the CSS > specified > >- // value, since this has had the enumerated border widths converted to > >- // lengths, and all lengths converted to twips. But it's not quite the > >- // computed value either. The values are rounded to the nearest device > pixel > >- // We also use these values when we have a loaded border-image that > >- // does not have width overrides. > >+ // mBorder holds the nscoord values for the border widths as they > >+ // would be if all the border-style values were visible (not hidden > >+ // or none). This member exists so that when we create structs > >+ // using the copy constructor during style resolution the new > >+ // structs will know what the specified values of the border were in > >+ // case they have more specific rules setting the border style. > >+ // These numbers are also used when computing the border image area. > >+ // > >+ // Note that this isn't quite the CSS specified value, since this > >+ // has had the enumerated border widths converted to lengths, and > >+ // all lengths converted to twips. But it's not quite the computed > >+ // value either. The values are rounded to the nearest device pixel. > > The removal of "We also use these values when we have a > loaded border-image that does not have width overrides." is good. > > However, the addition of "These numbers are also used when computing > the border image area." seems like a sign that the code is incorrect. > Based on my reading of the spec, we should be using mComputedBorder, > not mBorder. This needs to be fixed. > You are right, it was not using the computed border when it should. This is fixed by changing GetActualBorder to return the computed border. > You should also have a test for this (i.e., that the border-style on a > side being hidden hides that part of the border image (by making its > computed border-image-width 0) when border-image-width on that side is > 1... but that it doesn't for length or auto values of > border-image-width). > Done. See newtests.patch > nsStyleStruct.cpp: > Based on my reading of the spec, GetActualBorder() is basically no > longer needed. You should, for now, make it an inline method that > just returns mComputedBorder. You need tests to cover this as well. > A FOLLOWUP PATCH should remove the method. > Partially done. Need followup bug to remove GetActualBorder(). > nsPresContext.cpp: > > As I noted above in comments on nsStyleBorder::GetOutset and if you > make the changes I suggest below in nsFrame.cpp, you don't currently > need this ACTION_REFLOW_ON_LOAD code at all. If GetOutset actually > checked if the image was loaded, you'd need it, but I think it's > better this way, since it's faster for a normal case (images normally > take some time to load) but it slightly penalizes a silly case > (authors setting border-image-outset to non-default values when they > don't have a border-image at all or when their border-image doesn't > load). > Done. Apparently border-image was the only feature that used ACTION_REFLOW_ON_LOAD so I also removed code that handled it (See nsImageLoader.h and nsImageLoader.cpp) > However, additionally, before this lands, we'll also want to get a patch > to unprefix these properties. We can do this since the spec is in CR, > as long as we contribute tests to the CSS working group simultaneously. > I want the unprefixing to land in the SAME RELEASE as this patch. In > order to unprefix, I'd also like to look through our test suite and make > sure I'm happy with the coverage. (One gap that I noticed is that I > don't think we have tests of the border-image slices overlapping, which > should also be fixed in a FOLLOWUP BUG.) > Did you want to unprefix in this bug or in a followup bug? I've also had to update the refttests again because we changed to use the computed border instead of the specified border thus I had to add border-styles so that border-image would be visible when the border-image-width is a number.

(In reply to William Chen [:william] from comment #31) > > >+ // If parsing of <border-image-{slice/width/outset}> consumed token > > and > > >+ // returned false, then there was an error while parsing. > > >+ if (firstTokenType != mToken.mType || firstTokenIdent != > > mToken.mIdent) { > > >+ return PR_FALSE; > > >+ } > > > > Likewise, this will never happen. > > > Unlike the case above, this one can actually happen. Instead of ParseVariant, > I am using ParseBorderImage* functions which can consume tokens > and return false because each of those border-image-* properties can consist > of > many individual values. Which of them can consume tokens and then return false, and how?

Comment on attachment 554133 [details] [diff] [review] border-image revision 2 nsCSSRendering.cpp: >+ PRInt32 imgDimension = NS_SIDE_IS_VERTICAL(s) ? >+ imageSize.width : imageSize.height; >+ nscoord borderDimension = NS_SIDE_IS_VERTICAL(s) ? >+ borderImgArea.width : borderImgArea.height; Please put the ? at the start of the second line rather than the end of the first. >+ double scaleX = border.left + border.right > borderImgArea.width ? >+ borderImgArea.width / double(border.left + border.right) : 1.0; >+ double scaleY = border.top + border.bottom > borderImgArea.height ? >+ borderImgArea.height / double(border.top + border.bottom) : 1.0; Use three lines for these so that you (a) don't go past 80 chars and (b) put the ? (and :) at the start of lines. nsImageLoader.*: Please remove ACTION_REFLOW_ON_DECODE as well (always unused). nsFrame.cpp: In your comment, the word "invalidate" should be "include" since the function you're (now) modifying covers more than invalidation. You missed this comment from before: >In FinishAndStoreOverflow, as I said above for nsPresContext.cpp, >don't bother checking for GetBorderImage() -- just add the outset to >the visual overflow whether or not there's a border image. That said, my rationale the last time around was a little off. Given that I got confused, I think it would be helpful to include a comment explaining: (1) that it's important we not check whether there's a border-image since the style hint for a change in border image doesn't cause reflow, and that's probably more important than optimizing the overflow areas for the silly case of border-image-outset without border-image (2) that it's important that we not check whether the border-image is actually loaded, since that would require us to reflow when the image loads. However, since you don't want to set *aAnyOutlineOrEffects to true for all cases, you do want to check for the outset itself being nonzero. nsCSSParser.cpp: I think you're wrong about the consuming tokens thing (comment 38) and should still fix that. If you're right about the current situation that it's needed, I think you need to fix it to not be needed, and then remove the two chunks code in question (the last two mentioned for nsCSSParser.cpp in my previous review). nsStyleStruct.cpp: >+ // reflow update overflow areas when an image loads. "reflow update" -> "reflow to update" nsStyleStruct.h: Your new GetActualBorder() should return |const nsMargin&| like the old one rather than returning |nsMargin|. Also, you don't need the keyword |inline| explicitly since the method is inline within the class definition. r=dbaron with those things fixed I'm hoping the "You should also have a test for this" from comment 29 is in one of the later patches. Also, please both request review on and land the split patches rather than the merged patches.

Comment on attachment 554139 [details] [diff] [review] Cleanup of nsCoord initial values r=dbaron

Comment on attachment 554137 [details] [diff] [review] Update of existing -moz-border-image usage r=dbaron

Comment on attachment 554140 [details] [diff] [review] Tests for border-image using computed border width r=dbaron

Comment on attachment 554136 [details] [diff] [review] Tests for border-image You should keep the pixel-rounding tests; you can remove the half within them that use border-width overrides, though, and on the other half you'll need the border-image-width: auto (within the shorthand is fine). Don't forget to add them back to the reftest.list. border-image-outset-{move,resize}-1 need to use MozReftestInvalidate events rather than setTimeout; you need to ensure the move you're doing is after the initial paint. (See layout/tools/reftest/README.txt .) r=dbaron (based on a somewhat cursory look) with those things fixed

Comment on attachment 554132 [details] [diff] [review] border-image revision 2 including all patches Clearing review request on the combined patch (ugh -- no need to ever post such a thing) after reviewing the individual patches.

Talked to William and David, and I'm going to finish this off and get it in the tree.

(In reply to David Baron [:dbaron] from comment #38) > Which of them can consume tokens and then return false, and how? khuey says ParseBorderImageSlice can

(In reply to David Baron [:dbaron] from comment #46) > (In reply to David Baron [:dbaron] from comment #38) > > Which of them can consume tokens and then return false, and how? > > khuey says ParseBorderImageSlice can In particular, the failure mode here is that we are parsing a something with two required parts that can appear in either order. We can consume the token for the first, and then fail to parse the second. Does this need some sort of special handling? I'm not familiar enough with the CSS parser to know if we need to push back the tokens for the first part or not.

dbaron, can you give the parser bits one last review please?

Comment on attachment 576304 [details] [diff] [review] William's patch unbitrotted with minor changes In nsImageLoader, implicit in my "please remove" was to also remove the now unused DoReflow function. I should have made that explicit the last time around. And also make the bits 1 and 2 rather than 2 and 4. nsCSSParser.cpp: Instead of writing: if (!foundSliceWidthOutset &&· ParseBorderImageSlice(false, &sliceConsumedTokens)) { // big chunk here } if (sliceConsumedTokens && !foundSliceWidthOutset) { return false; } could you just split the first if at the && and write: if (!foundSliceWidthOutset) { if (ParseBorderImageSlice(false, &sliceConsumedTokens)) { // big chunk here } else { if (sliceConsumedTokens) { return false; } } } r=dbaron with that

But in the future please include a commit message in patches posted for review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda1146d1ec3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebbf5b5d278 https://hg.mozilla.org/integration/mozilla-inbound/rev/300c3cdf3e3c https://hg.mozilla.org/integration/mozilla-inbound/rev/129f4bb19d97 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2cb7624152

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #51) > https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2cb7624152 I guess you merged the patch attached to this bug manually, rather than removing the trailing "browser/" from the paths before applying it. In doing so, you seem to have removed a line break in browser/themes/gnomestripe/browser.css. Please fix this.

You mean the 2-declarations-on-one-line here? 2.12 - -moz-border-image: url("chrome://browser/skin/urlbar-arrow.png") 0 8 0 0 / 0 8px 0 0; 2.13 - -moz-margin-end: -8px; 2.14 + border-width: 0 8px 0 0; 2.15 + border-style: solid; 2.16 + -moz-border-image: url("chrome://browser/skin/urlbar-arrow.png") 0 8 0 0 fill; -moz-margin-end: -8px;

(In reply to David Baron [:dbaron] from comment #53) > You mean the 2-declarations-on-one-line here? > > 2.12 - -moz-border-image: url("chrome://browser/skin/urlbar-arrow.png") > 0 8 0 0 / 0 8px 0 0; > 2.13 - -moz-margin-end: -8px; > 2.14 + border-width: 0 8px 0 0; > 2.15 + border-style: solid; > 2.16 + -moz-border-image: url("chrome://browser/skin/urlbar-arrow.png") > 0 8 0 0 fill; -moz-margin-end: -8px; Yep.

Woops, my bad. I'll fix it next time the tree is open.

We should also try to get this unprefixed in the same release this landed in.

And also the other things you'll find as "FOLLOWUP" above.

https://hg.mozilla.org/mozilla-central/rev/dda1146d1ec3 https://hg.mozilla.org/mozilla-central/rev/8ebbf5b5d278 https://hg.mozilla.org/mozilla-central/rev/300c3cdf3e3c https://hg.mozilla.org/mozilla-central/rev/129f4bb19d97 https://hg.mozilla.org/mozilla-central/rev/0d2cb7624152

I filed bug 713643 on the followup issues.

-moz-border-image is now broken: see example at http://www.w3schools.com/cssref/tryit_view.asp?filename=trycss3_border-image - doesn't work in 27-Jan-2012 nightly build. Used to work just couple of weeks ago.

I suspect this test became invalid with the new draft. The test work if : "border-width:15px;" is replace by "border:15px solid;" “The border can either be a predefined style (solid line, double line, dotted line, pseudo-3D border, etc.) or it can be an image.” No border-style = no border.

I see, thank you. However there is also an issue with javascript: getComputedStyle(element).getPropertyCSSValue("-moz-border-image") now brings null (again, used to bring meaningful value). Is this a bug or the image is now referenced by another property?

Backed out of Aurora in http://hg.mozilla.org/releases/mozilla-aurora/rev/c81d62b02d19

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #65) > Backed out of Aurora in > http://hg.mozilla.org/releases/mozilla-aurora/rev/c81d62b02d19 I just want to make sure I understanding correctly. Does this mean Bug 497995 will not re-land on Fx 12 builds after the move to the Beta channel? I'm assuming that Bug 497995 will only remain present on Fx 13 builds, Is that correct?

That is correct.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #65) > Backed out of Aurora in > http://hg.mozilla.org/releases/mozilla-aurora/rev/c81d62b02d19 Kyle, Since all the issues haven't yet been resolved in Bug 713643 with there only being 5 days left until Aurora Fx 13 moves to Beta Channel. Are there plans for Bug 497995 to be backed out of Aurora Fx 13?

Is this code present in FF13? This was backed out of Aurora while FF12 was there, but it's not clear that this has occurred for FF13. The target milestone is a bit misleading in that case. Can you clarify?

This has been backed out of 13.

Nominating for kilimanjaro with the same rationale shown in https://bugzilla.mozilla.org/show_bug.cgi?id=713643#c19. I'm a bit concerned about the implementation of this bug. We're already seeing evidence that the requirement for border-style: solid with border-image is breaking top sites (Lord of Ultima, Command and Conquer Tiberium Alliances, Mobile Facebook). We're going to be forced to heavily evangelize this if we go forward with that requirement, as top sites functionality is greatly affected by border-style: solid becoming enforced. Do we have mitigation strategy around this issue?

I don't see why you want to nominate *this* bug.

(In reply to David Baron [:dbaron] from comment #72) > I don't see why you want to nominate *this* bug. Fair enough. I'll track the concerns and kilimanjaro reference in the followup bug (bug 713643).

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #70) > This has been backed out of 13. Shouldn't the status-firefox14 be updated now that this has been backed out by bug 713643 as well?

Yes, thanks for catching that.

Updated: https://developer.mozilla.org/en-US/docs/CSS/border-image https://developer.mozilla.org/en-US/docs/CSS/border-image-outset https://developer.mozilla.org/en-US/docs/CSS/border-image-repeat https://developer.mozilla.org/en-US/docs/CSS/border-image-slice https://developer.mozilla.org/en-US/docs/CSS/border-image-source https://developer.mozilla.org/en-US/docs/CSS/border-image-width https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers