28522 - Clicking or tabbing to link causes incremental reflow

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[troy]

See the attached test case for the HTML, but this simple test case causes us to 
generate an incremental reflow command when the link is clicked

The page is very straight forward (a slight;y simplified version of the test 
case David Baron filed for bug #28434) HTML.

It seems that the vlink="#444464" link="#000066" attributes are what is causing 
the incremental reflow command to be generated.

This is a performance problem to have us doing an incremental reflow when links 
are clicked on. Marking beta1 and setting milestone for M14, because I think we 
need to fix this and I think it's a regression

Comment 1

[troy]

Attachment: Simple test case

Comment 2

[jst]

This might seem like a weird request but please don't fix this untill the
crasher bug 28434 is fixed, the reason I say this is that the reason for the
crash in 28434 is the reflow that happends when you click on a link and if
there's no more reflow when clicking on a link that bug will be mush harder to
reproduce...

Comment 3

[troy]

I'm not worried about being able to reproduce bug 2834. It's a recent regression 
and I know exactly what lines of code caused the regression.

We really do need to fix before we go out to beta

Comment 4

[jst]

Ok, cool! ;)

Comment 5

[leger]

Putting on the PDT- radar for beta1.

Comment 6

[troy]

Why is this being downgraded to PDT-? This is a performance problem and it needs 
to get fixed...

Comment 7

[troy]

Rick asked me to better explain this. What happens is that when you click on the 
link the _current_ page gets an incremental reflow, and then we proceed to load 
the new page.

Doing the incremental reflow on the page we're leaving is unnecessary (it's only 
a color change), and it slows down the load of the new page

Comment 8

[daver]

Troy, what is the performance gain from fixing this?

Comment 9

[troy]

We avoid doing an unnecessary incremental reflow. That means we will get the new 
page displayed that much quicker.

I can't give you a percentage improvement, because it depends on the current 
page (that will get the additional incremental reflow) and how big it is and the 
new page and how small it is

Comment 10

[leger]

Putting on PDT+ radar for beta1.  Can you please add an estimated time to fix in 
the Status Whiteboard.  Thanks!

Comment 11

[rickg]

Removing PDT+ from this bug. It's not *as* important as other issues, and pierre 
is swamped. Let's try again in m15.

Comment 12

[rickg]

marking PDT-.

Comment 13

[Marc Attinasi]

Taking over this one to help reduce Pierre's bug count. Moved to M15 also.

Comment 14

[Pierre Saslawsky]

Marking this bug as dependent of bug 7617 and bug 28212. These 2 bugs are 

probably duplicates: they both describe reflow problems inside tables 

consecutively to a reflow generated by a clicking a link inside the table.



In fact there is no real dependency between this bug and the 2 others except that 

we should make sure we fix the table code before we stop generating the reflow 

from the link, unless we have another way to cause the problem described in 7617 

and 28212.

Comment 15

[Marc Attinasi]

The problem here is that clicking on a link causes the border to appear around 
the link (as a focus indicator) and that causes a reflow. Also, you can get the 
reflow by tabbing the focus to a link, you don;t have to click on one.

I will attach another testcase that shows how the color change on the link does 
NOT cause a reflow, it is just the border being drawn to indicate focus. 

This is the simplest of a series of bugs relating to the same problem: focus is 
indicated by drawing a border and that changes the spacing which causes a 
reflow.

Comment 16

[Marc Attinasi]

Attachment: Testcase showing that color chanegs do not cause reflows but focus does

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's being drawn is an *outline*, not a border, and the outline does not
affect spacing.

However, see my 2000-02-18 18:47 comment on bug 28434.

Comment 18

[Marc Attinasi]

I the culprit is in StyleSpacingImpl::CalcDifference where it says:

if ((mOutlineWidth != aOther.mOutlineWidth) ||
    (mOutlineStyle != aOther.mOutlineStyle) ||
    (mOutlineColor != aOther.mOutlineColor) ||
    (mOutlineRadius != aOther.mOutlineRadius)) {
      return NS_STYLE_HINT_REFLOW;	
      // XXX: should be VISUAL: see bugs 9809 and 9816
    }

Clearly there are problems with outlines that we need to address wholesale, 
because if I fix this one then we get painting problems in outlines. From 
looking over the comments in bug 9816 it sounds like the frame code needs to 
compute the area to invalidate such that it includes the outlines.

I'm marking thsi dependent on 9809 since that addresses fixing the paint issues.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Just thinking, without really looking at the code, I think there would be a few
ways to do this:
 * store the previous outline width in the frame.  This is bad for memory usage.
 * use a new style hint, NS_STYLE_HINT_OUTLINE, that tells the style system to
invalidate the frame both before and after the re-styling, so that the changes
are sure to be repainted correctly.  (I have not investigated the feasibility of
this at all in the code.  I'm just making things up here...)  We'd also need to
make sure invalidating the frame included invalidating the outline (which it
might not need to for any other invalidates).

This wouldn't deal with the problems with views, though, but it would be a
start.  It's not even clear if the invalidate works correctly on outlines... I'd
have to check.

I'm pretty sure the :focus is implemented using outlines, since the rule:
:link:focus, :visited:focus, :out-of-date:focus,
:link:focus img, :visited:focus img, :out-of-date:focus img {
  outline: 1px dotted black;
}
exists in html.css.

I'd like to look into this more, but I just don't have the time...

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The style change is created in StyleSpacingImpl::CalcDifference
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsStyleContext.cpp#744
and handled in nsCSSFrameConstructor::ProcessRestyledFrames
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#7602

However, it doesn't look like it would be to easy to do two invalidates, because
the style context is replaced.

Comment 21

[Marc Attinasi]

Moving to M16: too many M15 bugs and this one is dependent on 9809 which is M16.

Comment 22

[Marc Attinasi]

I just noticed that the offending line of code has been changed by jst (checking 
in for Vidur). StyleSpacingImpl::CalcDifference now returns VISUAL instead of 
REFLOW whent he spacing is different, however this is going to cause outlines to 
be left on the screen (see bug 9809 and 9816).

Marking fixed even though I didn't fix it; the reflow is no longer happening.

Comment 23

[Marc Attinasi]

Regarding my last comment:

VISUAL is returned when the spacing's *outline* values have changed, I didn't 
mean all spacing values.

Comment 24

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- MacOS9 2000-07-06-08-M17 Commercial
- Linux6 2000-07-07-10-M17 Commercial
- Win98  2000-07-07-13-M17 Commercial