Closed Bug 315920 Opened 14 years ago Closed 13 years ago

[FIX][SELECTORS-DYNAMIC]attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: dbaron, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: css3, Whiteboard: qa: please see comments 10, 11, 12)

Attachments

(6 files, 6 obsolete files)

2.20 KB, text/html; charset=UTF-8
Details
96.32 KB, patch
sicking
: superreview+
Details | Diff | Splinter Review
1.20 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
39.17 KB, application/zip
bzbarsky
: review+
Details
16.09 KB, application/zip
Details
3.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
The attribute and event state change optimizations in nsCSSStyleSheet assume that changes to attributes and event states are sequential and that notification for a change happens before any other changes have happened.  This assumption has been broken by a number of nsIContent::IntrinsicState methods that make an intrinsic content state equivalent to an attribute.

Steps to reproduce:
 1. load attached testcase
 2. press (alternately) the buttons to enable/disable option one (or option three)

Expected results:  Option two or four should change state as described, corresponding to the changes in the state of option one or three.

Actual results:  Once options two or four get a background color, they always keep it, unless the "Restyle all options" button is pressed.
Summary: attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential → [SELECTORS-DYNAMIC]attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential
I was just thinking about this earlier this morning.  I _think_ we could fix this if ContentStatesChanged also took an "attribute responsible for the change" parameter (and then HasStateDependentStyle passed both that attribute and the state mask to SelectorMatches).

But perhaps we should find some other way in general...
FWIW, this bug is based on the discussion from bug 98997 comment 46 through bug 98997 comment 55.
Flags: blocking1.9?
Another possibility, more complicated, but potentially making it near-impossible to screw up notifications:

1) Wrap the body of SetAttr in a Begin/EndUpdate (possibly of a new ATTRCHANGE
   type?)
2) While inside such an update, record state bits and attr atoms/namespaces
   that change for whatever nodes they change in in the frame constructor.
3) When the update ends, check for state and attribute dependent style (in a
   single call, probably passing in the state bitmask and an array of attribute
   names).
4) Possibly optimize for one attr, some states, all on a single element, or at
   most 2 elements (this should cover all our use cases, I believe).

If we don't do something like this, then we have to make sure that any time several content states all change (e.g. :checked and :default both changing when "type" changes as in bug 348809) we notify on all the states at once.  If some states are handled by subclasses and some by superclasses this is kinda hard...
Flags: blocking1.9?
Whiteboard: [wanted-1.9]
For what it's worth, I think we should either do this for 1.9 or disable a bunch of the CSS3 pseudo-classes we've added....
Flags: blocking1.9? → blocking1.9+
Whiteboard: [wanted-1.9]
So I think we should do something like what I propose in comment 4, but a little simpler:

1)  nsGenericElement::SetAttr calls IntrinsicState() up front and saves the
    value.
2)  Do the actual attr-setting.
3)  Call IntrinsicState() again.
4)  Send a single AttributeAndContentStatesChanged() notification or
    something. The important part is for the style matching code to know
    they all changed at once.  Pass the xor of the values from step 1 and
    step 2 to this.

I'm not happy with that notification name, and I'm not sure what to do with existing consumers.  Maybe we should just add an atom to ContentStatesChanged and a state mask to AttributeChanged and send ContentStatesChanged only if the state mask ended up nonzero...
Actually, all we _really_ need is to pass the state mask to AttributeChanged().  The frame constructor can then handle calling whatever style system APIs are needed.
Boris, since you seem to know what to do here, could you take this one?
Assignee: dbaron → bzbarsky
Yeah.  I'll try to get to this for 1.9, but it'll be tight.  I could really use some good tests for this stuff, if anyone knows people who're interested in writing such.
QA Contact: ian → style-system
Testing notes:

1) Need to test various combinations of :disabled and [disabled]
2) Need to test combination of :default and [checked] for radios and checkboxes
3) Need to test various image states for <img>, <input type="image">,
   <svg:image>, <object> images.
