Reflow of all cells occurs when the row's color changes

RESOLVED WORKSFORME

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED WORKSFORME
18 years ago
16 years ago

People

(Reporter: David Hyatt, Assigned: Marc Attinasi)

Tracking

Trunk
Future
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

18 years ago
Peter -

I'm seeing really slow tree widget selection performance, and it seems to be
because of a bug in the style code.  Basically the whole tree reflows when you
select an
item, even though the only style rule that I'm applying is one that changes
color.  I have verified this.  I assume that this constitutes a visual change,
and that the
computed style hint should therefore be VISUAL.

However, I'm seeing a REFLOW style hint being computed, and it happens in this
function.

PRInt32 StyleSpacingImpl::CalcDifference(const StyleSpacingImpl& aOther) const
{
  if ((mMargin == aOther.mMargin) &&
      (mPadding == aOther.mPadding) &&
      (mBorder == aOther.mBorder) &&
      (mFloatEdge == aOther.mFloatEdge)) {
    PRInt32 ix;
    for (ix = 0; ix < 4; ix++) {
      if ((mBorderStyle[ix] != aOther.mBorderStyle[ix]) ||
          (mBorderColor[ix] != aOther.mBorderColor[ix])) {
        if ((mBorderStyle[ix] != aOther.mBorderStyle[ix]) &&
            ((NS_STYLE_BORDER_STYLE_NONE == mBorderStyle[ix]) ||
             (NS_STYLE_BORDER_STYLE_NONE == aOther.mBorderStyle[ix]))) {
          return NS_STYLE_HINT_REFLOW;  // border on or off
        }
        return NS_STYLE_HINT_VISUAL;
      }
    }
    if ((mOutlineWidth != aOther.mOutlineWidth) ||
        (mOutlineStyle != aOther.mOutlineStyle) ||
        (mOutlineColor != aOther.mOutlineColor) ||
        (mBorderRadius != aOther.mBorderRadius)) {
      return NS_STYLE_HINT_VISUAL;
    }
    return NS_STYLE_HINT_NONE;
  }
  return NS_STYLE_HINT_REFLOW;
}

The very first comparison is looking at the margins.  (This is for a table cell
whose parent row had its color changed by a style rule.)  What's happening is
that the
margin on the left has values of 20,20,20,20, but the margin on the right of the
comparison has margins of 0,0,0,0.

This is causing the style code to give back a hint of REFLOW, and it is causing
the tree to reflow many many times just to select items (the table reflow code
handles
style changed by reflowing the whole table right now).  So I'd love to know if
there's an easy fix for this.

Hopefully this is something easy, like somebody inappropriately setting up a
default margin (or not) on one of the contexts.
(Reporter)

Comment 1

18 years ago
Peter, I did what you suggested.  At the time DidSetStyleContext is called when
the cell's style context is first initialized, it gets margins set to 0, and it
gets padding set to 1.

However at the time that the style change is being computed, those margins and
padding have both somehow been reset so that the unit is null and the values are
0.

Should the table cell frame be implementing ReResolveStyleContext or anything
like that? I don't know what that function is for, but it seems to have
something to do with knowing about doing stuff when the style context changes.

In any case, it looks like something is happening such that the stuff that the
table cell sets up in DidSetStyleContext is eventually lost.

I'm going to try working around this bug by setting a rule like this in xul.css.

treecell {
  margin: 0px;
  padding: 1px;
}

That should set up the same defaults that the table cell code is doing in C++
code, and hopefully will cause both values to always be explicitly set (and thus
to never get messed with by the table cell DidSetStyleContext code).
(Reporter)

Comment 2

18 years ago
Damn. It doesn't work.  The following comment is in DidSetStyleContext.

// Cache the border-spacing into margin and wipe out any previous
// margins, since CSS doesn't allow margins to be set on cells

That means it's going to be wrong no matter what I do.

Help!
(Reporter)

Comment 3

18 years ago
*** Bug 12283 has been marked as a duplicate of this bug. ***

Comment 4

18 years ago
Putting on [Perf] radar.
Whiteboard: [Perf]

Updated

18 years ago
Summary: [dogfood] Reflow of all cells occurs when the row's color changes → Reflow of all cells occurs when the row's color changes

Comment 5

18 years ago
[DOGFOOD] designation removed, being tracked by pork-jockeys.

Updated

18 years ago
Assignee: peterl → pierre

Comment 6

18 years ago
This needs to be fixed by removing all the code in nsTableCellFrame's
DidSetStyleContext and factoring the logic into style rules.

Comment 7

18 years ago
CSS does not allow table cells to have margins. However, the table code
currently uses the cell's margin as the cell spacing and it is very important
that all cells have the same margins otherwise tables will break, which is one
reason why DidSetStyleContext serves a useful purpose. The table code should
probably be changed to not use the cell's margins for anything. But then Hyatt's
attempt to set the margin of a cell wouldn't do anything.

DidSetStyleContext also sets the cells padding if it hasn't been set via a style
rule and the table has cell padding set.

There is also other code in DidSetStyleContext (ie. MapVAlignAttribute,
MapHAlignAttribute) which is currently not working and references bugs 1802 and
915.

If most of this can be accomplished with a new style rule as Peter suggests that
would be good. However, a change of this magnitude (and any table change in
general) needs to pass the table regression test before checkin.

Updated

18 years ago
Whiteboard: [Perf] → [dogfood]

Updated

18 years ago
Priority: P3 → P2
Target Milestone: M12

Updated

18 years ago
Target Milestone: M13 → M14

Updated

18 years ago
Target Milestone: M14 → M15

Comment 8

