1.05 KB, text/html
787 bytes, text/html
7.08 KB, patch
|Details | Diff | Splinter Review|
4.25 KB, text/html
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041007 Debian/1.7.3-5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041007 Debian/1.7.3-5 I had a lot of surprising problems when I designed sites with the border-collapse:collapse property enabled (this is the default). In fact there are a lot of tiny bug reports for several problems I encountered (see 262560, 244135, 248374, 155955), but I think it is a deeper problem. It is not specific to a dynamic change of CSS styles, or classes, because I get wrong behaviour even in a static case (but in the dynamic case several errors show up). It seems a) That invalidate rect covers only the space belonging to the table, that is to say half of the borders are out of it, but only in the case of a CSS border conflict which is solver in point 4 of border conflict resolution as specified in the CSS2 recommendation "http://www.w3.org/TR/1998/REC-CSS2-19980512/tables.html#border-conflict-resolution". b) That rects at the intersection of a horiz border and a vert border do not ever get the right color (the border conflict resolution algorithm sets its color as well as the neighbouring 'preferred' border color). There are also 'missing' (with gray color) borders. c) That sometimes the selection of a color for a border influences the whole line or column border (as in the second table of the example, when you put your mouse under the big bordered row)... Very strange. [ONLY PRODUCED IN DYNAMIC, do not know if specific or not] d) When dynamically changing class (and thus style), border-width seems not to be affected. It could (as far as I know) be a FEATURE, because CSS recommendation does not tell what to do in such a case, and one would surely avoid calculating table layout again, but... [ONLY PRODUCED IN DYNAMIC] e) Some times, when we change from a black border to a gray one, a black piece of border appears ! In fact, 'missing' borders are drawn at that time. It seems to occur only when we move the mouse to the cell above (and below ?). I am sure a lot of other errors can be found using this test page, which seems to gather every erroneous choices Gecko makes :-((( And there are shomewhat few :-) Reproducible: Always Steps to Reproduce: 1. Open the test page (xhtml, but not specific, tried in legacy HTML mode too) 2. That is already sufficient to see some of the errors (b in particular) 3. Move your mouse on the tables to see other errors Actual Results: I saw completely erratic rendering of borders, described in detail... in the details section ;-P Expected Results: 'highlighted' rows should be in bgcolor yellow and border black, with (depending of the choice of implementation) should (or not) be 5px wider in the second table only.
Sorry if this bug report is repeating information found in other bug reports. But I think it adds some.
Created attachment 167037 [details] Testcase Here is the test xhtml page I discuss about in the details section
I tested a bit more, and that's what I found. When a row's class is changed (and thus CSS style of it's tds), the invalidate rect on the page is limited to half of the border. Indeed, only half of the border really belongs to the row, and the other half is on neighbouring cells. That's why when we do dynamic changes some colors remain unchanged, and also why some appear at the wrong time ! For example : I am on a HL row and move the mouse down to the cell just below. Several cases can arise : - The first (upper) row is invalidated (because it looses its HL class) after the second gains its HL class ; then the upper part of the border beetween the two rows becomes black. The lower part become black when the below row is invalidated. Then the whole border is invalidated, though it really is a coincidence. So we should invalidate a bit more than the row's extent, namely the rect including whole borders. Then some borders may be invalidated twice, when the mouse leaves a row and when it enters the row below, but we cannot predict next position of the mouse, so it is the right behaviour It is one of the problems I encountered and can surely be solved easily [and may perhaps have, I use a standard release build so cannot know if the problem still exists in latest builds]. Solving this sole problem can bring much more correctness to the collapsed-border table rendering. _FrnchFrgg_
Concerning the 'bug' with borders not changing their width dynamicly, It IS a bug, because in the separate borders model Gecko's crew has decided to update border widths as well (which can lead to much flickering in a bad designed site). BUT, the fact that widths are changed in CSS - even if the change is not drawn - prevents borders to be half-invalidated. [It seems so at least] There remains problems, as whole line border drawn in black, ..., but this one disapeared. I so question myself : How is the rendering of border-collapse: collapse tables written ? Why invalidates are different if the border-size has changed ? Shouldn't there be a single rendering engine that 1) solves conflicts - and this part is currently wrong - and decide which will be the properties of every table's part 2) Decides what portion of the screen must be invalidated, in comparing old and new decided layout ? I have currently no time to look at the code, but I may do so. Meanwhile, I refer to you far more skilled folks. _FrnchFrgg_ P.S.: Sorry for so much updates in so short delays ; I know some people will get upset by my trend to fill up their mail boxes.
Attachment #167037 - Attachment is obsolete: true
Created attachment 167122 [details] New version of the testcase - To be able to test CSS choices without the invalidate of interferences, class HL is never removed for first table. - HL elements in second table have a wider border (notice that only the one that got HL during launch gets this border). - Second row of third table has a wider border. - Remaining tables are alike the first one, but with initial HL changed.
LAST COMMENT FROM ME FOR TODAY : Depending on the way we manualy invalidate the table, we get differents results : - When it is fully invalidated, either because of a change of the border width in CSS [which seems to, while not really changing widths of borders, invalidate the whole table] or because of a full screen redraw (tab switch, window reduction/restore, workspace change, ...), then we notice problems already described, as blackening of the full vertical border instead of just the part adjacent to the HL cell. - When we invalidate parts by scrolling the table, it becomes erratic : some times we get the same result as when we invalidate everybody (which is expected, because we want to get always the same result, even if it is wrong because of a previously ill-taken decision), and sometimes we get gray borders. There so are two profound problems in the rendering : 1) Invalidates are rarely done as they should, with the puzzling case of partial scroll-related redraw. 2) Choices of who to put in what color/size are wrong if we refer to the CSS2 specification. Try every case described with the testcase that should, now, work (I had sent it as text/html, so <!CDATA[[ ]]> part was ignored and function 'hi' undefined), and see. You can also use the same testcase with 'border-collapse: separate' to notice the correct behaviour in that case (WITH border changing). _FrnchFrgg_
It's better to limit bug reports to one issue each, so that issues can be discussed separately, simpler testcases can be added, bug status can be tracked, etc.
Although it seems, from reading the 5 points, that this does cover primarily one issue. Since this mostly covers failure to invalidate, I'll make this bug about that (although that bug summary may still be too broad). Bugs in static rendering should be filed as separate bugs.
Summary: Gecko's rendering of 'border-collapse: collapse' is completely broken. → dynamic changes in border-collapse tables don't invalidate enough
(In reply to comment #8) > Although it seems, from reading the 5 points, that this does cover primarily one > issue. > > Since this mostly covers failure to invalidate, I'll make this bug about that > (although that bug summary may still be too broad). Bugs in static rendering > should be filed as separate bugs. In fact, I primarily posted this bug report because I found a lot of tiny bug reports all describing single errors in the engine rendering collapsed bordered tables, and because there were a LOT of them I wondered if a more profound rewrite would be helpful... But I know this is a great amount of work to do so and that people able to do that are the happy few. While examining errors, I found there is mainly a well circled invalidate bug, and another problem*. We could fork this thread in two if you want. * it really seems that Gecko does not manage CSS priorities correctly in this case ; sometimes the border is blackened from one extent of the table to another instead of just around the cell, sometimes each border of the cell has not the same color - though the color of all of them is changed in the same rule. Perhaps Mozilla's decision is 'too' local and he makes different decisions at different times. The solution is perhaps to maintain a set of properties for EACH border of the table, which may permit to solve conflicts in a W3C manner, and compare between two successive states to know what to invalidate. See comments #4 & #6. Do whatever you want with my reports, I only want to help designing a better engine (hard task :-) and I am willing to adapt myself to your habits. _FrnchFrgg_
I'm not following any issues other than invalidation that you think are wrong. The testcase is complicated, and understanding what you think is wrong requires a *clear* description of what the expected behavior is and how Mozilla's behavior differs from that behavior. (In other words, saying something like "the left border of the third cell in the second row of the fifth table is black, but it should be gray. This appears to be because Mozilla is treating the column border as higher priority than the cell border".) There's not room for more than one of those in a bug without massive confusion about what comment is about what problem, so they should be in separate bugs.
The 'solution' I propose for the rendering -- if it is usable -- may probably solve both 'sub'-bugs I submitted in this thread. So it may be logical to discuss both in the same. I restate I do not know how it is implemented, somebody has certainly found a far cleverer way to do it, but it is currently broken. I am not sure fixing each error with a local patch may lead to fast and reliable code -- when I develop software (which is far simpler than Gecko I must admit) and it happens that there are a lot of bugs on the same unit of sense, I prefer planning a ground rewrite of the erroneous part, because most of the time such bugs are inter-related (but I write smaller software, and in this case it may be unfeasible to restart even the sole collapsed border table rendering). _FrnchFrgg_ P.S.: I'll try in the future not so split what I say in two posts. Sorry.
I can start a new bug-report for the other problem I noticed (apart from invalidating). The testcase is not that complicated. There are five tables, to really find out what are the causes of what. The invalidation problem is blattant, but does not occur in second table. The only difference between it and the others is that I order (but am not obeyed) a change of border-width. It thus seems that the invalidating error is related to moments when nothing but the color changes (at least not the border width). What I state is that even if we do not see it (because of the wrong invalidation problem) there are sometimes borders that are drawn in black whereas they shouldn't. Further explanation : When the first two rows of the first table are HL, whatever CSS tells us about how to render them, the other rows MUST remain all borders grayed, and that clearly isn't. That's why I disabled the lost of HL class for the first table, in order to be able to force invalidation in a couple of manners.
The example of error you gave in comment #10 is indeed a bug I encountered. ("the left border of the third cell in the second row of the fifth table is black, but it should be gray. This appears to be because Mozilla is treating the column border as higher priority than the cell border".) _FrnchFrgg_
Created attachment 167141 [details] [diff] [review] fix invalidation problems This fixes the invalidation problems by including a large estimate of the collapsed border in the overflow area.
Comment on attachment 167141 [details] [diff] [review] fix invalidation problems This is sort of ugly, but it fixes the repainting problems, although there are still a bunch of other problems.
Attachment #167141 - Flags: review?(bernd.mielke)
(In reply to comment #15) > (From update of attachment 167141 [details] [diff] [review]) > This is sort of ugly, but it fixes the repainting problems, although there are > still a bunch of other problems. > I read the patch you proposed, and though a bit ad-hoc (a too local solution) I think it solves this problem of invalidates. Could you yet try and see if you can get this wrong behaviour : I load the testcase, and highlight the second row of the first table by moving mouse over it. Then I force a total redraw of the screen and get a first error : the whole left and right borders of the table get painted in black. Then, I scroll down to hide the first table, and scroll up to reshow it. At that time results I get are unpredictable. I get a mix of the table whose left & right borders are gray and the table with the same borders black. In other words, repainting does not produce the same result each time. I am not even sure one of the renderings (the one when I get gray borders) is right, but I am sure of one thing : if it is the same repainting method which is called when we do a full repaint and a partial repaint due to scrolling, it's worth turning everybody crazy. I'll attach a image of what I get.
Assignee: nobody → dbaron
Hmmm, at first glance I can't believe that this patch is correct. The computed border styles for BC are stored in the cellmap and not in the table cell style properties. The style properties would need to be an array for this as one bc table cell border can have more than one segment due to col- and rowspans. The current bc computation stores in the cellmap the information whether a corner is a start for a segment. Color changes mean different segments. Each border change, including a color change in the current scheme needs to cause either a style change reflow of the corresponding bc table descendendants which should then set the BC damage Area (bug 229712) or set the BC damage area directly to force a recomputation directly. (yes, I know this is a hack and even worse its hack for a hack)
The things I'm using for the invalidation are a maximum for the inner part, set by nsTableFrame during border computation. Some other things seem to work right, modulo invalidation (done by ApplyRenderingChangeToTree) not invalidating what's needed, but a bunch of things also don't work. If you want this to work some other way, you'll need to implement a new style hint.
I think the model of rendering should be this one in collapsed border tables : Wa maintain a private member which is a two-dimmensionnal array of border properties. Every chunk of border is referenced (even when we have colspans or row spans) but we could have a special value meaning that we have the same properties than another specified chunk. Each cell in the table could have a member telling us which chunks belongs to it. Then, when we have a style change (due to whichever reason), we can compute again a new array of border properties according to CSS conflicts resolution, and then we can in each cell know what to invalidate, and the redraw is easy to do -- and fast, because every decision has already been taken. To know what to invalidate during the change of style, that is when we discard old border-props array and store the new one, it is easy to compare the old an new border array and invalidate each chunk that need it [while not forgetting to reflow the whole table if a border-width has changed -- unless we choose not to update border widths during dynamic CSS changes, to avoid too much flickering ; it is not, though, the choice made in the non-collapsed border model] I am pretty sure it would be efficient and easy to implement -- perhaps it is already done this way and then errors would be easy to track and eliminate. _FrnchFrgg_ P.S.: Rewriting only the collapsed border model seems to be a matter of reimplementation of nsBCTable*Frame classes, so it may be feasible.
Comment on attachment 167141 [details] [diff] [review] fix invalidation problems Including only the cell border is correct, if it is the widest and wins, then we will invalidate wide enough if it is too small or does otherwise not win then changin the color will have no effect anyway. What I like with this patch we make a step towards painting the border by the cells themself.
Attachment #167141 - Flags: review?(bernd.mielke) → review+
I also like the idea of painting borders directly by concerned cells. But the decision of which color/size/etc... to put on a particular border cannot be taken by a single cell, that's why I spoke about a pointer to each of four borders adjacent to the cell. I should download and discover how the engine of border collapse model is written -- or you could tell me (briefly) what is it's structure. _FrnchFrgg_
The general problem with current BC code is outlined in bug 203686. Inside CalcBCBorders we compute the borders and store them in the cellmap (this is the 2 dim array you are talking about see http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/celldata.h) Getting a working debug build is always a good idea. Just ask me by email if you ahve further questions.
I read source code from the link you submitted to me. I wonder if the strategy for border collaped tables painting is really good, because borders overlapp cells, so painting must be done in a specific order, which is not necessarily the order of the paint events. I think the table must be responsible of the drawing of everything of it that is OUTSIDE cells. That means nothing gets painted in cells by the table, and cells are to paint their halves of borders. This way we have no problem about repaints, and I think there won't be an enormous downgrade of speed introduced by the two-steps borders repainting. Or, we could consider that cells are responsible for drawing their full borders, but then we could redraw the same border twice, so we may not gain much concerning speed. Current solution is to my mind a hack of the non collapsed model, and we shouldn't rely on that rather than writing a separate engine for a separate model. _FrnchFrgg_
A precision to last comment : There is no problem for other thinks drawn by the table than borders, because only borders must stay over cells. Thus when I say 'the table paints nothing inside cell space' you must read 'no border'. _FrnchFrgg_
Is it normal that this bug is still UNCONFIRMED whereas it has successfully been reproduced on several systems ? _FrnchFrgg_
Created attachment 177480 [details] Minimal testcase for the bug (and another). I still can get the invalidation bug on Firefox 1.0.1 (debian build). Here is a minimal testcase, that also underlines another blattant bug (one of those I already talked about). I'll file in a bug report for this one too, if there is none already. _FrnchFrgg_
This bug has been reproduced on Mozilla & Firefox, Windows & Linux. I do not have a Mac lying around, so I couldn't check. But please do not leave this bug UNCONFIRMED, as nobody takes it into account. -- I am not empowered enough to do so bugzilla tells me;(. _FrnchFrgg_
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #167037 - Attachment mime type: text/html → application/xhtml+xml; charset=iso-8859-1
Attachment #167122 - Attachment mime type: text/html → text/html; charset=iso-8859-1
Created attachment 177591 [details] [diff] [review] fix invalidation problems same patch, merged to trunk
Comment on attachment 177591 [details] [diff] [review] fix invalidation problems We need a comment describing GetSelfOverflow. I also think that nsBCTableCellFrame::GetSelfOverflow needs an XXX comment noting that this is potentially going to throw outlines and scrollarea sizes off by one pixel.
Attachment #177591 - Flags: superreview?(roc) → superreview+
Invalidation fix checked in to trunk, 2005-03-17 18:15 -0800.
I filed Bug 286794 about a regression caused by this bug here.
My girlfriend was playing with the testcase, and discovered some other stuff : she realized she could do sort of a painting with a window invalidating regions of the viewport. And I noticed one thing : The border of the cell, which should be yellow, is always drawn yellow correctly (when and where it is asked to be redrawn, of course, but your patch solves that). But the part of the borders wrongly painted in yellow aren't painted yellow every time. In fact, I have a far more precise information and have filed a new bug for that : "https://bugzilla.mozilla.org/show_bug.cgi?id=286797"
Isnt this bug fixed by bug 286794?
The regression from the invalidation patch here was; this bug still describes a whole bunch of other problems (see comment 7, etc.).
(In reply to comment #36) > The regression from the invalidation patch here was; this bug still describes a > whole bunch of other problems (see comment 7, etc.). Yes, but I filed another bug (https://bugzilla.mozilla.org/show_bug.cgi?id=286797) only on the other problems encountered, with a minimal testcase (far more simpler than the one that is here) and a precise description of the behaviour (I namely found that the wrong color was painted if and only if the invalidate region intersected a given pair of rects, see tescase for details). The remaining bug here is only the invalidation not covering the whole borders, and it isn't solved in FireFox 1.0.2. But I gathered from previous comments that the proposed patch broke some other behaviour, so this may explain that. _FrnchFrgg_ P.S.: I'd really like some folk to examine my other bug, because it is far more precise. Moreover, nobody commented it, nobody confirmed it (apart from friends of mine, but not powered enough to mark the bug new). You advised me to split my bug in two, and I did so, but I expect it doesn't mean that a half would be paused an indefinite amount of time. (I am not upset, nor angry, nor agressive, I just express a wish).
_FrnchFrgg_ - have you asked Gerv for upgraded permissions yet? http://www.gerv.net/hacking/before-you-mail-gerv.html
The testcase there *still* seems to cover about 4 different things. The testcase for the bug should cover *one* problem; if you think the others are likely to be fixed by the fix, you can also add them as additional testcases to the bug, but the primary testcase should be something that either passes or fails.
(Maybe it's not so bad, actually, but it's still better to have the steps to reproduce in the bug, outside the testcase, and with clearer expected and actual results.)
This bug seems to be corrected in the Deer Park Alpha 1. There remains the other bug, but it is already far better. _FrnchFrgg_
Isn't this bug FIXED in FF 1.5 and current trunk ? I could set resolution since I am the reporter, but I am not sure I should play with such things...
There are still a bunch of problems with borders remaining in the wrong state when mousing over attachment 167037 [details] moving the mouse up and down over the tables, and also a weird problem in the second table when moving the mouse from the second row to the third. Those should be filed as separate bugs before this one is closed. (That's one of the reasons I wanted one issue per bug originally.)
(In reply to comment #44) > There are still a bunch of problems with borders remaining in the wrong state > when mousing over attachment 167037 [details]  moving the mouse up and down over the > tables, and also a weird problem in the second table when moving the mouse from > the second row to the third. Those should be filed as separate bugs before > this one is closed. (That's one of the reasons I wanted one issue per bug > originally.) > To me, those seem all to relate to bug 286797.
More precisely, if there are other problems than the one described in bug 286797, there will be very unlikely to be decipherable until 286797 is solved. Perhaps keep the bug open to still have the "monster testcase" handy...
I think that the reason of bug 286797 is that the style change does not trigger any update of the border map. By adding nsRect rect(0, 0, GetColCount(), GetRowCount()); SetBCDamageArea(rect); CalcBCBorders(); at the beginning of nsTableFrame::PaintBCBorders, thus forcing the border map to always be up to date by a ugly hack, the testcases (mostly) behave. Remaining gliches of monster testcase : - static one, outlined in bug 286797, should (and may already) have its own bug. Note that when 286797 is solved, the dynamic case will get the same (wrong) behaviour as the current static case. - some strange changes of size of cells in the second table, not outright wrong, but unpleasant to the eye. May come from my ugly hack.
Static case bug is bug 325074.
Is there any improvement on the border rendering coming? It´s 2 years ago now, and even in version 3.0b2 it´s still the same: Table borders cannot be drawn dynamically in collapse mode. This is very very nasty and prevents many designs which are no problem in Internet Explorer! Even with collapse=separate the rendering is not robust and sometimes stochastical. I can´t believe that this is such a big issue that it cannot be solved within 4 years. Best regards, Kristian
Kristian: The table border code is an ugly mess, and nobody has enough time amongst those skilled enough to rewrite it. Those happy few are... few, and their time has been spent evolving other parts of Gecko.
That´s what I expected, but the hope dies last ;) Thanks for your response. The "mess" applies to Thunderbird also, by the way. There´s so much ... okay, let´s see what happens in further versions. For the certain case I have found a workaround - when I create the complete table definition by document.write, it works. Best regards, Kristian
Created attachment 322785 [details] Additional testcase for dynamic changes, including workaround I've run into this bug myself on both Firefox 220.127.116.11 and Firefox 3.0-rc1. The attached testcase demonstrates a case where Firefox renders the borders wrong after a dynamic change. Additionally, it includes workaround code (tick the "Apply workaround" checkbox, basically toggles border-collapse after changing borders to force recalculation/repaint), which allows the dynamic changes to be applied correctly. To see the bug, mouse over each of the groups on the left. To see the correct rendering, tick the checkbox and then mouse over the groups again. Furthermore, the rendering is differently incorrect depending on the initial correct state (that is, if the first, second, or third row is selected and drawn correctly). There is a broken image at the right, this is intended, it's just a placeholder (and has an empty src).
Julien, isn't this bug wfm now?
(In reply to comment #53) > Julien, isn't this bug wfm now? See comment 42 to comment 46, and comment 48. Short story: the invalidation bug has been fixed, and I believe that the other bugs exhibited by the "monster testcase" (attachment 167037 [details]) are bug 325074 and bug 286797. I think we should close this bug, but it may be a good idea to keep the "monster testcase" around to really verify if it doesn't exhibit another problem after the others are fixed.
Just to be clear, the resolution should be FIXED and not WFM, since the bug has been fixed here by attachment 177591 [details] [diff] [review].
You need to log in before you can comment on or make changes to this bug.