Summary: | Fix test cases that are now spewing console errors that they are not testing for | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, aroben | ||||||||
Priority: | P2 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
Sam Weinig
2007-02-03 15:26:45 PST
Created attachment 12903 [details]
patch
Comment on attachment 12903 [details]
patch
This is an awesome patch. Some of the expected results seem a bit questionable, though.
-SUCCESS - Didn't crash
+SUCCESS
- <div>SUCCESS - Didn't crash</div>
+ <div id="result">SUCCESS - Didn't crash</div>
The two above changes don't seem to match up. Why is " - Didn't crash" not appearing in the expected results?
+caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of child 0 {HTML} of document
If the caret is inside the <input>, won't it have a focus ring (which is what this test says shouldn't happen)?
+try { [2, 5, 9].forEach(printEltAndException); }
+catch (e) { print(e); }
Can you add newlines here as if it were an if-else block?
+try { [2, 5, 8, 1, 4].some(isBigEnoughAndException); }
+catch (e) { print(e); }
+
+try { [12, 5, 8, 1, 44].some(isBigEnoughAndException); }
+catch (e) { print(e); }
Ditto.
-caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+selection end: position 0 of child 5 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
Why are we ending up with a selection now?
(In reply to comment #2) > (From update of attachment 12903 [details] [edit]) > This is an awesome patch. Some of the expected results seem a bit > questionable, though. > > -SUCCESS - Didn't crash > +SUCCESS > > - <div>SUCCESS - Didn't crash</div> > + <div id="result">SUCCESS - Didn't crash</div> > > The two above changes don't seem to match up. Why is " - Didn't crash" not > appearing in the expected results? It now displays only 'SUCCESS' because the way the patch was set up was to call the js which called document.getElementById('result').innerHTML = 'SUCCESS'; Before, the id was not set on the div, so this call could not be completed. I could get rid of this all together and the output would not change. > +caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of > child 0 {HTML} of document > > If the caret is inside the <input>, won't it have a focus ring (which is > what this test says shouldn't happen)? I'm not exactly sure what is happening here (I need to investigate it further) but this is not clicking inside the <input>. If it was, with a eventSender.mouseMoveTo(100, 45) like its sister test input-text-click-inside.html, the png would show focus ring. As I said I'm not exactly sure what the caret: ... syntax means in this case. > +try { [2, 5, 9].forEach(printEltAndException); } > +catch (e) { print(e); } > > Can you add newlines here as if it were an if-else block? > > +try { [2, 5, 8, 1, 4].some(isBigEnoughAndException); } > +catch (e) { print(e); } > + > +try { [12, 5, 8, 1, 44].some(isBigEnoughAndException); } > +catch (e) { print(e); } > > Ditto. I did it in this style to be consistent with the rest of the test. I will change it. > -caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of > child 1 {BODY} of child 0 {HTML} of document > +selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7 > {DIV} of child 1 {BODY} of child 0 {HTML} of document > +selection end: position 0 of child 5 {DIV} of child 7 {DIV} of child 1 > {BODY} of child 0 {HTML} of document > > Why are we ending up with a selection now? > We are selecting the third table element. At first I thought that was correct, but now I can't see why. Will look into it further as well. (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 12903 [details] [edit] [edit]) > > +caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of > > child 0 {HTML} of document > > > > If the caret is inside the <input>, won't it have a focus ring (which is > > what this test says shouldn't happen)? > > I'm not exactly sure what is happening here (I need to investigate it further) > but this is not clicking inside the <input>. If it was, with a > eventSender.mouseMoveTo(100, 45) like its sister test > input-text-click-inside.html, the png would show focus ring. As I said I'm not > exactly sure what the caret: ... syntax means in this case. I talked to mitz on IRC about this and he says this is the correct behavior. This indicates that the caret is not inside the input, which the png also shows. > > -caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of > > child 1 {BODY} of child 0 {HTML} of document > > +selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7 > > {DIV} of child 1 {BODY} of child 0 {HTML} of document > > +selection end: position 0 of child 5 {DIV} of child 7 {DIV} of child 1 > > {BODY} of child 0 {HTML} of document > > > > Why are we ending up with a selection now? > > > > We are selecting the third table element. At first I thought that was correct, > but now I can't see why. Will look into it further as well. > I am changing this so that we no longer have a selection by increasing the leapForward time to 1000ms. New patch forthcoming. Created attachment 12918 [details]
updated patch
Comment on attachment 12918 [details]
updated patch
The console error part of this patch looks great. r=me on that.
But the changes inside ksvg2 seem unrelated and probably shouldn't be in this patch.
Created attachment 12926 [details]
updated patch
Oops! I forgot those where in my tree. Removed.
Comment on attachment 12926 [details]
updated patch
r=me
|