4) Test generated content images.
5) Test that bug 348809 does not regress.
6) Test that bug 302186 does not regress.
7) Test handling of the "type" attribute of nsHTMLInputElement

When testing these, also test handling of '+' and '~' combinators, since some of these changes (e.g. type attributes of input elements) will reframe the target node no matter what.
More testing notes:

8) Test disabled/enabled/default states for <option>s (mapped to the "disabled"
   and "selected" attributes).
9) Special attention to the combination of :default and "type" attribute changes
   on HTML inputs.
10) Make sure we don't regress bug 306614.
11) Test that bug 84400 does not regress.
Attached patch Proposed patch (obsolete) — Splinter Review
This fixes the testcase, and doesn't regress our existing regression tests...  It's a little suboptimal because we'll call HasStateDependentStyle() twice when an attr change changes state -- once from the ContentStatesChanged notification, and once from the AttributeChanged notification.  Unfortunately, we have listeners other than the frame constructor watching ContentStatesChanged.  :(  Not sure what the best way to resolve this is, without lots more surgery.
Attachment #267245 - Flags: superreview?(jonas)
Attachment #267245 - Flags: review?(dbaron)
Oh, and that patch still needs a _lot_ more testing.
Summary: [SELECTORS-DYNAMIC]attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential → [FIX][SELECTORS-DYNAMIC]attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential
Target Milestone: --- → mozilla1.9alpha6
Actually, the redundancy problem might be solvable without too much trouble.  I'll give it a shot tomorrow.
So the basic idea is that the existing HasStateDependentStyle will catch all the state selectors that do not also select on the attr in question, and HasAttributeDependentStyle will ignore all the states involved as needed.
Attachment #267245 - Attachment is obsolete: true
Attachment #267289 - Flags: superreview?(jonas)
Attachment #267289 - Flags: review?(dbaron)
Attachment #267245 - Flags: superreview?(jonas)
Attachment #267245 - Flags: review?(dbaron)
Comment on attachment 267289 [details] [diff] [review]
Without the redundant HasStateDependentStyle call

>+   *                   Note that this is a PRUint32, not PRInt32 like the mask
>+   *                   for *nsIDocumentObserver::ContentStatesChanged, since
>+   *                   this is really a bitmask *and should be unsigned.

Is this even true?  It doesn't particularly make sense to me.