18 years ago
Moving table bugs to M15 just to see how many we have.

Comment 9

18 years ago
Moving [DOGFOOD] in Status Summary to dogfood in keyword field.
Keywords: dogfood
Whiteboard: [dogfood]

Comment 10

18 years ago
Is this still a problem with latest builds?  Is it still dogfood?
Whiteboard: [NEED INFO]
(Reporter)

Comment 11

18 years ago
I overrode this method in trees to prevent the reflow.  Tables still do it, but 
trees don't.  Therefore it's not a high priority for me.  karnaze will have to 
decide if it's a high priority for tables.

Comment 12

18 years ago
PDT- for dogfood and/or beta1
Whiteboard: [NEED INFO] → [PDT-]

Updated

18 years ago
Assignee: pierre → attinasi
Status: ASSIGNED → NEW

Comment 13

18 years ago
Block-moved to attinasi the bugs related to style inside tables
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

18 years ago
Created attachment 6493 [details]
Testcase showing that color change does not cause a reflow
(Assignee)

Comment 15

18 years ago
I just attached a testcase that shows that color change of an element within a 
table cell is not causing a reflow. I think this bug is reporting that a color 
change of a cell itself (background?) is causing a reflow... I have not been 
able to duplciate this. If anyone can attach a testcase I would appreciate it.
(Reporter)

Comment 16

18 years ago
Changing the background-color of a tr will cause a reflow.  Your test case has 
nothing to do with the actual bug.
(Assignee)

Comment 17

18 years ago
I cannot create test case that causes the TR to change it's background color. If 
you can then please submit it so I can work on it.
(Assignee)

Comment 18

18 years ago
Created attachment 6574 [details]
Testcase changing color of TR
(Assignee)

Comment 19

18 years ago
The last testcase I attached show the color of a TR being changed via the DOM. I 
see only Invalidates/Repaints taking place after that, no reflows. 

If there is some other way that the color is being changed that IS causing 
reflows then please report that (preferrably with a testcase) otherwise I'll 
mark this WORKSFORME.
(Assignee)

Comment 20

18 years ago
I do not see reflow when I change the color of a row. See the testcase at 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=6574. If there are 
other ways to change the color of the row that DO cause reflows please reopen 
and attach (or refer to) an illustrative testcase.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 21

18 years ago
Could you try this using "color" instead of "background-color"?  If that works, 
then I'll be convinced the bug is fixed.  I remember only that one of them 
worked and one of them didn't... it may have been background-color that worked.

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 22

18 years ago
Using the following code to test changing a TR color:

function trcolor(val) {
  if (val == true) {
    document.getElementById("tr1").style.color='red';
  } else {
    document.getElementById("tr1").style.color='green';
  }
}

I see a reflow the *first* time the color is changed, but all subsequent color 
changes result in no reflows. The Style system is returning a VISUAL hint in all 
cases, however the table code is reflowing after the first color change and I 
don't know why.

Just like was mentioned back when the bug was first written up, the problem is 
with the padding be set in the DidSetStyleContext method, causing the 
CalcDifference to send bac a REFLOW. I'm not sure if this has changed at all, 
since now there is only a reflow the first time the color of a row is changed, 
not every time.

From what peterl wrote, it looks like it is time to create style rules to set 
the padding and to remove the DidSetStyleContext logic, otherwise the table 
cells will continue to reflow.

Sorry for the mass-confusion on my part, Hyatt. It looks like the bug is still 
valid as written. I think this is a table bug, but I'll keep it and work with 
karnaze to resolve it (since there is a style component to the solution).

I've yet another testcase to checkin that shows the reflow happening the first 
time the color is changed.
(Assignee)

Comment 23

18 years ago
Created attachment 6729 [details]
Testcase showing a reflow on the first color change of a table row
(Reporter)

Comment 24

18 years ago
Just to reiterate, this bug is low priority for XUL/XPToolkit, so don't feel 
like you have to fix it on my account.  :)  Trees have "cheated" and overridden 
the table method that was causing problems, so trees don't cause a reflow when 
row colors change.
(Assignee)

Comment 25

18 years ago
I'm going to move this to M16 for now: it is an involved change and is not 
really that big of a problem (reflow is only on the first color change for a 
row). I have a lot of M15 bugs to fix... Thanks hyatt for the clarification- and 
prodding ;)
Status: REOPENED → ASSIGNED
Target Milestone: M15 → M16
(Assignee)

Comment 26

18 years ago
This is dependent on karnaze eliminating the DidSetStyleContext method from the 
TableCellFrame
Target Milestone: M16 → M20

Comment 27

18 years ago
Is this not fixed because it's not important to fix (as a small performance 
issue), or is it not fixed because we want to fix as many incremental reflow 
bugs as possible before reducing the number of incremental reflows?
(Assignee)

Comment 28

18 years ago
This has been pushed-off because it is only a small performance problem, and 
because the solution involves reworking a bit of Table code that is making it so 
the Style System cannot evaluate the impact correctly.
(Assignee)

Updated

18 years ago
Severity: critical → normal
Priority: P2 → P3
(Assignee)

Comment 29

18 years ago
Clearing dogfood - see David's comment of 2000-03-20 16:23
Keywords: dogfood
Whiteboard: [PDT-]
(Assignee)

Comment 30

18 years ago
This bug has been marked "future" because we have determined that it is not 
critical for netscape 6.0. If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.
Target Milestone: M20 → Future
Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
QA Contact: ian → amar
Just tried this with a recent Linux CVS build, and I see no reflow (WFM).
Still no sign of reflow, and this code's probably changed. WFM.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago16 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.