Summary: | TDs not always removing border when losing CSS class that gives them the border | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Karolis Dzeja <karolis> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||
Severity: | Normal | CC: | ahmad.saleem792, arpitabahuguna, bdakin, commit-queue, darin, esprehn+autocc, glenn, hyatt, robert, simon.fraser, thorton, yuki.sekiguchi | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
URL: | http://jsfiddle.net/7vhR7/ | ||||||||||||
Attachments: |
|
Description
Karolis Dzeja
2013-02-07 10:58:16 PST
Created attachment 204553 [details]
Patch
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. Created attachment 204555 [details]
Patch
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? Created attachment 204663 [details]
Patch
(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 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.
Simon, could you point us at what the right direction would be? Not sure, really. I'd investigate to see why this isn't a problem for other boxes when borders change. 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. 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! |