> nsHTMLStyleSheet::HasAttributeDependentStyle(AttributeRuleProcessorData* aData,
>                                              nsReStyleHint* aResult)
> {
>+  // Note: no need to look at the stat changing because we handle
>+  // that under HasStateDependentStyle() as needed.
>+

s/stat/state/.  And I don't think I really understand the comment, either.

> PresShell::ContentStatesChanged(nsIDocument* aDocument,
>                                 nsIContent* aContent1,
>                                 nsIContent* aContent2,
>                                 PRInt32 aStateMask)
> {
>   NS_PRECONDITION(!mIsDocumentGone, "Unexpected ContentStatesChanged");
>   NS_PRECONDITION(aDocument == mDocument, "Unexpected aDocument");
> 
>-  WillCauseReflow();
>-  mFrameConstructor->ContentStatesChanged(aContent1, aContent2, aStateMask);
>-  VERIFY_STYLE_TREE;
>-  DidCauseReflow();
>+  if (mDidInitialReflow) {
>+    WillCauseReflow();
>+    mFrameConstructor->ContentStatesChanged(aContent1, aContent2, aStateMask);
>+    VERIFY_STYLE_TREE;
>+    DidCauseReflow();
>+  }

Is mDidInitialReflow really the right test?  It still means that we haven't constructed frames?
Comment on attachment 267289 [details] [diff] [review]
Without the redundant HasStateDependentStyle call

>Index: accessible/src/base/nsDocAccessible.cpp

>+  if (aStateMask & NS_EVENT_STATE_CHECKED) {
>+    nsHTMLSelectOptionAccessible::SelectionChangedIfOption(aContent);
>+  }

If this is needed, shouldn't it be in a ContentStatesChanged rather than AttributeChanged?

r=dbaron (see previous comments as well)
Attachment #267289 - Flags: review?(dbaron) → review+
> Is this even true?  It doesn't particularly make sense to me.

Well.  It's a bitmask.  I think it should be unsigned everywhere.  I suppose I could make it PRInt32 here for now too and then eventually we'd change it globally, sometime.

> s/stat/state/.  And I don't think I really understand the comment, either.

The point was that we don't have to worry about whether some states changed with this attribute too, since this code doesn't rely on optimizing away changes the way nsCSSRuleProcessor does. I'll try to make this clearer.

> Is mDidInitialReflow really the right test?  

Yeah.  It still means we haven't constructed frames.

> If this is needed, shouldn't it be in a ContentStatesChanged

Yep.  It's not, in fact, needed.  It's a holdover from an earlier version.
> Is this even true?  It doesn't particularly make sense to me.

I'm just going to take that bit of comment out, I think.
Attached patch Updated to comments (obsolete) — Splinter Review
Attachment #268582 - Flags: superreview?(jonas)
Attachment #267289 - Attachment is obsolete: true
Attachment #268582 - Attachment is obsolete: true
Attachment #268592 - Flags: superreview?(jonas)
Attachment #267289 - Flags: superreview?(jonas)
Attachment #268582 - Flags: superreview?(jonas)
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Note to self: since the patch for bug 237964 uses a bit instead of the raw attribute value for form control read-write/read-only state, I don't think I need to update this patch to take that one into account...
Keywords: qawanted
Whiteboard: qa: please see comments 10, 11, 12
Attachment #268592 - Flags: superreview?(jonas) → superreview+
Checked in.  ctalbert said he'd take a shot at the tests.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
QA: I'll be working on reftests for the items in 10, 11, and 12.  If you have other test suggestions, please let me know.
Comment on attachment 272168 [details] [diff] [review]
Address bz's question that he added to comment in nsDocAccessible.cpp

r=me, but should aaa:disabled affect the disabled state?  What does the ARIA spec say as far as CSS matching goes for :disabled when that attribute is used?
Attachment #272168 - Flags: review?(bzbarsky) → review+
I'll have to look where it says in the spec, but basically, ARIA roles and properties do not affect the behavior of a web browser. It only is only used by the Javascript author to provide additional information about there custom controls, so that assistive technologies know what is happening.

The reason for this is backward compatibility. ARIA can be used to make existing Javascript widgets and applications accessible -- they may run in any browser including IE. Adding additional browser functionality in terms of event handling and rendering around it would create new browser differences for Javascript author to deal with.
I see.

In that case, I think it would make most sense for you to look for the disabled state (which can depend on the disabled attribute on the node itself, or on other parts of the DOM; for example in Web Forms 2, all form elements inside a disabled fieldset are automatically disabled, even though they don't have "disabled" attributes) and explicitly look for the ARIA attribute only here.
These are a first pass at the requested tests.  I would like to know if I am testing the proper attributes, if there need to be further tests on these items, and if this approach will also suffice for the later items.

I have specific questions about a few of them:
It appears that setting a selector of the form:
input:default + label {color: green} is not working. Please take a look at bug315920_2d.html and let me know if this is an error with the test or a bug in Minefield.

I think that for the testing of the moz-suppressed and moz-user-disabled selectors, mochitests are better suited for this than reftests.  bug315920_4b.html outlines my approach for this style of test.  Please let me know if that approach is a valid one.

Also, I noticed that if you include a local image in the HTML, then that local image is not replaced when the image is swapped, and therefore the ":-moz-user-disabled" rule is not applied.  Is this an expected behavior or is this a bug?

These are my first attempts at reftests, so if there are any stylistic issues that need addressing, please point them out.
Attachment #273717 - Attachment mime type: application/octet-stream → application/zip
> Please take a look at bug315920_2d.html

That first radio should have a "checked" attribute, no?

I'm not sure what you mean about local image.  Does one of these tests show the issue?

I'll try to look at the rest tomorrow.  Gotta sleep now.
(In reply to comment #31)
> > Please take a look at bug315920_2d.html
> 
> That first radio should have a "checked" attribute, no?

I can verify that adding a "checked" attribute makes the test work.  So, then a rule of the form "input:default + label {style}" will only match if the "checked" attribute is present?  I can verify that the rule will match if the "checked" attribute is present, regardless if there is a default attribute.  This seems unusual to me, but I take it that this is expected behavior?
 
> I'm not sure what you mean about local image.  Does one of these tests show the
> issue?
Test bug315920_4b shows this problem.  Here are the steps so that they are clear, with line numbers referring to the test html file (maybe they will help you spot a bug in my code):
1. Include a valid image link in the markup to a local image file (replace line 44 with an uncommented line 48)
2. Set permissions.default.images to 2 (don't load remote images) (line 28)
3. Change the image source to a remote image (line 37)

= Result =
The remote image does not load, which is correct.  However, the local image is still displayed, and the -moz-user-disabled pseudoclass is not applied so the test rule (line 15) fails to match.

I would expect that the local image would go to some kind of "broken image" image, and that the -moz-user-disabled rule should match.
> 
> I'll try to look at the rest tomorrow.  Gotta sleep now.
> 
Thanks! I appreciate your help very much.

> So, then a rule of the form "input:default + label {style}" will only match
> if the "checked" attribute is present? 

If the input is a radio or checkbox, yes.  For submit and image inputs :default matches if the input is the default submit of the form, etc.  So for purposes of this bug we want to test the combination of [checked] and :default on checkbox and radio inputs, since that attribute and that state are connected for those inputs.

> regardless if there is a default attribute. 

There is no such attribute defined for <input>...  We're not talking about [default] here, right?

> Test bug315920_4b shows this problem.

Oh, I see.  Yeah, if the image is still showing the first thing it loaded the state it matches will be the state for that thing.  So in this case, I think the behavior of the style code is correct.  The behavior of the image itself is at least on purpose (if it's showing a valid image and is asked to show something it's not allowed to load it won't drop the valid image), so let's call it correct for now.  ;)
Depends on: 391994
This is a zip file containing new and updated tests.  Below is the content of the README.txt file contained in this zip archive, which describes the tests. Items marked with *<testnum>* are items about which I have questions.  If you open the referring test, you'll find the question in the top of the file.  (You can also grep the files for "Bz" and get all the questions.

Items 6 and higher have not yet been written/performed.  I wanted to make certain that these tests demonstrate adequate coverage before I attempted to analyze the coverage of existing tests (required for points 6, 7, 9 and 10).

This file was put together by taking each "testing" comment above and writing tests that exercise that comment.  Please let me know if anything is missing.
Here's the contents of the file:
= Tests For This Bug =
* Overall rules for these tests:
** When testing these, also test handling of '+' and '~' combinators, since some
of these changes
** Test disabled/enabled/default states for <option>s (mapped to the "disabled"
   and "selected" attributes).

== Test combinations of :disabled and [disabled]==
1a. Tests a varying the disabled attribute on <option>s
1b. Tests varying disabled attribute using double not([disabled]):not(:disabled)
*1c.* Tests swapping a selected attribute and a disabled attribute on the fly, and that selected removes disabled
1d. Tests using the {disabled] attribute with input type = button
1e. Tests using the [disabled] attribute with input type=submit
*1f.* Tests using [disabled] and :disabled rules when inserting a disabled element into DOM
1g. Tests using [disabled] and [selected] when removing a disabled element from DOM *Question on 1g_ref*

== Test combinations of :default and [checked] for radios and checkboxes==
*2a.* Tests :not(:default) rule on radio input
2b. Tests removing checked and default and setting checked with radio
2c. Tests swapping default attributes with radio buttons and that :default doesn't match if checked is not set
2d. Tests that setting default attribute matches [checked]:default over :default
2e. Tests creating input type=radio elements 
*2f.* Tests removing radio elements that match :default and that the browser reassigns the first element the checked status.
2g. Tests switching checked attributes without changing default attributes.
3a. Tests toggling checked attributes and implicit use of :default 
3b. Tests that :default overrides all other combinations when boxes are checked
*3c.* Tests that removeAttribute for :Default and checked attributes changes style appropriately

== Need to test various image states for <img>, <input type="image">, <svg:image>, <object> images.==
4a. Tests swapping the image to match :not(:-moz-broken) on img
4b. Tests disabling images via preferences and matching :not(:-moz-user-disabled) on img tags
4c. Tests swapping images in order to match :-moz-broken and not(:-moz-broken) on <input type="image">
4d. Same as 4c, except tests using <object type="image/png">
4e. Test interaction between not(:-moz-broken) and :disabled
4f. Test interaction between :moz-broken and :disabled
*4g.* Test disabled and moz-broken with loading background-images on input type=image. 
4h. Test :not:-moz-broken with svg:images
4i. Test :moz-broken with svg images

== Test generated content images ==
5a. Test generated image where generation depends upon a default, checked attribute
5b. Test generated image where generation depends upon a disabled state
5c. Test generated image dependent on a :not:disabled state
*5d.* Test generated image where it depends on the disabled state removing an element to change applicability of ~ and + rules.
*5e.* Same as 5d except have the test based on adding an element
*5f.* Tests generated image load in static page contingent on disabled state 

== Test that bug 348809 does not regress. ==
6. Do these tests pass?
6a. Is there going to be any additions to these tests for this issue?

==  Test that bug 302186 does not regress. ==
7a. Do these tests pass?
7b. Is there going to be any additions to this test suite to address these bugs?

== Test handling of the "type" attribute of nsHTMLInputElement ==
* Special attention to the combination of :default and "type" attribute
changes on HTML inputs.
8a. Test changing type of input element so that a + rule matches over a ~ rule
8b. Test changing type of input element where ~ rule matches over an invalidated  rule.
8c. Test changing type of input element so that the default/checked attribute no longer applies
8d. Test changing type of input element so that the disabled attribute no longer applies.
8e. Test changing type of input element so that default attribute varies and changes generated content
8f. Test changing the type of input element so that selected attribute rules will not apply.
8g. Test changing the type of input element so that selected attribute rules wil apply in combination with disabled attributes
8h. Test that changing type of input element so that the default/checked attribute suddenly matches

== Make sure we don't regress bug 306614.==
9a. Do the existing tests pass?
9b. Is there any need to expand those tests for this issue?

== Test that bug 84400 does not regress.==
10a. Do the existing tests pass?
10b. Is there any need to expand those tests for this issue?
Attachment #273717 - Attachment is obsolete: true
> (You can also grep the files for "Bz" and get all the questions.)

Answers in order:

1c: Unclear.  That's the behavior we _used_ to have.  But bug 391994 changed
    that, I think.  What does IE do?
1f: You're seeing bug 229915.
1g_ref: Can't reproduce the problem.
2a: Radios and checkboxes match :default if they have a "checked" attribute.
    See the mail I sent you on 08/01/2007 (subject "Re: First run at the
    requested tests for bug 315920).
2e_ref: Can't reproduce the problem.
2f: Can't reproduce the problem
3c: Clicking the checkboxes doesn't change whether the [checked] selector
    matches.  The :not(default) part always matches (because the tag name is
    "input"; did you meant :default here?).  In any case, the only selector
    that will change state when you check or uncheck a checkbox with the mouse
    is :checked.  This test doesn't test that selector.  Furthermore, this
    still has "default" attributes, which are meaningless.  Please see the mail
    I mention above.
4e_tester: "input:not(:disabled):not([disabled]) ~ input.test[disabled] + span"
           matches as follows: "input:not(:disabled):not([disabled])" matches
           the first submit input, "input.test[disabled]" matches the second
           submit input.  You might want to break this testcase up into
           multiple tests to reduce the number of inputs around, or add a
           :not(.test) to the first part of that selector.
4g: "input:disable + input" doesn't match the second input, because of the <br>
    in between.
5d: Bug 229915
5e: Bug 229915
5f: "input[disabled]:disabled ~ input + span#span1:after" matches nothing,
    since the <span> that comes after the second input does not have id="span1".
> What does IE do?

We should probably get a bug filed on this, since it's a change in behavior from 1.8...
Boris, I appreciate you taking the time to look at this before you go on vacation.  I'll try to get these changes finalized and the rest of the tests written by the time you return.

My approach has been to try a bunch of changes and then make tests out of the ones that either had an unexpected or different behavior between a build before 315920 and one containing the fix.  I haven't tried to make an exhaustive list of all tests that are possible with this, but to come to some sort of good sample set.  I'd appreciate feedback on whether I'm succeeding or failing in that effort.

Thanks again and have a great trip!

(In reply to comment #36 and #35)
> > What does IE do?
> 
IE 6: selects the "should be green" text, and the "should be blue" text is red (the option:not([selected]) {color: blue} rule is not matching).

IE 7: same as IE 6

> We should probably get a bug filed on this, since it's a change in behavior
> from 1.8...
> 
I'll file a bug on it.

> --- Comment #35 from Boris Zbarsky (gone 9/4-9/11 and 9/14-9/17) <bzbarsky@mit.edu>  2007-08-31 13:37:34 PDT ---
>> (You can also grep the files for "Bz" and get all the questions.)
>
> Answers in order:
>
> 1c: Unclear.  That's the behavior we _used_ to have.  But bug 391994 changed
>     that, I think.  What does IE do?
Answered above, I'll file a bug
> 1f: You're seeing bug 229915.
> 1g_ref: Can't reproduce the problem.
> 2a: Radios and checkboxes match :default if they have a "checked" attribute.
>     See the mail I sent you on 08/01/2007 (subject "Re: First run at the
>     requested tests for bug 315920).
I think I just confused myself on this one.
> 2e_ref: Can't reproduce the problem.
> 2f: Can't reproduce the problem
> 3c: Clicking the checkboxes doesn't change whether the [checked] selector
>     matches.  The :not(default) part always matches (because the tag name is
>     "input"; did you meant :default here?).  In any case, the only selector
>     that will change state when you check or uncheck a checkbox with the mouse
>     is :checked.  This test doesn't test that selector.  Furthermore, this
>     still has "default" attributes, which are meaningless.  Please see the mail
>     I mention above.
Yes, I was trying to do :default not ([default]).  Sorry about that.  I'll correct it.
> 4e_tester: "input:not(:disabled):not([disabled]) ~ input.test[disabled] + span"
>            matches as follows: "input:not(:disabled):not([disabled])" matches
>            the first submit input, "input.test[disabled]" matches the second
>            submit input.  You might want to break this testcase up into
>            multiple tests to reduce the number of inputs around, or add a
>            :not(.test) to the first part of that selector.
I'll try breaking this thing up.  This was something I was using to try to understand the difference in the builds after this fix and before this fix.  (Since I could click on it and see the changes visually).
> 4g: "input:disable + input" doesn't match the second input, because of the <br>
>     in between.
Ah!  Thanks.
> 5d: Bug 229915
> 5e: Bug 229915
> 5f: "input[disabled]:disabled ~ input + span#span1:after" matches nothing,
>     since the <span> that comes after the second input does not have
> id="span1".
Doh! Thanks.



> I'd appreciate feedback on whether I'm succeeding or failing in
> that effort.

Seem to be succeeding.  But note that list of things I mentioned in the mail I sent as ones that should definitely have tests.  I don't see any optgroup tests, for example.

> IE 6: selects the "should be green" text,

Sounds like we'll need to fix that, then.  Would be good to know IE's behavior in general (e.g. whether both options are selected after the set but before the remove, etc).  That should go in the new bug.  Please cc me on it?

I understand your concern.  I'll make sure that those tests are also added to my plan and will continue creating tests.  I have also seen several testcases (for other bugs) that seem to only be exposed when form control elements are encapsulated within HTML tables.  Should I add some extra tests with tables as well?

I have opened bug 395107 for the issue with test 315920_1c.html and CC'd you.

What is the proper procedure/format for submitting reftests into the reftest system?  Do I need to provide them as a patch and request review etc?
Sorry.  I somehow missed comment 39...

I wouldn't bother with tables; they shouldn't be affecting this stuff.  As for how to submit reftests, a diff is ok.  So is just a tarball if that's easier.  And yes, you want to get review, generally.
(In reply to comment #40)
> I wouldn't bother with tables; they shouldn't be affecting this stuff.
Boris, you just made my day! :-)

>  As for
> how to submit reftests, a diff is ok.  So is just a tarball if that's easier. 
> And yes, you want to get review, generally.
> 
These are all new files, so I think I will tar them since a diff isn't going to tell you anything useful.  I have marched through each test and I think I have all the issues addressed from your comments above as well as your email.  I should have a tar ready a little later today after I have analyzed the tests and ensured that there is a good reference for them.
This is a zip file containing the tests. 

They will go into mozilla/layout/reftests/bugs

except for the mochikit test, which will go into mozilla/testing/mochikit/tests/
Attachment #278685 - Attachment is obsolete: true
Attachment #283651 - Flags: review?(bzbarsky)
Attached file Image used in tests
There are two images used in the tests.  I can change the tests to use the images in the reftests directory or we can take these two images.
Attachment #283652 - Flags: review?(bzbarsky)
Sorry, clicked on wrong file.
Attachment #283652 - Attachment is obsolete: true
Attachment #283654 - Flags: review?(bzbarsky)
(In reply to comment #42)
> except for the mochikit test, which will go into
> mozilla/testing/mochikit/tests/

Please don't put tests there; put them near the code being tested (not sure where that is, but definitely not testing/mochitest/tests).  Maybe get feedback from the reviewer on this?
Attachment #283651 - Attachment mime type: application/octet-stream → application/zip
Attachment #283653 - Attachment mime type: application/octet-stream → application/zip
Clint, I can't make sense of the mochikit test.  It doesn't actually do any testin that I can see.  What am I missing?

I'm looking at the rest of the tests again, finally.  Sorry that took so long.  :(
I've checked in the reftests (but not the non-functional mochitest) with the following changes:

* Renamed to better fit with the way reftests are named.
* Dropped test 1c, since I couldn't make sense of it (and more importantly,
  couldn't get it to fail).
* Minor edits to the 1d/1e tests to make it more likely that they will catch
  bugs.
* Added tests to make sure that we apply color styles to buttons,
  options, etc (so we know that the other tests are meaningful).
* Marked failing tests as failing, with the relevant bug#s commented in
  reftest.list.
* Added tests for [disabled]:not(:enabled) in various places.
* Made 2e/2e-ref and 3e/3e-ref actually match (they had different DOMs and
  hence different renderings).
* Made the various image tests actually work (fix the image paths, make sure to
  not stop the test before the images are loaded, add tests for the attribute
  selector and a state selector at the same time, remove unnecessary script in
  some of the references, make the reference and test for 4e actually match,
  etc, etc).
* Dropped the 4f and 4g tests because as far as I can tell what they're testing
  is actually a subset of what the other tests test.
* Left out  the 5* tests for now, because I don't see a sane way to tell
  reftest when the test is done...
* Minor edits to the 8* tests, because some of them seemed to be testing things
  that were not actually being mutated.  We need a lot more tests here; looking
  at the code to see what states depend on type and writing tests for them all
  would be very nice.

Clint, thanks again for writing these up!  It saved me a ton of time!
Flags: in-testsuite? → in-testsuite+
Attachment #283654 - Flags: review?(bzbarsky) → review+
Attachment #283651 - Flags: review?(bzbarsky) → review+
Blocks: 413573
(In reply to comment #48)
> I've checked in the reftests (but not the non-functional mochitest) with the
> following changes:
Boris, Thanks for checking these in and cleaning them up.  This was my first attempt at reftests and I really jumped in at the deep end.
> 
> * Renamed to better fit with the way reftests are named.
> * Dropped test 1c, since I couldn't make sense of it (and more importantly,
>   couldn't get it to fail).
> * Minor edits to the 1d/1e tests to make it more likely that they will catch
>   bugs.
> * Added tests to make sure that we apply color styles to buttons,
>   options, etc (so we know that the other tests are meaningful).
Thanks.  I had just assumed that basic tests like that already existed.

> * Added tests for [disabled]:not(:enabled) in various places.
Ah, yes.  I actually had one test with that selector, and talked myself out of using it.  I can't remember why.

> * Made 2e/2e-ref and 3e/3e-ref actually match (they had different DOMs and
>   hence different renderings).
Yes. Thanks.

> * Made the various image tests actually work (fix the image paths, make sure to
>   not stop the test before the images are loaded, add tests for the attribute
>   selector and a state selector at the same time, remove unnecessary script in
>   some of the references, make the reference and test for 4e actually match,
>   etc, etc).
I didn't realize this needed to be done.  Thanks.

> * Dropped the 4f and 4g tests because as far as I can tell what they're testing
>   is actually a subset of what the other tests test.
Ok

> * Left out  the 5* tests for now, because I don't see a sane way to tell
>   reftest when the test is done...
Should I file a Testing follow-on bug for this?  Perhaps we need to augment reftest?

> * Minor edits to the 8* tests, because some of them seemed to be testing things
>   that were not actually being mutated.  We need a lot more tests here; looking
>   at the code to see what states depend on type and writing tests for them all
>   would be very nice.
I filed follow-on bug 413573 for extending these types of tests.

From Comment 47...
The Mochikit test was to test the matching of the -moz-user-disabled selector.  In order to test that, the test attempted to set the user preferences to not allow the loading of remote images.  My thought was that setting the preference (turning off remote images) would cause a reflow that would make the img:-moz-user-disabled selector match, changing the text color in the label following the image.

Admittedly, this is a bit far-fetched.

> Clint, thanks again for writing these up!  It saved me a ton of time!
> 
You're quite welcome. Thank you for your patience.  I hope that next time it will be a quicker, smoother process.
Keywords: qawanted
Yeah, jumped in at the deep end is about right.  :)

> Should I file a Testing follow-on bug for this? 

The issue, as I recall, is that there is no way to know when the image has finished loading....  I suppose one might be able to do it with UniversalXPConnect and watching the nsIWebProgress here directly.  If you'd be willing to file a bug to get a test with that sort of thing going here, that would be great.

As far as the mochitest goes, I think the value of the preference affects pages that load after the preference has been set, but not existing pages.  Makes it a pain to test that stuff, for sure.  Might be worth a separate bug there too; I'm not quite sure how to test it offhand.

And again, apologies that this took so long on my end.  Things kept coming up.  :(
You need to log in before you can comment on or make changes to this bug.