Bug 3579 – In-place style-switching is leaving junk behind, messing up styles

• Component: CSS
• Status: RESOLVED FIXED

[Dan Wood]

I am using a javascript style switcher, as described here <http://www.alistapart.com/articles/alternate/
>.  I am seeing problems where switching to a particular stylesheet, and then back to a previously 
working stylesheet, is exhibiting strange display where a previously visible element is now moved up 
too far.

Steps to reproduce.
1. Open index.html reduction test case (attached) in Safari.  The initial stylesheet, "No Parking Anytime", 
is fine ... the title "Announcing Sandvox" looks fine.
2. Switch, using the popup menu, to "Gnarled."
3. Now switch back to "No Parking Anytime."

EXPECTED: the page should look the same as it did before.
ACTUAL: the h2 text and graphic have been moved up substantially, above the webview's bounds

REGRESSION:
* This works fine in FireFox
* Don't need to view "No Parking Anytime" first; if you switch "stylesheet" and "alternate stylesheet" in 
the code to view Gnarled first, it still screws up No Parking Anytime when you switch to that.

NOTES:
* There is no common stylesheet that both designs are using; one should completely cancel out the 
other when you switch.
* The entire page can be viewed in context at <http://wallace.karelia.com/>.  In the full page, you see 
that the h2 element is moved up underneath the div to the "north" of it.
* This problem was demonstrated to Maciej and SuperKevin at WWDC last week.

[Dan Wood]

The reduced test case is available here for download:  <http://forms.karelia.com/KareliaSiteReduced.zip>  
(It's big, but only becuase of the images with the style sheets; I didn't want to pick through them)

[Chris Petersen]

Attached test case reproduces with Safari 2.0 (v412) and TOT webkit.

[Dan Wood]

