99344 - context selectors in stylesheet trash dom styles

Status: VERIFIED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[alex.dent]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.4+) Gecko/20010911
BuildID:    2001091108

see the testcase.

the inner div is supposed to be either at 0px or 100px, nowhere else. internet
explorer does this right, in mozilla build 2001091108
(tried on mac, have seen it in 2001081504 as well) it goes more and more to the
left. VERRRRY UGLY! this happens when you set the
classname on the outer div and there is a context selector style on the innerdiv
that is triggered by this change. and it does happen as well if you use any
other context selector construct that would do the same, for example it doesnt
work if you use

div[isActive="true"] .trayTitle { cssgoeshere }

and then try setAttribute('isActive', 'true') on the outerdiv. so the bug is at
least not something random but a general logic failure, and
renders context selectors somewhat useless on scripted documents. btw, i have
not tested any other styles than position, but i assume the bug
affects more than just positioning.


Reproducible: Always
Steps to Reproduce:
1. click
2. click again
3. click once more. now you see the bug.

Actual Results:  the innerdiv position is **** up.

Expected Results:  the left position should be either 0px or 100px.

Comment 1

[alex.dent]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

This probably has more to do with offsetLeft being broken for nested positioned
divs.  Also, giving position:absolute without specifying any dimensions seems
like a bad idea....

Comment 3

[Jason Johnston]

It's not strictly an offsetLeft problem because the same exact behavior occurs
when you change  document.getElementById('innerdiv').offsetLeft to
parseInt(getComputedStyle(document.getElementById('innerdiv'),
null).getPropertyValue("left")).

However it's not strictly a style resolution problem either, because the
background-color of the inner div does change correctly. It's just the
positioning that's getting thrown off.

Same thing happens when setting className and setAttribute("class","...")

Comment 4

[Jason Johnston]

Narrowed it down a bit.  Will attach testcase that shows what is happening.

When styles are set via the DOM, it looks like they are not actually being
attached to the element, but instead are attached to the style-resolution-tree
representation of that element.  So when you change an element's position in the
resolution-tree (by changing the className of an ancestor, for instance) it
acquires a whole different set of DOM-specified styles.

The testcase I will attach does the following:
onload:
  sets the parent div's className to "tray"
  sets the inner div's DOM style to "left:100px;border:2px solid red;"
  sets the parent div's className to "trayActive"
  sets the inner div's DOM style to "left:200px;border:2px dotted blue;"

After this, you would expect that the element is at 200px from the left and has
a blue dotted border.  This is the case.  However, subsequently changing the
parent div's className back to "tray" recalls the first set of styles which were
set on the child element when it was at that place in the style tree.

Comment 5

[Jason Johnston]

Attachment: Narrowed-down testcase: shows DOM styles being attached to position in style-resolution tree, insteaad of element.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Assigning to hyatt for investigation

Comment 7

[Jason Johnston]

forgot to mention that this occurs on Win2K and Linux as well.  Platform and OS
should be changed to ALL.

Comment 8

[alex.dent]

nice testcase, jason. much clearer than mine. have set platform/os to all.

Comment 9

[alex.dent]

just found a really old build 2001031608 on one of my computers, and tested it
there. the bug does not exist there, so i suspect it came in with the new style
system. 

Comment 10

[David Hyatt]

Can you tell me what the expected behavior should be in the second testcase
attached above?  Exactly which styles are incorrect and when?

Comment 11

[alex.dent]

i'll answer this for jason, just to get this fixed fast ;-)

the inner div is set to 100px left and a red border onload. at this time the
outer div has the class "tray".

you will not see this, because immediately after this, the outer divs class is
changed to "trayActive" (which activates the context selector ".trayActive
.trayTitle"), and the inner div is set to 200px left and a blue border 
if you click on "first style" then, the outer divs class is set to "tray" again.
that deactivates the context selector ".trayActive .trayTitle" correctly. but
what is not supposed to happen is that the inner div gets the old style (100px
left, red border) again! it should stay at 200px, and have still the blue
border. the only thing that is supposed to change is color and background-color.

Comment 12

[David Hyatt]

Holy cow.  That's really bizarre.  How are those old styles staying in the rule
tree?

Comment 13

[David Hyatt]

cc'ing many others, who should be able to follow this after my style 
presentation this week. ;)

The current code makes an assumption that this nifty test case has managed to 
disprove, namely that an inline style rule is always represented by only one 
rule node in a rule tree.

In this test case, the rule tree looks like this:

B --> C --> S
^
|
A --> D --> S'

