921341 - Incorrect background-color bleeds on cell corners using border-radius

Status: NEW

(Core :: Layout: Tables, defect)

Description

[Barry Carlson]

Attachment: table-cell-corner-bleed.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

Set TABLE background-color, border-collapse:separate and border-spacing:>0.
Set ROW background-color, then target one CELL in ROW and give it a new background-color plus a border-radius:>0.

  


Actual results:

The exposed corners of the CELL with the new background-color inherits the ROW background-color instead of the TABLE background color.


Expected results:

With TABLE border-collapse:separate, border-spacing:>0 and the CELL border-radius:>0, the exposed CELL corner should inherit the TABLE background-color; not the ROW background-color (if declared). 

This rendering evident from Firefox 4.0.

IE9+, Opera, Chrome and Safari comply with above.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This should be fixed by:

(1) adjusting the function TableBackgroundPainter::PaintCell (in layout/tables/nsTablePainter.cpp) to use the border-radius of the cell for all the backgrounds painted

(2) adding a reftest (and maybe also a reftest for bug 455668 while you're there)

Comment 2

[Tareq Khandaker]

1) I can't find the border-radius of the cell. I went through all of aCell's methods. The closest thing I found was https://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableCellFrame.h#301.

2) Where can I find out more about reftests? (How to make one etc.)

Thanks

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Tareq Khandaker from comment #2)
> 1) I can't find the border-radius of the cell. I went through all of aCell's
> methods. The closest thing I found was
> https://mxr.mozilla.org/mozilla-central/source/layout/tables/
> nsTableCellFrame.h#301.

See the code in nsCSSRendering::PaintBackgroundWithSC in layout/base/nsCSSRendering.cpp for examples.

> 2) Where can I find out more about reftests? (How to make one etc.)

http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt
https://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests

Comment 4

[Casper van Donderen (Telenor)]

I am looking at this bug and I do not understand why I need to look at the code in nsCSSRendering if that is already the code that gets called. Is the bug in there, or is it in nsTablePainter? Or am I misunderstanding why one should look in nsCSSRendering.cpp?
I tried using aCell->GetBorderRadii(radii) in nsTablePainter, which does return true for the rounded cell, but trying to disable background rendering in that case [if (!haveRoundedCorners) {}] will still render the black background.
Now I'm trying to understand if the black background is rendered beforehand and I need to overwrite it with white, or am I then looking in the wrong direction?

Comment 5

[Adam Casey]

I've started on this and made a fair amount of progress. The test case above now renders properly.

However I'm struggling with background-image's not being offset correctly when we're drawing the row / column background image.