Here's an additional, non-reduced, test case, which may be related.  Go to http://wallace.karelia.com/ and 
choose "Aqua" theme (if it's not already selected), then Iris Spring.  The site menu (below the flowers) will 
be word-wrapped (slightly, at least -- it should be all on one line in this design.).  Then choose "No 
Parking Anytime" and then back to Iris Spring, it should be OK again.

This one doesn't reproduce as well as the main test case, unfortunately.  But the Iris Spring design 
definitely gets messed with when switching to other designs.

[Eric Meyer]

I've seen this bug in S5.  Try going here:

http://meyerweb.com/eric/tools/s5/s5-intro.html

Once you get there and the slide show come up, hit "C" to reveal the controls (in the lower right-hand 
corner of the screen), then hit "T" to switch to outline view and then "T" again to go back to the slide 
show.  The 'select' menu for navigating the slide show will be moved up from where it was, sitting on 
top of the symbol-links that used to be above it.

Note: this is in 1.3.1 (v312.3.1), OS X 10.3.9.

[Dave Hyatt]

This is likely to be multiple bugs.  Style switching without reloading the page when you do many changes 
is very difficult to get right.

[Dan Wood]

Hmm, the site where I had the non-reduced test case is not there any more.  Let me know if you want me 
to put it back up.  Otherwise, the reduced test case is attached.

[Graham Dennis]

(In reply to comment #6)
Can you put the reduced test case back up? 'gnarled' is no longer an option at http://wallace.karelia.com

[Graham Dennis]

(In reply to comment #3)
The problem with the word-wrapped text in Iris Spring seems to be related to the order in which the 
style sheets are disabled and enabled. If the old style sheet is disabled before the new one is enabled, 
then you get the strange behaviour in 'Iris Spring', however if the new one is enabled before the old one 
is disabled, then it works. This is demonstrated by putting 'No Parking Anytime' above 'Iris Spring' in 
the list, and 'Aqua' below it. A workaround is to rewrite the setActiveStyleSheet function as:
function setActiveStyleSheet(title) {
	var i, a, main;
	for(i=0; (a = document.getElementsByTagName("link")[i]); i++) {
		if(a.getAttribute("rel").indexOf("style") != -1 && a.getAttribute("title")) {
			if(a.getAttribute("title") == title) a.disabled = false;
		}
	}
	for(i=0; (a = document.getElementsByTagName("link")[i]); i++) {
		if(a.getAttribute("rel").indexOf("style") != -1 && a.getAttribute("title")) {
			if(a.getAttribute("title") != title) a.disabled = true;
		}
	}
}

I am doubtful as to whether this will fix the original bug with the h2 text and graphic.

[Dan Wood]

(In reply to comment #7)

I've put up "gnarled" back to wallace.karelia.com so you can now test out the original, non-reduced test-
case.

[Dan Wood]

(In reply to comment #8)

I put an additional popup on wallace.karelia.com to test out Graham's suggested workaround, but it does 
not (as he suspects) fix the problem.

[Graham Dennis]

Created attachment 4977 [details]
patch

The problem was that when the top or bottom margin was changed from positive to
negative (Gnarled has a negative top margin for the h2 element, but Aqua has
0), only one of m_maxTopPosMargin and m_maxTopNegMargin were appropriately set.
This patch makes sure both are set.

[Dave Hyatt]

Comment on attachment 4977 [details]
patch

Great find.  That's the right fix indeed.  Please write a little test case that
dynamically changes the margin from positive to negative so we have a layout
test that covers this.

[Dan Wood]

(In reply to comment #11)

Graham, does that also fix the test case in comment #3?

[Graham Dennis]

Created attachment 4980 [details]
testcase

This page has two buttons, one to set negative margin-top and margin-bottom
values for the second div, one to set positive values. To aid automation, these
functions are called from onLoad.

If they weren't called automatically, then the page originally displays
correctly (with positive margins on the second div). Clicking 'Negative margin'
will change margin-top and margin-bottom to negative values. Currently, this
has the same effect as margin-top and margin-bottom = 0px. Clicking 'Positive
margin' then does nothing.
With the patch, 'Negative margin' then behaves correctly, and clicking
'Positive margin' then returns to the original layout.

[Graham Dennis]

(In reply to comment #13)
No, that appears to be a completely different bug caused by the ordering of enabling and disabling the 
style sheets. I have investigated it, but I'm stumped at present. It will need it's own bug report.

[Graham Dennis]

(In reply to comment #14)
A better check might be to comment out the setTimeout line that calls setPositiveMargin so that only 
changing positive -> negative margins is tested and not positive->negative->positive.

[Dave Hyatt]

Comment on attachment 4977 [details]
patch

r=me

[Darin Adler]

I'd like to land the fix, but I can't get the test case to do anything in Safari or even Firefox, so I'm not 
comfortable checking anything in.

[Dan Wood]

Darin, I'm a bit confused ... were you unable to get the style switcher to work on <http://
wallace.karelia.com/> on either firefox or Safari?  (Perhaps you have javascript disabled?)  Or were you 
unable to *reproduce* the bug on Safari or firefox?

I just ran the latest nightly build and viewed <http://wallace.karelia.com/> ... first with "Gnarled" then 
with "No Parking Anytime" and I confirmed that the problem is still there on TOT.  Doing the same on 
Firefox 1.5, I did not have the problem.

Let me know some more specifics so I can help you bring 'er in for a landing. :-)

[Darin Adler]

I was testing the attached test case.

I need a test case to land with the bug as a layout test.

[Graham Dennis]

(In reply to comment #20)
The test case doesn't look like it does anything in current ToT because the bug is present. Also the 
rendering is incorrect. If patch is fixed, the rendering is different, and the buttons actually work. Because 
of the bug setting the margin to anything between +20px and -20px changes nothing.

I'll attach a second test case (commenting out the line mentioned in comment #16) that tests what 
happens when the margin is changed from positive -> negative, instead of positive -> negative -> 
positive. In this case, the ''positive margin" button will do something the first time, but no more in current 
ToT.

[Graham Dennis]

Created attachment 5044 [details]
testcase 2

Second testcase with setTimeout line commented.
In this patch only changing the margin from positive -> negative is tested
automatically.

[Graham Dennis]

(In reply to comment #14)
I've just realised my explanation of the original testcase was slightly inadequate. My description of the 
testcase was describing what happens if the onLoad="test();" is removed. With it, what is described in 
comment #14 happens automatically, and the buttons appear to do nothing. To see what I described in 
comment #14, one would need to remove the onLoad. I added 'onLoad' to make automatic testing easier.

[Darin Adler]

Here are things that still confuse me:

    1) When I open test case 2 <http://bugzilla.opendarwin.org/attachment.cgi?id=5044> in Safari, without applying the patch, I see what looks like a negative 
margin. That's good because the code sets a negative margin in its load handler. So I don't see how that illustrates the bug in a way suitable for use as an 
automated test.

    2) When I open test case 1 <http://bugzilla.opendarwin.org/attachment.cgi?id=4980> in Safari, without applying the patch, I see what looks like a positive 
margin. That's good because the code sets a negative margin and then a positive margin in its load handler and the timer. So I don't see how that illustrates the 
bug in a way suitable for use as an automated test.

    3) I assume that this works properly in Firefox, so I opened both test cases in Firefox 1.5 for comparison with Safari. In both cases, the blocks show up with 
what look like positive margins, the layout is different than in Safari (more space between elements) and the buttons don't do anything. Do the test cases take 
advantage of something that is not implemented in Firefox? Does it illustrate other bugs in Safari?

    4) The test cases don't have any self-documentation; when I open them they don't say what they should look like or how they should behave. This is important 
for the understanding of people working on the project in the future. Good test cases should describe, within the test page, what to expect.

I do see with both test cases that if I click the positive margin and then the negative margin button in Safari that it doesn't go back to negative margins. So that 
does look like a way to reproduce the bug. But that's a manual test.

Can someone create an automated test for this bug?

[Graham Dennis]

Created attachment 5159 [details]
testcase 3

Darin, thanks for the feedback. This is my first testcase, so I'm still
learning :-)

1) Point taken. It was actually rendering correctly and not displaying the bug.

2) It's not actually displaying with the 20px top and bottom margin that the
code sets. Instead it displays as though the top and bottom margin are 0px.
3) The buttons didn't work in Firefox (I should have tested this) because I was
accessing element.style["margin-top"] and element.style["margin-bottom"], while
these do exist for Safari, only element.style["margin"] (or
element.style.margin) exists in Firefox. So my testcase was taking advantage of
something in Safari.
4) I've tried to rectify this with this testcase. I include the two examples of
what the negative and positive margin cases should look like, as well as
testing both changing the margin from positive -> negative (which works), and
changing the margin from positive -> negative -> positive (which doesn't work).
I have also implemented the testcase in such a way that the buttons work in
Firefox. This bug is not present in Firefox, and so the behaviour after the
patch is the same as Firefox's behaviour.

[Darin Adler]

Comment on attachment 5159 [details]
testcase 3

OK, the dynamic cases look fine and seem to demonstrate the problem perfectly.

The automatic testing does not seem to work. The automatic cases don't seem to
fail or do anything wrong in TOT Safari or in the latest released Safari
either.

So at this point, this is fine for a manual test, but not for the layout tests
directory.

[Darin Adler]

Comment on attachment 5159 [details]
testcase 3

I take back what I said. Test case to be working fine.

[Darin Adler]

Comment on attachment 5159 [details]
testcase 3

I'll land this now (a modified version to fit better in 800x600).

[Darin Adler]

I'll land this patch later tonight or tomorrow.