243748 - Cannot dynamically change (v)linkColor to the same color twice

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P5)

Description

[zug_treno]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040515
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040515

If you want to change the link color with javascript, you cannot change the
color back to an earlier chosen color. The variable document.linkColor does
change every time, but the displayed links do not change appearance. Sometimes
the color will change after randomly trying, but if another color (vlinkColor or
fgColor) is succesfully changed, the link is always painted in the right color
again. Visited links (document.vlinkColor) are affected the same way, but it
seems text (document.fgColor) is not.

Reproducible: Always
Steps to Reproduce:
1. Use a javascript function to change document.(v)linkColor.
2. Change to an already displayed value.
3. Mozilla stops painting the right (v)link color.

Actual Results:  
Linkcolor is not/erratically updated.

Expected Results:  
Linkcolor should change every time.

After checking with older releases, I think this regressed sometime between 1.6
final and 1.7a.

Comment 1

[zug_treno]

Attachment: Testcase

It is possible to see the bug using this html file, just click on the buttons
and the links will stop changing color if you choose a color twice, but the
color codes (#...) will change.

Comment 2

[Martijn Wargers (dead)]

Confirmed.

The testcase does work in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040120
Firebird/0.8.0+

But it doesn't work in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040213
Firebird/0.8.0+

Comment 3

[Boris Zbarsky [:bzbarsky]]

Er.. The testcase certainly doesn't work with a 2004-04-01-08 trunk SeaMonkey
build...

In fact, the last build I have that it works in is 2004-01-30-08.  It fails in
2004-02-01-08.

Sicking landed some bug 195350 in that time period, and that seems like a likely
culprit to me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, I assume what happened is that sicking made the attr code not allocate a
brand new mapped attr set if a matching one exists (though I thought we already
did that...)

Anyway, the upshot is that the body element sets the link colors only when its
display struct is being resolved, and if we change to a rule that already exists
then we may not have to resolve a new display struct.

So the sheet really needs to be updated when those attributes are being set. 
Which means that the body should override SetAttr, I guess?  Changing the sheet
from nsHTMLBodyElement::ParseAttribute seems evil.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One other issue... we probably need to reset the sheet on insertion in a
document too (or leave the style resolution hack), since SetAttr shouldn't
change the sheet of the ownerdocument when the body is not in a document, and
inserting the body _should_ change the sheet.  :(

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> OK, I assume what happened is that sicking made the attr code not allocate a
> brand new mapped attr set if a matching one exists (though I thought we 
> already did that...)

I shouldn't have made any dramatic changes that caused this, like changing 
when or how we create new mapped-attrs-maps

My guess is more something along the lines of either a simple bug in the 
changes i did to nsHTMLBodyElement, or that the old code in there went through 
codepaths that did more then what the new code did.

Unfortunatly I won't be able to do any hacking on this until early june

Comment 7

[Jonathan Evans]

Attachment: new test case shows sporadic nature

After the code has cycled back to the first color(orange) it stops but does not
change the color to orange. About 10 seconds it will cycle again but skips 5
colors every time straight to gray the 5th color.

Comment 8

[Akkana Peck]

Another test case, very reproducible: http://shallowsky.com/bugs/change-colors.html

The code works in Safari, Konqueror and older firefoxes.

Is there a workaround, since it doesn't look like this regression is going to be fixed?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Can we just ClearStyleDataAndReflow in the (rare) case that this changes?  That should do the trick...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and as a workaround, you could remove the body from the DOM, call those setters on the document that I mention, and reinsert the body...

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  So I'm willing to try to fix this, but I'd like to get the behavior we want straight first so we only do this once, and I unfortunately don't have the time (or OS access) to test the relevant other UAs (IE, in particular) or in general spec this out.  In particular:

1)  How are multiple <body> tags to be handled?
2)  Does removing a <body> from a document remove the effect of its attributes?
3)  Does removeAttribute do what one would expect?
4)  How does all this interact with the setters on nsIDOMNSHTMLDocument? e.g.
    if I set the color via one of those, then insert a body that sets a
    different color, what happens?  What then happens when I remove that body?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 4)  How does all this interact with the setters on nsIDOMNSHTMLDocument? e.g.
>     if I set the color via one of those, then insert a body that sets a
>     different color, what happens?  What then happens when I remove that body?

For this one I think we should map the nsIDOMNSHTMLDocument properties directly to the attributes on the <body>. And if there is no <body> when those properties are get/set then throw.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We do the former, but not the latter.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker, but it would be great if you could get a fix bz?

> 1)  How are multiple <body> tags to be handled?

Ideally use the first. But if performance might be an issue just use whichever is more performant.

> 2)  Does removing a <body> from a document remove the effect of its attributes?

IMHO yes.

> 3)  Does removeAttribute do what one would expect?

IMHO yes

> 4)  How does all this interact with the setters on nsIDOMNSHTMLDocument? e.g.
>     if I set the color via one of those, then insert a body that sets a
>     different color, what happens?  What then happens when I remove that body?

As answered above.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Should I make bgColor and fgColor also throw, for consistency?  Right now they are silent no-ops if no body.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

The only odd part here is the notifying during Bind/Unbind.  I don't see a good way around that short of putting things off on events and the like... I suppose we could do that, if desired.

I also didn't worry about multiple bodies.  If someone does that, sucks to be them.  Worrying about that would be a pain, basically.

This patch needs a bunch of testing.  In particular, DOM mutations involving moving the body around, parsing, removeAttribute, setAttribute, the document methods, etc, all need mochitests.  I don't really have the time to create those, unfortunately.  So anyone who wants to get this fixed, please help!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274219 [details] [diff] [review]
Possible patch

Actually, the HTML content sink can't deal with SetAttr on the body doing an update, even when aNotify is true.  So this would need more work

I really think that the sink could probably just pass aNotify false for the body attr sets if it hasn't notified on the body yet.  But I really don't have time to mess around with that code.  :(

Comment 18

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 19

[Daniel Bodea [:danibodea]]

This issue no longer reproduces in Windows 10. Closing this old ticket.