Bug 109212 – TDs not always removing border when losing CSS class that gives them the border

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Layout and Rendering

[Karolis Dzeja]

Created attachment 187136 [details]
Screenshot of the Bug. That lone border shouldn't exist and disappears when those cells are selected with mouse.

URL: http://jsfiddle.net/7vhR7/

This jsfiddle was extracted from a real world example. In the newest Chrome and Safari browsers, when the highlighted TD is changed, sometimes there are borders remaining that disappear when the TDs are selected with the mouse.

If you keep clicking the button, you'll eventually see the bug. It shouldn't take more than 10 or 20 clicks.

I've seen this on latest Chrome & Chrome Canary on Windows XP, Windows 7 and latest Safari on Mountain Lion. This bug does not happen in IE or Firefox.

[Yuki Sekiguchi]

Created attachment 204553 [details]
Patch

[Darin Adler]

Comment on attachment 204553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204553&action=review

> Source/WebCore/rendering/RenderTableCell.h:105
> +    void computeOverflow(LayoutUnit oldClientAfterEdge, bool recomputeFloats = false);

This should be marked virtual and OVERRIDE. Also, this should be private, not public.

[Yuki Sekiguchi]

Created attachment 204555 [details]
Patch

[Darin Adler]

Comment on attachment 204555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204555&action=review

I’m not going to review this because it should probably be looked at by one of the layout experts, but I still have one small comment.

> Source/WebCore/rendering/RenderTableCell.h:216
> +    virtual void computeOverflow(LayoutUnit oldClientAfterEdge, bool recomputeFloats = false) OVERRIDE;

Why protected instead of private?

[Yuki Sekiguchi]

Created attachment 204663 [details]
Patch

[Yuki Sekiguchi]

(In reply to comment #4)
> I’m not going to review this because it should probably be looked at by one of the layout experts, but I still have one small comment.

I see.
I'm waiting for review from a layout expert :)

> > Source/WebCore/rendering/RenderTableCell.h:216
> > +    virtual void computeOverflow(LayoutUnit oldClientAfterEdge, bool recomputeFloats = false) OVERRIDE;
> 
> Why protected instead of private?

Fixed.

[Simon Fraser (smfr)]

Comment on attachment 204663 [details]
Patch

I'm not sure that stuffing the old repaint rect into visual overflow is the correct approach here. Visual overflow should only be used for things like shadows/outlines, and not just for temporary dirty rects around style changes.

[Darin Adler]

Simon, could you point us at what the right direction would be?

[Simon Fraser (smfr)]

Not sure, really. I'd investigate to see why this isn't a problem for other boxes when borders change.

[Yuki Sekiguchi]

Hi Simon,

Thank you for reviewing.

(In reply to comment #9)
> Not sure, really. I'd investigate to see why this isn't a problem for other boxes when borders change.

The rect of RenderBox is always border box. This dose not depend on box-sizing property.
When border-width of a block is changed, we need to re-layout.
Before laying out the block, we save the original rect to LayoutRepainter at RenderBlock::layout().
After laying out the block, we repaint the original rect and the new rect.
Therefore, this isn't a problem for normal boxes when borders change.

However, the rect of RenderTableCell which border is collapsed contains half of its border width.
Therefore, we need to add other half of the border width to its rect when we calculate repaint rect.

I don't remember another CSS values which I need to investigate.
If you have concern about another CSS values, I can investigate them.

If adding RenderTableCell rect to visual overflow is not correct approach, I think we need to save the rect to    a member of RenderTableCell.

[Ahmad Saleem]

I am unable to reproduce this bug based on attached test case URL in Safari 15.5 on macOS 12.4.

I tried to spam click "highlight random" and then zoom in and out and also change viewport to trigger "lone border" but I am unable to reproduce it.

I think something along the lines got this fixed. Can we mark this as "RESOLVED CONFIGURATION CHANGED". Thanks!

If this is reproducible on any other platform etc., please retest accordingly. Thanks!