[Webkit-unassigned] [Bug 9268] Quirksmode: Backgrounds on table rows

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=9268





--- Comment #15 from Daniel Bates <dbates at webkit.org>  2010-01-19 12:44:09 PST ---
(In reply to comment #14)
> (From update of attachment 44362 [details])
> Hi Daniel! I am sorry it took so long to get back to you on this. Dan and I
> talked this over today, and here are some of out thoughts.
> 
> First, the specification that you link to outlines a whole list of how tables
> should be painted, and this patch addresses one of the items on the list. Do
> you know if we do all of these things correctly right now? I suspect that we do
> not, but I could be wrong. It would be good to have a set of tests to know
> exactly where we stand on all of these issues before we try to fix one of the
> issues since we may want to take a different approach entirely if we want to
> try to fix them all at once. At the very least, it would be good to have bugs
> representing any of the other issues and to have a vague plan about how they
> might be fixed just to feel confident that all of the code to fix this issue
> wouldn't have to be re-written to fix the others. 

Yes. I am aware of related issues with respect to background images for column
and row groups.
I agree, the preferred solution is to fix everything in one shot. So, I will
look into this.

> 
> > Index: WebCore/rendering/RenderBoxModelObject.cpp
> > ===================================================================
> …
> > @@ -457,11 +459,32 @@ void RenderBoxModelObject::paintFillLaye
> >          IntPoint phase;
> >          IntSize tileSize;
> >  
> > +        RenderTableCell* cell = isTableCell() ? toRenderTableCell(this) : 0;
> > +        RenderTableRow* row = cell ? toRenderTableRow(cell->parent()) : 0;
> > +        bool isBackgroundImageForRow = row && bgLayer == row->style()->backgroundLayers();
> > +
> 
> 
> I don't like having all of this table-specific code in
> paintFillLayerExtended(). Generally, this is something we try to avoid in
> RenderBoxModelObject via polymorphism. I know that paintFillLayerExtended() is
> not virtual right now, but it might be worth considering making it virtual. Dan
> and I doubt there would be a performance hit for that. On the other hand, you
> could add a new virtual function, something like doNewBackgoundStuff() (but
> with a better name) that would do nothing in the base class
> (RenderBoxModelObject) but all kinds of things in RenderTable.

Yes, I will look to clean this up.

> > +        if (isBackgroundImageForRow && !cell->col()) {
> > +            // We calculate and store the background image geometry for the table-row with respect to the
> > +            // first cell's (tx, ty) coordinates. We will use this geometry to determine which table cells
> > +            // need to be painted (via intersection test) and what part of the background to paint in them.
> > +            calculateBackgroundImageGeometry(bgLayer, tx, ty, row->width(), row->height(), destRect, phase, tileSize);
> > +            row->setBackgroundDestRect(destRect);
> > +            row->setBackgroundPhase(phase);
> > +        }
> 
> Dan pointed out to me that this is insufficient. A lot of painting occurs
> incrementally and only certain cells get repainted. When a subset of cells is
> being repainted that doesn't include the first cell in the row, this will go
> awry. 

Will look into this.

> 
> > +        if (cell && cell->paintedBackgroundRect().width() < destRect.width())
> 
> I don't understand this comparison.Can you explain it to me? This probably
> deserves a comment.

If I recall correctly (been I while since I looked at this), I did this to
handle the case where the destRect is the rectangle that represents that scroll
difference when you scroll a page that overflow vertically. Notice, the width
of this rectangle <= the width of the rectangle that represents the table cell
whose background we want to paint (*). To paint the background of table cell i,
I calculate the phase in X to shift the background image, call it P_i =
(\sum_{j=0}^{i-1} P_j) + "painted background width of cell i - 1", where
"painted background width of cell i - 1" reflects the width of the largest area
painted of cell i - 1 so far (**). Notice, the largest such area is the area of
the entire table cell. Suppose the current vertical scroll position is such
that table cell i is completely outside of the view and table cell i - 1 is
slightly outside of the view. Assume I call
cell->setPaintedBackgroundRect(destRect) when destRect.width() <
cell->paintedBackgroundRect().width(). Then (**) is violated. In particular,
this can happen whenever you scroll (by (*)).

> > +    IntRect m_paintedBackgroundRect;
> > +    int m_paintedBackgroundPhaseX;
> >  };
> 
> 
> I don't like the idea of adding these bits to RenderTableCell. I don't like it
> because generally it's good to use fewer bits, and because very few
> RenderTableCells will actually even use these bits. I think it would be better
> to do these computations on the fly.

Will look into this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.