90081 - changes to Colors prefs aren't immediate, requires page reload

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[sairuh (rarely reading bugmail)]

i've seen this for some time: when i change the Colors prefs, not all of the
changes take immediate effect. i need to reload the page to see the changes.

tested using 2001.07.09.0x-branch commercial bits on linux, mac and winNT.

recipe
------
pre-existing conditions: make sure you have the default color prefs set (it'll
make this easier ;). go to http://www.yahoo.com: the background is white, the
text is black, unvisited links are blue and visited links are purple. visit a
couple, if needed, so that some of the links appear as purple/visited.

1. open the prefs dialog and go to the Colors panel.
2. change these color prefs:
  * set Text to yellow
  * set Background to red
  * set Univisited Links to lime green
  * set Visited Links to pink
3. keep the other prefs in this panel unchanged, ie:
  * "system colors" should be off
  * "underline links" should be on
  * "always used the colors and background specified by the web page"
radiobutton should be selected
  * "view source syntax highlighting" should be on
4. click OK to save and dismiss the prefs dialog.

expected: the colors changed in step (2) should immediately be displayed on the
web page.

result: i'll attach a screenshot soon. the result is the same on all platforms:
the background remains white and the text remains black.

workaround: reload the page.

Comment 1

[sairuh (rarely reading bugmail)]

in speaking w/marc over email, he said that this is a regression...

Comment 2

[sairuh (rarely reading bugmail)]

Attachment: shot showing incorrect display after changing color prefs

Comment 3

[sairuh (rarely reading bugmail)]

Attachment: shot after reloading page, showing correct color pref changes for comparison

Comment 4

[Marc Attinasi]

One interesting thing is that the link colors are changed. I think that the 
problem has to do with the body fixup rule not getting updated with the new 
colors. I'll create a testcase for that now, but I'm pretty sure that is the 
problem, and I think Hyatt has a bug on it too...

Comment 5

[Marc Attinasi]

Accepting: this is pretty important I think

Comment 6

[David Hyatt]

Yeah, I'm sure this is related to my CSS landing.

Comment 7

[David Hyatt]

I think this is a separate bug from the body fixup rule issue... you have to
blow away more than that (e.g., tables also reset to document color and link
color in quirks mode).

My guess is that this method right now calls ReconstructFrames or some such and
that we should just ensure that the entire rule node tree gets blown away in
this case so that all the style data can just be recomputed.

Comment 8

[Marc Attinasi]

I've been toying with this for an hour now and it is not as easy as I had hoped ;)

First I tried to clear the rule tree in the StyleSet where we clear the rule
processors (nsStyleSet::NotifyStyleSheetStateChanged or
nsStyleSet::ClearRuleProcessors) but that is bad because there are still frames
around, and they reference the ruleNodes which get destroyed out from under them
(and it does crash).

So, I next tried to clear the rule tree in a deferred manner by setting a flag
on the styleSet such that the next time the rule tree is accessed for style
resolution, it is first torn down. This causes a crash due to the fact that
there are still frames and style contexts around, and there are references to
ruleNodes in the existing style contexts (for the root scroll frame).

Next I tried a similar thing, but put in a call to clear the rule tree in the
CSSFrameConstructor method ReconstructDocElementHierarchy, however the same
problem occurs: namely, there are style contexts around for the root scroll
frame and they end up having stale rule nodes that get accessed as the
doc-element's frame is styled.

Basically, we cannot clear the rule tree unless ALL frames are destroyed first,
and this means not just reconstructing the doc-element hierarchy as we do now,
but tearing down the root scroll frame as well. This is currently done in the
presShell::InitialReflow only, and I'm a bit leary of changing this. It seems to
me it is just easier to cause a document reload! OK, so maybe that is a bad
idea. I'll try rebuilding the root scroll frame in
ReconstructDocElementHierarchy too since it appears to be pretty simple to do.

On another note: it seems that we really do need to clear the rule tree whenever
the rule processors are cleared, but given the references to the rule nodes in
the frames and the style contexts, I don't see how this can happen without
tearing down all of the frames first. This may not be a problem, but we
currently nuke the rule processors  when a new style sheet is added or removed
(and we are not managing the rule tree there)...

Comment 9

[Marc Attinasi]

Oye Vey! What a can of worms. Just try tearing down the root scroll frame and
see where that gets ya. 

It seems that we don't have any support in the system for doing that, and after
making little changes in FrameManager so that I can clear the root frame after
it has been set, lots of other whacky stuff starts asserting, and in the end it
only partially works. 

I have a short-term fix for this: just issue a Reload instead of a reframe,
after the prefs are changed. It certainly fixes the bug, but it has some
drawbacks (like, if you are on a non-cachable page it has to re-fetch it).