It seems to me as though I should be using the "aBGClipRect" argument of PaintBackgroundWithSC to adjust the background render offset, however as soon as I pass anything into this argument (including the same as aBorderArea or an empty nsRect, the rounded corners stop rendering.

After some more looking into this, I think that there is some background / clip preparation which goes on in this if-statement: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2467

In the true block, clipState.mCustomClip is set to true. This disables rounded corners according to http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#1522 . However even if I comment out that line for testing, the problem remains.

GetBackgroundClip is called only in the else block of that if-statement. In GetBackgroundClip it does some more calculations (for example converting outer co-ordinates into inner co-ordinates.

I'm a bit lost as to what I can change / do to get around this here, if I start changing lots in nsCSSRendering I get the feeling that I'll break a lot of other parts.

Should I be looking for another way to offset the background to the correct place, or should I be trying to change PaintBackgroundWithSC to perform more of the same logic whether aBGClipRect is passed in or not?

Comment 6

[Adam Casey]

Attachment: Patch which lets the test case render correctly.

See comment 5 for more details, I've uploaded the patch just to show the way I got the test case to render as expected.

Comment 7

[Adam Casey]

Attachment: bug921341.PNG

Showing the problem with the previous patch. The top is IE11 rendering correctly, the bottom is Firefox with this patch.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Adam Casey from comment #5)
> It seems to me as though I should be using the "aBGClipRect" argument of
> PaintBackgroundWithSC to adjust the background render offset, however as
> soon as I pass anything into this argument (including the same as
> aBorderArea or an empty nsRect, the rounded corners stop rendering.

Based on:
http://dxr.mozilla.org/mozilla-central/search?q=function-ref%3APaintBackground
and then recurring into:
http://dxr.mozilla.org/mozilla-central/search?q=function-ref%3APaintBackground
etc.

it appears that the only callers that pass non-null aBGClipRect are:

TableBackgroundPainter::PaintCell
nsDisplayBackgroundImage::PaintInternal (only when called from nsDisplayCanvasBackgroundImage::Paint)

So you're *mostly* free to make the aBGClipRect behavior do what you need; the only cases that have a non-null aBGClipRect are the cases you're dealing with here, and painting of backgrounds on the canvas (that is, painting any background on the root or body element that's propagated to the canvas).  And I suspect (though I didn't check) that the canvas is never going to get painted with an nsStyleBorder that specifies a border-radius.  (It might be worth double-checking both of those things.)

So I think the patch mostly looks like the right direction, though it seems like it would probably be better to fix whatever does the wrong thing when mCustomClip is true rather than lie about mCustomClip.  (I'm assuming this is the only codepath that sets mCustomClip to true -- worth checking, though.)  This probably involves fixing the DrawBackgroundColor and SteupBackgroundClip functions.

> Should I be looking for another way to offset the background to the correct
> place, or should I be trying to change PaintBackgroundWithSC to perform more
> of the same logic whether aBGClipRect is passed in or not?

I'd presume the reason that you're seeing the wrong pieces of the row's background in the cells is because you've disabled the custom clip code that's intended to produce that result (by using a custom clip of the row's background for each cell).  I think the right thing is to keep using the custom clip code, but make it work with rounded borders.

Comment 9

[Adam Casey]

Attachment: bug921341.diff

Could someone push this to the tryserver with params: 
try: -b do -p all -u all[x64]


More progress on the bug, it correctly handles the background position for cells now. I haven't looked into how careful I need to be regarding the other instance of PaintBackgroundWithSC being called with a non-null aBGClipRect yet. I also had to edit some DrawBackgroundColor code due to the fact it specifically didn't perform some rounded corners stuff if we had rounded corners. According to a comment this was due to a TablePainter problem, so perhaps that is fixed now.

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Adam Casey from comment #9)
> Could someone push this to the tryserver with params: 
> try: -b do -p all -u all[x64]

https://tbpl.mozilla.org/?tree=Try&rev=3dc5997a1e5d

Comment 11

[Adam Casey]

Attachment: bug921341.diff

This patch should not cause any other tests to fail..

Includes proper reftest.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Adam Casey from comment #11)
> Created attachment 8371684 [details] [diff] [review]

https://tbpl.mozilla.org/?tree=Try&rev=e22d0c93cfda

Comment 13

[Adam Casey]

Attachment: bug921341.diff

This patch fixes the behaviour for the testcase above, and implements a reftest along similar lines.

It should pass all tests.

Seems like PaintBackgroundWithSC has a lot of code paths which have an unclear purpose, is there anywhere which stores documentation for this kind of thing or is it a case of just repeatedly prodding in different places?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Adam Casey from comment #13)
> Seems like PaintBackgroundWithSC has a lot of code paths which have an
> unclear purpose, is there anywhere which stores documentation for this kind
> of thing or is it a case of just repeatedly prodding in different places?

No documentation, but it ought to be possible to figure it out from the code.  If there are bits you aren't sure about, feel free to ask.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8372265 [details] [diff] [review]
bug921341.diff

># HG changeset patch
># Parent 1ca0ce406aad20597a7f22603611ff400b2242d4
>Bug 921341 - Rounded corners on table cells with row/col/group backgrounds now render correctly.

You're missing the "user" bit that attributes the message to you.  See
https://developer.mozilla.org/en-US/docs/Installing_Mercurial for some
configuration tips that might help (though if you're using mq, you'll
probably need to do an "hg qrefresh -U", since the suggested
configuration only adds -U to qnew, to avoid changing the attribution
for all patches to the current user).

>reftest requires fuzzy due to inconsistent rendering of rounded rectangle

Using SVG as a reference seems less than ideal; could you use a rounded
border/background that's not a table part instead?


Given that you've touched only DrawBackgroundColor (which is for
background color drawing) and not the equivalent check in
SetupBackgroundClip (which is used for background-image drawing), I
think you've fixed the bug only for background colors but not for
images.  Or at least it looks that way, since you've left in the code
for background-image handling that explicitly says it will ignore
rounded borders when there's a custom clip.  You should remove all of
that code, and test both (though it looks like you do; but see more
comments below).  And you should entirely remove mCustomClip, since it
should be unused once you fix both.

>+  // We only take this short cut if we don't have rounded corners now

So comments in the code should generally be about the code as it exists,
and not about your specific changes in the code.  (If there are comments
you're making as questions to the reviewer, to be removed before
landing, prefix them with "REVIEW:".)  I think the code is pretty
self-explanatory ("if we don't have rounded corners, just draw a
rectangle and return") so it doesn't seem to me to need a comment.

>+  // Removing means that some transforms aren't pixel snapped, is this a bug?

I'm not sure what you mean; could you explain?

>-  if (aBGClipRect) {
>+  if (aBGClipRect && !haveRoundedCorners) {

What's this for?  Or is this what happened to fix the bug for background
images as well, by taking the wrong codepath?  (If that's the intent,
it's extremely confusing.  It's a really bad idea to have code in one
function that says "if we have a custom clip rect, don't support rounded
corners", and then have code in the function that calls it that says "if
we have both a custom clip and rounded corners, lie about having a
custom clip to the code we're calling".)

>+    // Ensures we hit some code further down when we drawing a Table
>+    if(aBGClipRect) {
>+      clipState.mBGClipArea = *aBGClipRect;
>+      aBGClipRect = 0;
>+    }

Two comments on this:
 * doing something here in order to influence a condition further down
   sounds bad -- the code is clearer if you fix the condition where it
   is.  (It's even less clear given that you don't say why.)  But maybe
   once you fix the previous comment it won't be needed at all?
 * "if(" should be "if ("

>+  // Should this go below #2509-2514?

Comments related to line numbers, 2500 lines into a file that's changing
frequently, make very little sense.  I can't tell if you're referring to
line numbers pre-patch or post patch.

(We're trying to switch to a much less stateful graphics API called
Azure or Moz2D, which involves passing the color along with the drawing
calls rather than setting it as state -- to recognize that we very
rarely use the fact that this sort of state sticks around, and always
set it to what we want for each drawing call.)

>+  const nsStyleBorder* aCellBorder = aCell->StyleContext()->StyleBorder();

Two comments here:
 * the "a" prefix on variables is for function arguments
 * you can use aCell->StyleBorder() as a shorthand
so this should just be:
  const nsStyleBorder* cellBorder = aCell->StyleBorder();




Have you pushed this to try to run the full test suite, or do you need
somebody to do that for you?



This is good progress, but the patch needs further revision, so marking as review-.  (And sorry for the delay getting back to you.)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To respond to your IRC comments from #layout (sorry for missing them -- better to mention my name at the start of each line you want me to see):

>[2014-02-16 15:15:12 -0800] <adamncasey> Re bug 921341 (Thanks for your feedback btw): There are two (or perhaps more?) approaches to take with the PaintBackgroundWithSC changes.
>[2014-02-16 15:16:49 -0800] <adamncasey> When the function is called from TablePainter we want some of the adjustments / calculations which are currently in GetBackgroundClip which aren't wanted when called from nsCanvasFrame. These are ignored because we check whether aBgClipRect is passed in which groups these two callees together.

Are they *unneeded* for nsCanvasFrame, or are they *harmful*?  If they're merely unneeded, then it's preferable to just have one codepath, rather than to have extra code to optimize for a single frame in the frame tree.

>[2014-02-16 15:17:07 -0800] <adamncasey> I think what I'm stating is correct anyway, I might not be correct..
>[2014-02-16 15:19:19 -0800] <adamncasey> One method might be to keep the table painter call and the canvasframe call following roughly the same codepath (keep in the aBGClipRect check) and add the additional adjustments to that codepath if we have rounded borders, or if we are being called with a Table object.

I don't think you'd need to check that.  The nsCanvasFrame call involves nsDisplayCanvasBackgroundImage::Paint calling nsDisplayBackgroundImage::PaintInternal calling nsCSSRendering::PaintBackground calling nsCSSRendering::PaintBackgroundWithSC.  nsCSSRendering::PaintBackground is what handles the rules in http://www.w3.org/TR/CSS21/colors.html#background about propagating the background to the canvas, but it still uses the nsStyleBorder from the canvas frame -- which I believe *cannot* be styled by the page.  (You could add assertions to that effect in FindBackground or FindCanvasBackground if you want.)  So I don't think you need to worry about the canvas frame having rounded borders -- you should just be able to assume it doesn't happen -- and if it did, rounding them would be the right thing to do.


>[2014-02-16 15:20:06 -0800] <adamncasey> Or we could try and make the call from tablePainter somehow indicate that it wants the adjustments which nsCanvasFrame doesn't. But this will add another codepath which might be confusing because other Paint*WithSC functions won't have this option.
>[2014-02-16 15:21:00 -0800] <adamncasey> Let me know if I should email this to you so you can look at it later, sorry

If the adjustment is about the question of needing to ignore border styles -- I don't think two codepaths are needed here.

Comment 17

[Adam Casey]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #15)
> Comment on attachment 8372265 [details] [diff] [review]
> bug921341.diff
> 
> ># HG changeset patch
> ># Parent 1ca0ce406aad20597a7f22603611ff400b2242d4
> >Bug 921341 - Rounded corners on table cells with row/col/group backgrounds now render correctly.
> 
> You're missing the "user" bit that attributes the message to you.  See
> https://developer.mozilla.org/en-US/docs/Installing_Mercurial for some
> configuration tips that might help (though if you're using mq, you'll
> probably need to do an "hg qrefresh -U", since the suggested
> configuration only adds -U to qnew, to avoid changing the attribution
> for all patches to the current user).

Fixed, thank you
> 
> >reftest requires fuzzy due to inconsistent rendering of rounded rectangle
> 
> Using SVG as a reference seems less than ideal; could you use a rounded
> border/background that's not a table part instead?

Good point, not sure why I thought using SVG was best. This also seems to fix the fuzzy problem.

> 
> Given that you've touched only DrawBackgroundColor (which is for
> background color drawing) and not the equivalent check in
> SetupBackgroundClip (which is used for background-image drawing), I
> think you've fixed the bug only for background colors but not for
> images.  Or at least it looks that way, since you've left in the code
> for background-image handling that explicitly says it will ignore
> rounded borders when there's a custom clip.  You should remove all of
> that code, and test both (though it looks like you do; but see more
> comments below).  And you should entirely remove mCustomClip, since it
> should be unused once you fix both.

I had to change DrawBackgroundColor because it specifically didn't draw rounded corners.
Removing mCustomClip is a good idea, I'll do that.
I didn't intend on leaving in comments which stated it didn't support rounded corners, I'll double check I haven't left any of this in the next patch.

> >+  // We only take this short cut if we don't have rounded corners now
> 
> So comments in the code should generally be about the code as it exists,
> and not about your specific changes in the code.  (If there are comments
> you're making as questions to the reviewer, to be removed before
> landing, prefix them with "REVIEW:".)  I think the code is pretty
> self-explanatory ("if we don't have rounded corners, just draw a
> rectangle and return") so it doesn't seem to me to need a comment.

Ok, good to know

> >+  // Removing means that some transforms aren't pixel snapped, is this a bug?
> 
> I'm not sure what you mean; could you explain?

When I removed this section for testing I had an issue where drawing a transformed div wouldn't produce a pixel snapped square (layout/reftests/image/image-seam-1a.html). I thought this might cause problems when drawing rounded corners on a transformed div, but I realised it does some extra pixel snapping when we have rounded corners.

> >-  if (aBGClipRect) {
> >+  if (aBGClipRect && !haveRoundedCorners) {
> 
> What's this for?  Or is this what happened to fix the bug for background
> images as well, by taking the wrong codepath?  (If that's the intent,
> it's extremely confusing.  It's a really bad idea to have code in one
> function that says "if we have a custom clip rect, don't support rounded
> corners", and then have code in the function that calls it that says "if
> we have both a custom clip and rounded corners, lie about having a
> custom clip to the code we're calling".)

This was to get it to follow the right codepath for drawing background images, but you're right that it's not easy to understand. 


> >+    // Ensures we hit some code further down when we drawing a Table
> >+    if(aBGClipRect) {
> >+      clipState.mBGClipArea = *aBGClipRect;
> >+      aBGClipRect = 0;
> >+    }
> 
> Two comments on this:
>  * doing something here in order to influence a condition further down
>    sounds bad -- the code is clearer if you fix the condition where it
>    is.  (It's even less clear given that you don't say why.)  But maybe
>    once you fix the previous comment it won't be needed at all?
>  * "if(" should be "if ("

I'll find a better way to do this.

> >+  // Should this go below #2509-2514?
> 
> Comments related to line numbers, 2500 lines into a file that's changing
> frequently, make very little sense.  I can't tell if you're referring to
> line numbers pre-patch or post patch.
> 
> (We're trying to switch to a much less stateful graphics API called
> Azure or Moz2D, which involves passing the color along with the drawing
> calls rather than setting it as state -- to recognize that we very
> rarely use the fact that this sort of state sticks around, and always
> set it to what we want for each drawing call.)

> >+  const nsStyleBorder* aCellBorder = aCell->StyleContext()->StyleBorder();
> 
> Two comments here:
>  * the "a" prefix on variables is for function arguments
>  * you can use aCell->StyleBorder() as a shorthand
> so this should just be:
>   const nsStyleBorder* cellBorder = aCell->StyleBorder();
> 
Thanks.

> 
> 
> Have you pushed this to try to run the full test suite, or do you need
> somebody to do that for you?
> 
I generally ask on #introduction for someone to push to the tryserver for me, thank you. If this bug is resolved I'll probably apply for commit level 1.
> 
> This is good progress, but the patch needs further revision, so marking as
> review-.  (And sorry for the delay getting back to you.)

Re IRC Comments:

Yeah, from what I've tried they've seemed unneeded. I'm not sure how to go about finding out that this is 100% not going to break something, but I'll see what the try server kicks up.

I've removed the if (aBGClipRect) codepath and, with a test passed into GetBackgroundClip it fixes this bug, and makes the code slightly easier to understand I hope. I'll get the patch pushed to the tryserver just to double check that nothing I haven't spotted has gone wrong.

Comment 18

[Adam Casey]

Attachment: A clearer method to fix the bug.

This patch removes the if (aBGClipRect) codepath which (hopefully) makes PaintBackgroundWithSC easier to understand rather than harder like the last patch.

Could someone push this to the try server with:
try: -b o -p all -u all[x64,10.8,6.2] -t none

Thanks

Comment 19

[Manish Goregaokar [:manishearth]]

Pushed to try

https://tbpl.mozilla.org/?tree=Try&rev=fbdbbc672dcc

Comment 20

[Manish Goregaokar [:manishearth]]

Assigned to Adam as per request in IRC.

Comment 21

[Adam Casey]

Attachment: bug921341.diff

Trying a fuzzy reftest on all platforms to see if this keeps the reftest from failing.

Could this be run on the tryserver with:
try: -b o -p all -u reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc -t none

Comment 22

[Manish Goregaokar [:manishearth]]

(In reply to Adam Casey from comment #21)
> Created attachment 8377289 [details] [diff] [review]
> bug921341.diff
> 
> Trying a fuzzy reftest on all platforms to see if this keeps the reftest
> from failing.
> 
> Could this be run on the tryserver with:
> try: -b o -p all -u
> reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc -t none

If you only want to run reftests, why not

    try: -b o -e -p linux,linux64,linux64-st-an,linux64-valgrind,linux64-br-haz,macosx64,win32 -u reftest,reftest-ipc,reftest-no-accel -t none

?

This would skip all the mobile builds.

Comment 23

[Adam Casey]

Sure that would work.

I was just being (over?) cautious to prevent causing an test failure on a platform which I haven't tested it on if this gets checked-in in future.

Comment 24

[Manish Goregaokar [:manishearth]]

Enjoy!

https://tbpl.mozilla.org/?tree=Try&rev=25e88a7681c2

Comment 25

[Adam Casey]

Comment on attachment 8377289 [details] [diff] [review]
bug921341.diff

Patch passes tests.

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8377289 [details] [diff] [review]
bug921341.diff

Review of attachment 8377289 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +1491,5 @@
>                    uint8_t aBackgroundAttachment,
>                    nsIFrame* aForFrame, const nsRect& aBorderArea,
>                    const nsRect& aCallerDirtyRect, bool aHaveRoundedCorners,
>                    const gfxCornerSizes& aBGRadii, nscoord aAppUnitsPerPixel,
> +                  bool useBGClipArea, 

Nit: Trailing whitespace

@@ +2517,5 @@
>      NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT_WITH_RANGE(i, bg, bg->mImageCount - 1,
>                                                                nLayers + (bg->mImageCount -
>                                                                           startLayer - 1)) {
>        const nsStyleBackground::Layer &layer = bg->mLayers[i];
> +	  

Nit: Extra whitespace

@@ +2648,5 @@
>  
>    BackgroundClipState clipState;
>    GetBackgroundClip(ctx, currentBackgroundClip, bg->BottomLayer().mAttachment,
>                      aForFrame, aBorderArea,
> +                    aDirtyRect, haveRoundedCorners, bgRadii, appUnitsPerPixel, false,

Nit: Might as well put the next few arguments on a new line, instead of having it wrap around.

Comment 27

[Adam Casey]

Attachment: bug921341.diff

Fixed nits suggested by Manishearth

Comment 28

[Manish Goregaokar [:manishearth]]

Comment on attachment 8377587 [details] [diff] [review]
bug921341.diff

Review of attachment 8377587 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2516,5 @@
>      NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT_WITH_RANGE(i, bg, bg->mImageCount - 1,
>                                                                nLayers + (bg->mImageCount -
>                                                                           startLayer - 1)) {
>        const nsStyleBackground::Layer &layer = bg->mLayers[i];
> +  

Still some trailing space here

Comment 29

[Adam Casey]

Attachment: bug921341.diff

Whoops, now I've fixed the nits.

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8377590 [details] [diff] [review]
bug921341.diff

Review of attachment 8377590 [details] [diff] [review]:
-----------------------------------------------------------------

f+, but you forgot to include the other files in the patch.

Comment 31

[Adam Casey]

Attachment: bug921341.diff

The full .diff file. I'm not quite sure how I managed to only include the changes for one file.

Sorry for all the bugmail spam.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8377593 [details] [diff] [review]
bug921341.diff

>-
>-  // Whether we are being asked to draw with a caller provided background
>-  // clipping area. If this is true we also disable rounded corners.
>-  bool mCustomClip;
> };
> 
>+

No reason for a double-newline here, I think.

> static void
> GetBackgroundClip(gfxContext *aCtx, uint8_t aBackgroundClip,
>                   uint8_t aBackgroundAttachment,
>                   nsIFrame* aForFrame, const nsRect& aBorderArea,
>                   const nsRect& aCallerDirtyRect, bool aHaveRoundedCorners,
>                   const gfxCornerSizes& aBGRadii, nscoord aAppUnitsPerPixel,
>+                  bool useBGClipArea,
>                   /* out */ BackgroundClipState* aClipState)
> {
>-  aClipState->mBGClipArea = aBorderArea;
>+  //REVIEW: This is a bit like the old mCustomClip, however it is only needed here so I thought it was better
>+  //        to just add a new argument here then repurpose the mCustomClip and carry it around everywhere else.
>+  if (!useBGClipArea) {
>+    aClipState->mBGClipArea = aBorderArea;
>+  }

So this change seems weird for four reasons:
 * your useBGClipArea doesn't follow the prefixing convention ("aUse...")
 * it's a confusing boolean:  "use" really means "don't modify"
 * if the goal is don't modify, the it's not clear why you don't just change the value after calling the function
 * ... except that there are other places in the function that do still modify mBGClipArea, and we need to consider what the correct behavior in those cases is

In particular, the other cases that modify mBGClipArea are when:
 * aForFrame is a scroll frame with background-attachment:local (not relevant here)
 * the 'background-clip' property has a value other than 'border' (which I think probably is relevant here)

The first question, then, is which 'background-clip' property should matter when we're drawing the row background applied to the cell.  The current code considers the row's background-clip since it's the row that's passed as "aForFrame" from TableBackgroundPainter.

My weak intuition is that it makes more sense to use the cell's background-clip, but I didn't look at the spec (and I also suspect the spec doesn't say).

But that brings up another issue:  in the TablePainter.cpp code, should you be passing the row or the cell as aForFrame?  I think you need to go through the uses of aForFrame, see what they're used for, and see which usage would be correct.  For example, one use is for calculating the basis of percentage border-radius, and inthat case, it's clearly the cell that you want -- this patch doesn't handle percentage border-radius on the cell correctly.  Another use is for an nsFrame::AssociateImage call, which leads to invalidation when the image loads -- it's possible that also wants the cell, since it's not clear to me whether the way it invalidates the frame when the image loads would correctly invalidate a rowspanning cell that extends out of the bounds of its row.  etc.  (I'd almost have been ok with leaving the aForFrame business to another bug, except that it's used for percentage border-radius.  Sorry for not noticing that with my initial advice for how to proceed here.)

It's possible you want to simply pass the cell as aForFrame, but I think it's more likely that you want to split the aForFrame argument into two, one for the frame the background comes from (which is probably what you want to use for the IsThemed() check) and one for the frame in whose area the background is being drawn, and then figure out what the right thing to do for any other cases is.

Comment 33

[Adam Casey]

Attachment: Test case with background-clip and % border-radius.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #32)
> Comment on attachment 8377593 [details] [diff] [review]
> bug921341.diff
> 
> >-
> >-  // Whether we are being asked to draw with a caller provided background
> >-  // clipping area. If this is true we also disable rounded corners.
> >-  bool mCustomClip;
> > };
> > 
> >+
> 
> No reason for a double-newline here, I think.
> 
> > static void
> > GetBackgroundClip(gfxContext *aCtx, uint8_t aBackgroundClip,
> >                   uint8_t aBackgroundAttachment,
> >                   nsIFrame* aForFrame, const nsRect& aBorderArea,
> >                   const nsRect& aCallerDirtyRect, bool aHaveRoundedCorners,
> >                   const gfxCornerSizes& aBGRadii, nscoord aAppUnitsPerPixel,
> >+                  bool useBGClipArea,
> >                   /* out */ BackgroundClipState* aClipState)
> > {
> >-  aClipState->mBGClipArea = aBorderArea;
> >+  //REVIEW: This is a bit like the old mCustomClip, however it is only needed here so I thought it was better
> >+  //        to just add a new argument here then repurpose the mCustomClip and carry it around everywhere else.
> >+  if (!useBGClipArea) {
> >+    aClipState->mBGClipArea = aBorderArea;
> >+  }
> 
> So this change seems weird for four reasons:
>  * your useBGClipArea doesn't follow the prefixing convention ("aUse...")
I will try not to make this same mistake again..

>  * it's a confusing boolean:  "use" really means "don't modify"
>  * if the goal is don't modify, the it's not clear why you don't just change
> the value after calling the function
I guess my reasoning behind the name was "use the preexisting mBGClipArea" rather than explicitly don't modify. Or perhaps a better concept is more "Don't use the border area as the starting mBGClipArea.

>  * ... except that there are other places in the function that do still
> modify mBGClipArea, and we need to consider what the correct behavior in
> those cases is
> 
> In particular, the other cases that modify mBGClipArea are when:
>  * aForFrame is a scroll frame with background-attachment:local (not
> relevant here)
>  * the 'background-clip' property has a value other than 'border' (which I
> think probably is relevant here)
> 
> The first question, then, is which 'background-clip' property should matter
> when we're drawing the row background applied to the cell.  The current code
> considers the row's background-clip since it's the row that's passed as
> "aForFrame" from TableBackgroundPainter.
> 
> My weak intuition is that it makes more sense to use the cell's
> background-clip, but I didn't look at the spec (and I also suspect the spec
> doesn't say).
> 
> But that brings up another issue:  in the TablePainter.cpp code, should you
> be passing the row or the cell as aForFrame?  I think you need to go through
> the uses of aForFrame, see what they're used for, and see which usage would
> be correct.  For example, one use is for calculating the basis of percentage
> border-radius, and inthat case, it's clearly the cell that you want -- this
> patch doesn't handle percentage border-radius on the cell correctly. 
> Another use is for an nsFrame::AssociateImage call, which leads to
> invalidation when the image loads -- it's possible that also wants the cell,
> since it's not clear to me whether the way it invalidates the frame when the
> image loads would correctly invalidate a rowspanning cell that extends out
> of the bounds of its row.  etc.  (I'd almost have been ok with leaving the
> aForFrame business to another bug, except that it's used for percentage
> border-radius.  Sorry for not noticing that with my initial advice for how
> to proceed here.)
> 
> It's possible you want to simply pass the cell as aForFrame, but I think
> it's more likely that you want to split the aForFrame argument into two, one
> for the frame the background comes from (which is probably what you want to
> use for the IsThemed() check) and one for the frame in whose area the
> background is being drawn, and then figure out what the right thing to do
> for any other cases is.

So far I've only been looking at making the testcase cover these problems.

I have struggled to find anything relating to background-clip and table layers / elements. However by comparing to different browsers (using the attached test case) it seems though the following is what is going on:
the background-clip property from the actual <tr> element is used, but the border-box / padding-box / content-box from the cell is used for drawing the background. I'm also attaching an image showing the spectrum of results (on Windows anyway) as a reference.

One thing which seems to stand out in Firefox compared to other browsers is that when using background-clip: content-box; on a table element, Firefox behaves the same for a table cell as it does for a div. IE and Chrome (and Opera) do not draw the td background if the cell is empty and the background-clip is content-box. If the cell has content, then IE and Chrome draw the background only immediately covering the content, still not the same as Firefox's behaviour.
This doesn't necessarily affect this bug, except that it will cause problems with the test case (at least the way I was going to implement it) if this is ever changed in future. I would consider opening a bug for this, but I'm also not sure if it is a bug.

I'll continue looking into the aForFrame requirements.

Comment 34

[Adam Casey]

Attachment: Different renderings of the testcase

Excuse the reference not matching up exactly to the table code, the point with the differing content-box renderings in IE&Chrome vs Firefox is still there.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, what is it you wanted to know?

Comment 36

[Barry Carlson]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #35)
> Sorry, what is it you wanted to know?

Excuse my interference; but as the originator of the bug, I see that Adam Casey is trying to establish whether the method he is using is the right one. As mentioned, he has established that the IE/Chrome approach is different, and is seeking reassurance that his approach to resolving this bug is on the right track or not. 

Not that it actually matters to the resolution, but my observation of the progress made so far, is that the original application of border-radius failed to take into account many of the matters AC has raised. Is it not worthwhile pausing and reassessing the differences we now know that exist between the FF and IE/Webkit approach and determining from what point in the Gecko code sequence, the resolution of this bug needs to start.

Comment 37

[Adam Casey]

Sorry, I should have been more clear.

Is this a bug? If not that's fine. ("this" being how firefox renders a table cell with background-clip: content-box;)

If it is a bug, how can I ensure the testcase will still work when the bug is fixed, or should I just leave it for that bug to handle?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the IE/Chrome behavior with background-clip:content-box seems correct (definitely if the background-clip is on the cell; less clear if the background-clip is on a row or something else other than a cell), since the extra space around the contents of a cell is described as padding.  See, in particular, http://www.w3.org/TR/CSS21/tables.html#height-layout which says:

In CSS 2.1, the height of a cell box is the minimum height required by the content. The table cell's 'height' property can influence the height of the row (see above), but it does not increase the height of the cell box. 

and says:

Cell boxes that are smaller than the height of the row receive extra top or bottom padding. 


I think it's worth filing a separate bug for that if there isn't one already.  It's not worth spending too much energy making a test that handles both behaviors, though; the test could certainly be modified when fixing the second bug.

Comment 40

[Adam Casey]

Attachment: aForFrame comments.diff

Okay, I've looked over PaintBackgroundWithSC at the use of aForFrame. I'm not really sure how to narrate this, but I thought a patch might work.

The main point is that it should be fine to just pass cell as aForFrame.


Except:
The biggest question marks are over;
1. The use in PrepareBackgroundLayer. As said in comments, I tested the properties and they seem to function reasonably.
(the background-break behaviour is probably worth discusing in another bug, if it is a bug).

2. Using the cell's "GetUsedBorder" for background-clip, however as mentioned in the comment I think that using the other frame's value might cause problems anyway.

3. attachedToFrame in ComputeBackgroundPositioningArea.

Comment 41

[Adam Casey]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #32)
> 
> So this change seems weird for four reasons:
>  * your useBGClipArea doesn't follow the prefixing convention ("aUse...")
>  * it's a confusing boolean:  "use" really means "don't modify"
>  * if the goal is don't modify, the it's not clear why you don't just change
> the value after calling the function
>  * ... except that there are other places in the function that do still
> modify mBGClipArea, and we need to consider what the correct behavior in
> those cases is
> 
> In particular, the other cases that modify mBGClipArea are when:
>  * aForFrame is a scroll frame with background-attachment:local (not
> relevant here)
>  * the 'background-clip' property has a value other than 'border' (which I
> think probably is relevant here)
> 
> The first question, then, is which 'background-clip' property should matter
> when we're drawing the row background applied to the cell.  The current code
> considers the row's background-clip since it's the row that's passed as
> "aForFrame" from TableBackgroundPainter.
> 
> My weak intuition is that it makes more sense to use the cell's
> background-clip, but I didn't look at the spec (and I also suspect the spec
> doesn't say).
The background-clip property is retrieved from the aBackgroundSC parameter, and I think that's the best behaviour to stick to after looking at what is implemented by other browsers. I tried to find any mention of it in the spec, but your intuition over it's existance is probably better than my searching efforts anyway.

> But that brings up another issue:  in the TablePainter.cpp code, should you
> be passing the row or the cell as aForFrame?  I think you need to go through
> the uses of aForFrame, see what they're used for, and see which usage would
> be correct.  For example, one use is for calculating the basis of percentage
> border-radius, and inthat case, it's clearly the cell that you want -- this
> patch doesn't handle percentage border-radius on the cell correctly. 
> Another use is for an nsFrame::AssociateImage call, which leads to
> invalidation when the image loads -- it's possible that also wants the cell,
> since it's not clear to me whether the way it invalidates the frame when the
> image loads would correctly invalidate a rowspanning cell that extends out
> of the bounds of its row.  etc.  (I'd almost have been ok with leaving the
> aForFrame business to another bug, except that it's used for percentage
> border-radius.  Sorry for not noticing that with my initial advice for how
> to proceed here.)
Comment 40
> It's possible you want to simply pass the cell as aForFrame, but I think
> it's more likely that you want to split the aForFrame argument into two, one
> for the frame the background comes from (which is probably what you want to
> use for the IsThemed() check) and one for the frame in whose area the
> background is being drawn, and then figure out what the right thing to do
> for any other cases is.
For table painting, I'm not sure why IsThemed would be applicable. Is there something I'm overlooking?

I just realised the patch in comment 40 was created applied to a modified version of the previous code patch I submitted. 

I'll attempt to tackle the other issues raised and then submit the new code patch.

Comment 42

[Adam Casey]

I'm now trying to fix this again..

After running my assumptions against the test suite it was clear some of them were wrong.
Lots of border-collapse tests were failing, so I think it points to some assumptions of aForFrame usage in the border code (Probably GetBackgroundClip).

However, focusing on the border-radius/table cell background bug - the latest patch passes tests on my computer, and it addresses most of the concerns above.

The issue it doesn't concern is the background-clip problem mentioned above, this patch still reads the background-clip from the row / colgroup / etc. Unless you think this should really be fixed in this bug I was going to open a new bug and work on that one separately to this.

Comment 43

[Adam Casey]

Attachment: bug921341.diff

Some code-related comments for this patch: (If you're reading this in bugmail, see comment 42 for other comments)

I added a new argument to PaintBackgroundWithSC, which also involved changing one other instance of this function in a way which isn't intrusive, it's just a bit weird.
Another way to implement this might be cleaner (add a new function which is used non-tablepainter code).
The only thing we currently use the aBGAreaFrame for right now is correcting the border-radius issue - I plan to extend the use of this for the background-clip fix.

The argument name: aBGAreaFrame isn't exactly perfect, something more table painter specific would have been clearer, but I didn't think it was so good to have such table painting specific references in a different module - some guidance here would be useful.

In reply to previous feedback: I changed the name of the variable which had a confusing name - hopefully this is more clear. I do think the basic premise of using a boolean there makes sense - it's less of a "don't modify" and more of a "don't override".

Comment 44

[Adam Casey]

For the review, some of my comments in the previous "aForFrame comments" patch are worth looking at (the rest probably aren't worth your time) as in the latest patch I have not passed the cell's frame where I thought it might make sense to do so.

Specifically the frame usage in two functions;
In GetBackgroundClip I have left it passing the non-cell frame (for GetUsedMargin/Padding/SkipSides etc). I had problems with some table-background layout tests when doing that.

I also have not passed the cell's frame in PrepareBackgroundLayer as I never worked out what that would actually change, and I thought defaulting to not passing the cell's frame would provide the smallest impact on functionality.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry for letting this drag on so long.

I'm not convinced that I successfully communicated what I expected you to make the decisions for which frame's data to use at which point, or that I understand how you made the choices you did, so I feel like I need to review the patch unusually carefully.  I'm also concerned about it hurting the long-term maintainability and readability of the code in ways that I don't think are necessary to fix the bug; it feels like what we're trying to do conceptually isn't that complicated, but the conditions in the patch make it look more complicated than it is, perhaps out of a misguided desire to keep the modifications to the code minimal or safe.  For these reasons I feel I need to review it particularly carefully.

There were also 2 pieces of work going on here that are touching the same code; once bug 35168 lands I'm hoping to come back to look at this again.

Comment 46

[Flavio]

This is still a thing, please fix it.
Got many coders on this already: http://virtualinteractivege.com/index.php?action=who;theme=2 and still cannot render the last tr with the border-radius.
I have tried with Maxthon, Chrome, and even IE can render it. Honestly it is not acceptable from such a good browser to have a flaw like this. 
Please have a look on this and fix it once and for all. Thank you.

Comment 47

[Flavio]

Also, I didn't want my e-mail address to appear but I am not able to find a way to change it. Please delete my account and all the occurrencies of my e-mail appearing in the database, I'll create another one without my personal e-mail.
Why don't you allow people to change it anyway? :/ I don't want to get spammed.

Comment 48

[Matt Brubeck (:mbrubeck)]

(In reply to Flavio from comment #47)
> Also, I didn't want my e-mail address to appear but I am not able to find a
> way to change it. Please delete my account and all the occurrencies of my
> e-mail appearing in the database, I'll create another one without my
> personal e-mail.

Bugzilla will only show your e-mail address to logged-in users.  If you want to change your Bugzilla e-mail address, use the form here:

https://bugzilla.mozilla.org/userprefs.cgi?tab=account

Comment 49

[Flavio]

Thank you for your reply, I missed the last box.

By the way please do NOT hide my comments since this bug, like I explained, is STILL PRESENT. And it's the ONLY browser with this issue I was able to find, so once again please fix it.

Comment 50

[Matt Brubeck (:mbrubeck)]

We're sorry that this bug isn't fixed yet, but adding comments to this bug is not going to make it get fixed faster, unless those comments add information that directly helps with the fix.  Please follow the Bugzilla etiquette rules:  https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Thanks!

Comment 51

[Flavio]

Ooooh my bad then, yet I thought about bumping it because it's been a very very long time since this bug report has been opened. Sorry for not following those then, hope you fix this asap though, it's crazy that this is the only browser with this problem :/
Is there any ETA for this? It has been around for too much time :/

Comment 52

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 54

[Mitchell]

(In reply to Manish Goregaokar [:manishearth] from comment #24)

Enjoy!

https://tbpl.mozilla.org/?tree=Try&rev=25e88a7681c2 geometry dash world

I have enjoyed it.