Or something like that.  You get the idea.  The S and S' nodes are both 
pointing to the same inline style rule.  Basically what happens is that node S 
gets all the original data.  Then you switch the style context for the inner 
div from the old path (a, b, c, s) over to the new path (a, d, s') and alter 
inline style.  I properly blow away the data at S' and replace it with the new 
data, but S still contains the old data.

Later when you switch back to the (a,b,c,s) path, you end up looking at the old 
data again, since I never blew it off the S node before.

This leads us to a prickly problem, and I'm seeking input from others before 
coming up with a solution.

When inline style data changes, I need to find and blow away the data for a 
given inline style rule S on all inline style nodes that reference that rule.  
Some possible approaches:

(1) Do what I do now for rules in the CSS DOM, namely walk the entire tree.  
This would impact performance (possibly severely with DHTML) without impacting 
footprint.
(2) Maintain a mapping from rule to rule nodes.  This could be done for inline 
style rules only, or it could be done for all rules, depending on how much you 
want to impact footprint.  This would retain the current level of performance 
but would increase our footprint.
(3) Label a rule node that points to an inline style rule with a special bit.  
Label the descendants of that node with that bit as well.  Whenever a style 
context's rule node changes, and if its old rule node had the inline style bit 
set, you blow away the data from the entire branch leading back to the inline 
style rule node (while leaving the rule nodes intact).  The tricky part here 
involves hacking into ReResolveStyleContext (a messy function already).  You 
also lose the intelligent caching of previously seen styles, always 
anticipating that inline style *might* change at some point.

Anyone else have any clever ideas or suggestions?

Comment 14

[David Hyatt]

Note that if the reporter is seeking a workaround for the testcase, change the 
inline style on the inner div *before* you change the class on the outer div.  
Reversing the order of those operations should enable you to bypass the bug.

Comment 15

[Pierre Saslawsky]

I like solution #2 better (maintain a mapping from rule to rule nodes). Doing it 
for all rules would also allow us to put in place some instrumentation and 
debugging tools.

Comment 16

[alex.dent]

i have no deeper knowledge of mozillas inner structure, but i have plenty of ram
and i dont't want mozilla to get slower. therefore i would prefer either method
2 or method 3, depending on what's easier / cleaner to implement and maintain.

Comment 17

[David Hyatt]

Ok, I'm going to code up solution #2 then.  If anyone objects to this approach,
let me know now before I start coding.  I'll give it another day or two before I
start working on this.

Comment 18

[Johnny Stenback (:jst)]

I'm ok with solution #2, but please add this new mapping only to inline style
rules only, if possible. Adding bloat only for instrumentation or debugging
purposes seems silly, if you do that then wrap it in #ifdef DEBUG, or #ifdef
SOMETHINGELSE.

Comment 19

[David Hyatt]

Yes, I'll do it only for inline style rules, since I don't think we want to
incur the footprint bloat for all rules (there are 1100 rules in the chrome, so
that would be pretty substantial bloat to add a mapping for all of those rules).

Comment 20

[David Hyatt]

Need to take some time with this one.

Comment 21

[Boris Zbarsky [:bzbarsky]]

When inline style is being changed by onmouseout handlers and context selectors 
involving :hover are also being used there is no way to affect the order in 
which things happen (see bug 111363)....

Comment 22

[David Hyatt]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Peter Trudelle]

Nav triage team needs info: What top sites does this affect?

Comment 25

[Peter Trudelle]

nsbeta1+ per Nav triage team

Comment 26

[David Anderson]

IMHO, this DOM manipulation is a 'cutting edge' technology, thus I would not
expect many top sites to be implementing the scripts that cause these errors.

However, there is a new generation of sites about to hit the scene as soon as
the common browsers are the new browsers [ Read ns6 replaces ns4 ].

How can the 'new' developers feel confident about implementing cool scripts for
cool, standard-compliant browsers when those browsers are faulty with their new
technology?

I'm a little shocked to be seeing this bug placed on the 'back-burner'.  I'd
assume that a solid manipulable DOM is one of the things that Moz 1 gets right.

Comment 27

[John Morrison]

Being marked 'nsbeta1+' is the opposite of "being put on the 'back-burner'".

Comment 28

[David Anderson]

( Duh! ) I feel like such an idiot.  Oh well, this brings me one step closer to
understanding the bugzilla infrastructure. ( I looked up the keyword, but didn't
understand it. )

On the other hand, thanks, triage team, for giving this bug prominence...

Comment 29

[Peter Trudelle]

ADT triage team needs info: What is the impact of this? Any top sites affected?

Comment 30

[David Snyder]

There are no "top" sites I am aware of (guys?) that run into this but this bug
does and can interfere with the use of a website if it hits this bug.  Case in
point is my site: http://www.snydersweb.com  Without trying my coding ran smack
into it, I tried everything to get around it to no avail.  This bug makes it
difficult, if not impossible, to use my site after running into it.

My guess is down the road, particularly after Mozilla is default with AOL and
some of the marketshare is recovered, more developers working in DHTML are bound
to run into this.

Comment 31

[Boris Zbarsky [:bzbarsky]]

This bug can't affect top sites, because there is no way to have legacy code
that really triggers this bug.  Non-legacy code for top sites is presumably
tested against NS6 and bugs such as this one are worked around.  So the site
works, but the developers curse us.  Old story, repeated all over.

Comment 32

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 33

[David Hyatt]

Attachment: Patch that fixes the bug.

Comment 34

[Chris Waterson]

Comment on attachment 77732 [details] [diff] [review]
Patch that fixes the bug.

r=/sr=waterson. As if I have any clue.

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 77732 [details] [diff] [review]
Patch that fixes the bug.

r=jst for what it's worth.

Comment 36

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

Comment on attachment 77732 [details] [diff] [review]
Patch that fixes the bug.

>+  nsISupportsKey key(aRuleNode->Rule());

I think the keys to this hashtable should be |nsVoidKey| rather
than |nsISupportsKey| -- it will save a bunch of unnecessary
refcounting.  There's no risk of aliasing with deleted rules since
rule nodes own their rules (which is probably unnecessary anyway, except for
pointer aliasing) and we never delete rule nodes.  And if we eventually start
removing rules nodes
from the rule tree, we'll need to be sure to clear this mapping
anyway.  I should add a comment about that to bug 117316.

It would also be nice to use PLDHashTable (and it's probably faster, and
you could use the clearEntry callback rather than the enumerator), but
whatever...

Other than that, r=dbaron

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 77732 [details] [diff] [review]
Patch that fixes the bug.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 38

[David Hyatt]

Fix checked in.  I used nsVoidKey.

Comment 39

[Jason Johnston]

VERIFIED on build 2002040608.  Both testcases work correctly.