The basic problem I have run into is this: we cannot clear the rule tree unless
we first destroy all of the frames, and we cannot destroy all of the frame
without essentially tearing down the docViewer. This is all new territory for
me, so I'm not sure how hard I can beat on it.

It is bothersome that we cannot rebuild the rule tree without first tearing down
every frame, and that we cannot (readily) tear down every frame... Somebody tell
me I'm missing something, please :)

Comment 10

[Marc Attinasi]

Attachment: Hack-On-Crack to reload the document after a pref change (just shoot me, please)

Comment 11

[Marc Attinasi]

I've spent several hours trying to find a good way to tear down all of the
frames so that the rule tree can be cleared and rebuilt after the pref is
changed. The problems are many. The biggest (last) problem is that destroying
the root frame causes the root view to be destroyed too, and re-creating the
root view requires also creating a widget - stuff done currently in the
nsDocumentViewer::MakeWindow. There must be a better way, as tearing down the
root view and having to recreate it (and the native widget) seems like way too
much just to tear down all of the frames... Also, it may be better to find a way
for the rule tree to partially invalidate itself so that the root frame does not
have to be destroyed.

Over to Hyatt in hopes that he has a better approach - he certainly has more
experience in this area than I do! (let me know if I can help in any way)

Comment 12

[David Hyatt]

One idea that might naturally fix this (that I don't really like, but here goes)
is to remove rule nodes from the tree when they have no external observers any
more.  Then when a frame tree gets destroyed, you'd naturally tear down all of
the rule nodes that no longer have any external style contexts referencing them.

The only down side I can think of to this is you lose the caching of :hover and
:active states, which was a UI perf improvement.  For example, the rule tree
would remember all the states of the Back button after you'd pressed it, so that
the next time you hovered or pressed it, you'd resolve style a little bit faster.

Comment 13

[Marc Attinasi]

I like caching - it has helped us performance-wise quite a bit. Maybe instead of
always tearing down the rule nodes when a frame is destroyed, we can first tear
down the frames we want to get rid of (the DocElementHierarchy in this case) and
then make a pass thought the rule tree and purge any nodes that are no longer
observed. A simple 'Prune' method on  the RuleNode might take care of this- ok
myabe it is not totally simple since the tree of nodes would have to be
restructured, but it sounds possible, and it would only happen when we needed it
to instead of whenever a frame is destroyed.

Comment 14

[David Hyatt]

Attachment: Patch to invalidate appropriate cached data in style and rule trees

Comment 15

[David Hyatt]

Attachment: Better patch. Duh, the prescontext is passed in already.

Comment 16

[Marc Attinasi]

What a great fix! sr=attinasi

Comment 17

[Chris Waterson]

r/sr=waterson

Comment 18

[David Hyatt]

Fixed.

Comment 19

[Jacob Kjome]

Is this fixed on the Branch?  That is, is it going to make it into Netscape 6.1?

Jake

Comment 20

[Terri Preston]

Verified Mac commercial build 20010717
Verified W2k commercial build 20010717
Verified linux commerical build 20010717

Comment 21

[Terri Preston]

Duh, I lied, background was changing :-(  Reopening and will test on tomorrow's
build

Comment 22

[John Morrison]

resetting to fixed, pending verification with a build that actually 
has the fix :-]

Comment 23

[Daniel Bratell]

hyatt: "Backing out 90081 fix until I understand why opt builds only crash." 

Comment 24

[Jacob Kjome]

The patch for this bug fixed this issue and a few others ( such as bug 
87485 ).  Too bad it caused Mozilla to crash after a bit.  What is the status 
here?  I would hope this could go back in before the freeze (from what I here) 
tomorrow night.  Stuff like this just *has* to be in Netscape 6.1 otherwise a 
lot of DHTML is unusable.

Jake

Comment 25

[David Hyatt]

The only DHTML that is broken is manipulation of rules in stylesheets (inline
style works fine).  Internet Explorer does not even support this feature, so
there aren't many real-world Web pages that are going to be broken if this
doesn't get fixed for NS6.1.  

That said, I am working on a fix right now, but convincing PDT to take might be
difficult, since my first attempt caused serious instability.

Comment 26

[David Hyatt]

Working on this for 0.9.3 actually.

Comment 27

[David Hyatt]

Here comes a brand new patch.  Much more involved. :)

Comment 28

[David Hyatt]

Attachment: Patch to content DLL

Comment 29

[David Hyatt]

Attachment: Patch to layout DLL

Comment 30

[David Hyatt]

This is ready for review.

Comment 31

[Marc Attinasi]

David, what is the impact of leaving these in:

+        // XXXdwh figure this out.
+        // oldContext->RemapStyle(aPresContext, PR_FALSE);

???

Comment 32

[David Hyatt]

RemapStyle is commented out currently on the trunk.  I don't think these cases
are actually hit now, but I wanted to leave the // XXXdwh in case I'm wrong to
remind myself to investigate further.

