Closed
Bug 315920
Opened 19 years ago
Closed 17 years ago
[FIX][SELECTORS-DYNAMIC]attribute and event state change optimizations in nsCSSStyleSheet assume changes are sequential
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
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
Assignee | ||
Comment 2•19 years ago
|
||
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...
Reporter | ||
Comment 3•19 years ago
|
||
FWIW, this bug is based on the discussion from bug 98997 comment 46 through bug 98997 comment 55.
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 4•18 years ago
|
||
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]
Assignee | ||
Comment 5•17 years ago
|
||
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?
Flags: blocking1.9? → blocking1.9+
Reporter | ||
Updated•17 years ago
|
Whiteboard: [wanted-1.9]
Assignee | ||
Comment 6•17 years ago
|
||
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...
Assignee | ||
Comment 7•17 years ago
|
||
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.
Reporter | ||
Comment 8•17 years ago
|
||
Boris, since you seem to know what to do here, could you take this one?
Assignee: dbaron → bzbarsky
Assignee | ||
Comment 9•17 years ago
|
||
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.
Reporter | ||
Updated•17 years ago
|
QA Contact: ian → style-system
Assignee | ||
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
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.
Assignee | ||
Comment 12•17 years ago
|
||
11) Test that bug 84400 does not regress.
Assignee | ||
Comment 13•17 years ago
|
||
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)
Assignee | ||
Comment 14•17 years ago
|
||
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
Assignee | ||
Comment 15•17 years ago
|
||
Actually, the redundancy problem might be solvable without too much trouble. I'll give it a shot tomorrow.
Assignee | ||
Comment 16•17 years ago
|
||
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)
Reporter | ||
Comment 17•17 years ago
|
||
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?
Reporter | ||
Comment 18•17 years ago
|
||
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+
Assignee | ||
Comment 19•17 years ago
|
||
> 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.
Assignee | ||
Comment 20•17 years ago
|
||
> 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.
Assignee | ||
Comment 21•17 years ago
|
||
Attachment #268582 -
Flags: superreview?(jonas)
Assignee | ||
Comment 22•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Assignee | ||
Comment 23•17 years ago
|
||
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...
Attachment #268592 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Comment 24•17 years ago
|
||
Checked in. ctalbert said he'd take a shot at the tests.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment 25•17 years ago
|
||
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 26•17 years ago
|
||
Attachment #272168 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•17 years ago
|
||
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+
Comment 28•17 years ago
|
||
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.
Assignee | ||
Comment 29•17 years ago
|
||
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.
Comment 30•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #273717 -
Attachment mime type: application/octet-stream → application/zip
Assignee | ||
Comment 31•17 years ago
|
||
> 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.
Comment 32•17 years ago
|
||
(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.
Assignee | ||
Comment 33•17 years ago
|
||
> 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. ;)
Comment 34•17 years ago
|
||
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
Assignee | ||
Comment 35•17 years ago
|
||
> (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".
Assignee | ||
Comment 36•17 years ago
|
||
> What does IE do?
We should probably get a bug filed on this, since it's a change in behavior from 1.8...
Comment 37•17 years ago
|
||
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.
Assignee | ||
Comment 38•17 years ago
|
||
> 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?
Comment 39•17 years ago
|
||
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?
Assignee | ||
Comment 40•17 years ago
|
||
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.
Comment 41•17 years ago
|
||
(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.
Comment 42•17 years ago
|
||
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)
Comment 43•17 years ago
|
||
Attachment #283652 -
Flags: review?(bzbarsky)
Comment 44•17 years ago
|
||
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)
Comment 45•17 years ago
|
||
Sorry, clicked on wrong file.
Attachment #283652 -
Attachment is obsolete: true
Attachment #283654 -
Flags: review?(bzbarsky)
Comment 46•17 years ago
|
||
(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?
Assignee | ||
Updated•17 years ago
|
Attachment #283651 -
Attachment mime type: application/octet-stream → application/zip
Assignee | ||
Updated•17 years ago
|
Attachment #283653 -
Attachment mime type: application/octet-stream → application/zip
Assignee | ||
Comment 47•17 years ago
|
||
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. :(
Assignee | ||
Comment 48•17 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Attachment #283654 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #283651 -
Flags: review?(bzbarsky) → review+
Comment 49•16 years ago
|
||
(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.
Assignee | ||
Comment 50•16 years ago
|
||
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.
Description
•