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

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
6 years ago
4 years ago

People

(Reporter: barrycarls, Assigned: adamncasey, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(5 attachments, 10 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Component: Untriaged → Layout: Tables
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 24 Branch → Trunk
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)
Whiteboard: [mentor=dbaron]

Comment 2

6 years ago
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
Flags: needinfo?(dbaron)
(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
Flags: needinfo?(dbaron)
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
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?
Flags: needinfo?(dbaron)
(Assignee)

Comment 5

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Posted image bug921341.PNG โ€”
Showing the problem with the previous patch. The top is IE11 rendering correctly, the bottom is Firefox with this patch.
(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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 9

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
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.
Attachment #8367488 - Attachment is obsolete: true
(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
(Assignee)

Comment 11

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
This patch should not cause any other tests to fail..

Includes proper reftest.
Attachment #8370312 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
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?
Attachment #8371684 - Attachment is obsolete: true
Attachment #8372265 - Flags: review?(dbaron)
(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 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.)
Attachment #8372265 - Flags: review?(dbaron) → review-
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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Posted patch A clearer method to fix the bug. (obsolete) โ€” โ€” Splinter Review
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
Attachment #8372265 - Attachment is obsolete: true
Assigned to Adam as per request in IRC.
Assignee: nobody → adamncasey
Status: NEW → ASSIGNED
(Assignee)

Comment 21

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
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
Attachment #8377234 - Attachment is obsolete: true
(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.
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Comment 25

5 years ago
Comment on attachment 8377289 [details] [diff] [review]
bug921341.diff

Patch passes tests.
Attachment #8377289 - Flags: review?(dbaron)
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.
Attachment #8377289 - Flags: feedback-
(Assignee)

Comment 27

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
Fixed nits suggested by Manishearth
Attachment #8377289 - Attachment is obsolete: true
Attachment #8377289 - Flags: review?(dbaron)
Attachment #8377587 - Flags: review?(dbaron)
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
(Assignee)

Comment 29

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
Whoops, now I've fixed the nits.
Attachment #8377587 - Attachment is obsolete: true
Attachment #8377587 - Flags: review?(dbaron)
Attachment #8377590 - Flags: review?(dbaron)
Attachment #8377590 - Flags: feedback?(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.
Attachment #8377590 - Flags: feedback?(manishearth) → feedback+
(Assignee)

Comment 31

5 years ago
Posted patch bug921341.diff (obsolete) โ€” โ€” Splinter Review
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.
Attachment #8377590 - Attachment is obsolete: true
Attachment #8377590 - Flags: review?(dbaron)
Attachment #8377593 - Flags: review?(dbaron)
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.
Attachment #8377593 - Flags: review?(dbaron) → review-
(Assignee)

Comment 33

5 years ago
(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.
(Assignee)

Comment 34

5 years ago
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.
Flags: needinfo?(dbaron)
(Reporter)

Comment 36

5 years ago
(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.
(Assignee)

Comment 37

5 years ago
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?
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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 40

5 years ago
Posted patch aForFrame comments.diff (obsolete) โ€” โ€” Splinter Review
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.
(Assignee)

Comment 41

5 years ago
(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.
(Assignee)

Comment 42

5 years ago
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.
(Assignee)

Comment 43

5 years ago
Posted patch bug921341.diff โ€” โ€” Splinter Review
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".
Attachment #8377593 - Attachment is obsolete: true
Attachment #8384985 - Attachment is obsolete: true
Attachment #8412176 - Flags: review?(dbaron)
(Assignee)

Comment 44

5 years ago
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.
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]
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 hidden (advocacy)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
You need to log in before you can comment on or make changes to this bug.