Comment 33

[Marc Attinasi]

Looks sane to me :)  [s]r=attinasi

Comment 34

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

r=dbaron, although I suspect the O(N^2) nature of the clearing will bite us in
some case or other.

Comment 35

[David Hyatt]

There are many clever things we can do to improve perf here.  I hope to tackle
many of these DHTML issues for 1.0.

Comment 36

[David Hyatt]

Fixed on trunk.

Comment 37

[sairuh (rarely reading bugmail)]

hrmmm, ran into some issues while verifying this. the Colors prefs still don't
seem to work as expected. i'm gonna reopen this for the nonce. however, if what
describe below should be filed as new, seperate issues do let me know and i'll
re-resolve this one...

tested on linux, 2001.08.15.14-comm. will test other platforms soon.

a. i realized that http://www.yahoo.com defines a color for text, so the Text
choice with the original recipe doesn't work. i did create a simple testcase
where there text doesn't have a page-defined color, which i'll attach shortly.

b. in spite of (a), testing with with original recipe at http://www.yahoo.com
works fine now for Background, Visited Links and Unvisited Links colors. i don't
need to reload to see my changes after saving/dismissing the prefs dialog.

c. [1st bug] before i created the simple test case, i went to a directory
listing to test out the recipe for Text color:
http://jrgm.mcom.com/bugs/mimetypes/ --under the assumption that it'd display
like a simple html page without any page-specified colors. using the original
recipe, everything seemed to work --except for the Text color. i had to reload
the page in order to see the newly-applied Text color.

d. [2nd bug] with the soon-to-be-attached testcase, everything worked *except*
that whichever links i visited never got the visited color! strange, since (a)
at http://www.yahoo.com displayed visited links with the appropriate color...
not sure why that'd be happening --ideas, anyone?

Comment 38

[sairuh (rarely reading bugmail)]

Attachment: simple HTML file w/o defined colors

Comment 39

[sairuh (rarely reading bugmail)]

nominating for 0.9.4.

Comment 40

[sairuh (rarely reading bugmail)]

*sigh* correction about (d) above: as in (c), the Text color didn't immediately
update until i reloaded the page. but i still didn't see the visited color
applied at all in my sample page [weird, since visited colors appeared fine at
both http://www.yahoo.com and http://jrgm.mcom.com/bugs/mimetypes/].

and i see the same issues using 2001.08.15.06-comm bits on winNT and
2001.08.15.08-comm bits on Mac OS 9.1 [emul on X].

e. [3rd bug] the only platform diff i've noticed [yet another color
strangeness], is that on Mac, the "Category" text above the prefs category tree
appeared in yellow [rather than staying black], when reopening the prefs dialog
after going thru the original recipe.

Comment 41

[Marc Attinasi]

The link colors are not changing because of good ol' bug 12493 (my favorite bug
to hate). You need to put the URLs in the canonical format in the markup (eg.
"http://www.yahoo.com/" to get them to match what the history thinks).

Comment 42

[Marc Attinasi]

Also, most of the text on www.yahoo.com is not explicitly colored - I just
tested it and (though I have to reload) the text color is changing (there is
amazingly little text on that page that is not a link, but there is some).

I do not see the quirky Mac behavior myself, but I am not sure exactly how you
got that. It sounds like another bug anyway since the colros int eh chrome
should not be changing according to the color pref anyway (or am I missing
something?).

Comment 43

[sairuh (rarely reading bugmail)]

sorry about the delay in my response --i've filed bug 97534 for the weird
"Category" text color change on Mac.

marc, thx a bunch for referring me to bug 12493!

btw, how's the progress on the remaining issue here, ie, Text color not
immediately displaying unless you reload?

Comment 44

[sairuh (rarely reading bugmail)]

*** Bug 51066 has been marked as a duplicate of this bug. ***

Comment 45

[Peter Trudelle]

nsbeta1-, cc tpringle.

Comment 46

[R.K.Aa.]

*** Bug 185987 has been marked as a duplicate of this bug. ***

Comment 47

[Sven Grull]

This is strange, bug 185987 was filed december 2002, this bug is much older and
I never ran into this problem before december last year.
As I found out a while ago the same problem appears when changing between
"underline links" and "do not underline links". Is this also covered by this bug?

Comment 48

[Jo Hermans]

*** Bug 205118 has been marked as a duplicate of this bug. ***

Comment 49

[Eli Friedman]

This appears to be working; can anyone still reproduce?

Comment 50

[Serge Gautherie (:sgautherie)]

David,
Are you still working on this ?

Comment 51

[Boris Zbarsky [:bzbarsky]]

Marking worksforme until someone can reproduce.

Comment 52

[dentaku]

Well, I have Firefox 22 (latest) and when I change the colors, I have to reload the page for the page to use the new colors ... => doesn't work for me either.