Bug 109212 - TDs not always removing border when losing CSS class that gives them the border
Summary: TDs not always removing border when losing CSS class that gives them the border
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://jsfiddle.net/7vhR7/
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-07 10:58 PST by Karolis Dzeja
Modified: 2013-06-17 19:36 PDT (History)
11 users (show)

See Also:


Attachments
Screenshot of the Bug. That lone border shouldn't exist and disappears when those cells are selected with mouse. (6.20 KB, image/png)
2013-02-07 10:58 PST, Karolis Dzeja
no flags Details
Patch (8.46 KB, patch)
2013-06-12 21:44 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2013-06-12 23:06 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2013-06-13 19:12 PDT, Yuki Sekiguchi
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karolis Dzeja 2013-02-07 10:58:16 PST
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.
Comment 1 Yuki Sekiguchi 2013-06-12 21:44:57 PDT
Created attachment 204553 [details]
Patch
Comment 2 Darin Adler 2013-06-12 22:17:08 PDT
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.
Comment 3 Yuki Sekiguchi 2013-06-12 23:06:24 PDT
Created attachment 204555 [details]
Patch
Comment 4 Darin Adler 2013-06-13 09:58:38 PDT
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?
Comment 5 Yuki Sekiguchi 2013-06-13 19:12:32 PDT
Created attachment 204663 [details]
Patch
Comment 6 Yuki Sekiguchi 2013-06-13 19:14:55 PDT
(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.
Comment 7 Simon Fraser (smfr) 2013-06-14 11:47:09 PDT
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.
Comment 8 Darin Adler 2013-06-14 17:47:28 PDT
Simon, could you point us at what the right direction would be?
Comment 9 Simon Fraser (smfr) 2013-06-14 17:54:28 PDT
Not sure, really. I'd investigate to see why this isn't a problem for other boxes when borders change.
Comment 10 Yuki Sekiguchi 2013-06-17 19:36:07 PDT
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.