Bug 71348 – Make AccessibilityObject::lineForPosition return the correct value for cases where the position is not within the current object.

• Component: New Bugs
• Status: RESOLVED FIXED

[Alice Boxhall]

Restructure AccessibilityObject::lineForPosition to make the logic clearer

[Alice Boxhall]

Created attachment 113278 [details]
Patch

[Alice Boxhall]

Chris, would you be able to have a look at this?

[chris fleizach]

Comment on attachment 113278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113278&action=review

Is this patch changing functionality or just re-structuring. the title of the bug leads me to believe that it will not actually fix anything (bur thither just restructure).

> Source/WebCore/accessibility/AccessibilityObject.cpp:945
> +    // if the position is not in the same editable region as this AX object, return -1

this line should be a complete sentence (capitalization, punctuation)

> Source/WebCore/accessibility/AccessibilityObject.cpp:948
> +        return -1;

why do we return -1 here but 0 if visiblePos,isNull()

> Source/WebCore/accessibility/AccessibilityObject.cpp:951
> +    VisiblePosition currentVisiblePos = visiblePos, savedVisiblePos;

separate lines for separate folks here

[Alice Boxhall]

Created attachment 114392 [details]
Patch

[Alice Boxhall]

Comment on attachment 113278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113278&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:945
>> +    // if the position is not in the same editable region as this AX object, return -1
> 
> this line should be a complete sentence (capitalization, punctuation)

Done.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:948
>> +        return -1;
> 
> why do we return -1 here but 0 if visiblePos,isNull()

This is my attempt to replicate the original intended logic as per http://trac.webkit.org/browser/trunk/LayoutTests/accessibility/textarea-insertion-point-line-number-expected.txt, added in http://trac.webkit.org/changeset/35355.

However, I'm not sure the test as committed in that change tests the right thing; the check at line 40 describes itself as "Not focused on text area line number: "; however, at that point it is in fact focused on a textarea which has 0 lines, and that case returns -1. I rewrote the test to check the case where focus is placed outside of the textarea which is being queried by the AX API, and then rewrote the code to make that case return -1 to keep the test result consistent, assuming that was the intended behaviour. Are you able to tell me whether this is the correct behaviour? I can't follow the radar link so I am unable to see any discussion which may have occurred there.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:951
>> +    VisiblePosition currentVisiblePos = visiblePos, savedVisiblePos;
> 
> separate lines for separate folks here

Done.

[Early Warning System Bot]

Comment on attachment 114392 [details]
Patch

Attachment 114392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10394139

[Darin Adler]

Comment on attachment 114392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:952
> -    VisiblePosition savedVisiblePos;
> +    VisiblePosition savedVisiblePos = 0;

Why this change? What effect does adding "= 0" have?

[chris fleizach]

Comment on attachment 114392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review

> LayoutTests/accessibility/textarea-insertion-point-line-number.html:29
> +                var lineNumber = area1AXUIElement.insertionPointLineNumber;

it looks like there are changes to this layout test that don't actually do anything. i.e.) renaming focusedElement to area1AXUIElement.
those kinds of changes make this patch harder to review because i don't know if you're changing the layout test because its wrong or what.

also, i believe this layout test was functioning correctly before. there was probably a good reason why <textarea> was empty. you're changing what this test does and it seems like we might be losing coverage. i would suggest adding a new test, since it seems like you're trying to fix a bug

[WebKit Review Bot]

Comment on attachment 114392 [details]
Patch

Attachment 114392 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10319179

[Gyuyoung Kim]

Comment on attachment 114392 [details]
Patch

Attachment 114392 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10404149

[Alice Boxhall]

Created attachment 114407 [details]
Patch

[Alice Boxhall]

(In reply to comment #7)
> (From update of attachment 114392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:952
> > -    VisiblePosition savedVisiblePos;
> > +    VisiblePosition savedVisiblePos = 0;
> 
> Why this change? What effect does adding "= 0" have?

Nothing good; side effect of coding while jetlagged and uploading without building first. Fixed.

[Alice Boxhall]

Comment on attachment 114392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review

>> LayoutTests/accessibility/textarea-insertion-point-line-number.html:29
>> +                var lineNumber = area1AXUIElement.insertionPointLineNumber;
> 
> it looks like there are changes to this layout test that don't actually do anything. i.e.) renaming focusedElement to area1AXUIElement.
> those kinds of changes make this patch harder to review because i don't know if you're changing the layout test because its wrong or what.
> 
> also, i believe this layout test was functioning correctly before. there was probably a good reason why <textarea> was empty. you're changing what this test does and it seems like we might be losing coverage. i would suggest adding a new test, since it seems like you're trying to fix a bug

Pulling accessibilityController.focusedElement out into a variable actually does change the logic, as at line 39 accessibilityController.focusedElement is a different element to what it is at line 24.

I made these changes to try and make this test correct in terms of what it says it's testing: the comment "Not focused on text area" suggests that it's intended to test the case where a given textarea's line number is being queried, but the focus is not in that textarea; however, it's actually testing the case where focus is placed in a textarea with 0 lines. If you think that's what it was intended to test, I'm happy to reword the comment and restore the original test (and code) logic. I'm asking for your opinion here as you wrote the original test, and I'm unable to see any discussion that happened on that patch as the radar link is inaccessible to me; the original code was very difficult to follow, so I wanted to make it more explicit before I made the changes in bug 71263.

[chris fleizach]

Comment on attachment 114392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review

>>> LayoutTests/accessibility/textarea-insertion-point-line-number.html:29
>>> +                var lineNumber = area1AXUIElement.insertionPointLineNumber;
>> 
>> it looks like there are changes to this layout test that don't actually do anything. i.e.) renaming focusedElement to area1AXUIElement.
>> those kinds of changes make this patch harder to review because i don't know if you're changing the layout test because its wrong or what.
>> 
>> also, i believe this layout test was functioning correctly before. there was probably a good reason why <textarea> was empty. you're changing what this test does and it seems like we might be losing coverage. i would suggest adding a new test, since it seems like you're trying to fix a bug
> 
> Pulling accessibilityController.focusedElement out into a variable actually does change the logic, as at line 39 accessibilityController.focusedElement is a different element to what it is at line 24.
> 
> I made these changes to try and make this test correct in terms of what it says it's testing: the comment "Not focused on text area" suggests that it's intended to test the case where a given textarea's line number is being queried, but the focus is not in that textarea; however, it's actually testing the case where focus is placed in a textarea with 0 lines. If you think that's what it was intended to test, I'm happy to reword the comment and restore the original test (and code) logic. I'm asking for your opinion here as you wrote the original test, and I'm unable to see any discussion that happened on that patch as the radar link is inaccessible to me; the original code was very difficult to follow, so I wanted to make it more explicit before I made the changes in bug 71263.

I think the intended behavior is test what happens in the first text area when it no longer has focus.
The test is wrong is that it's clearly getting the insertion point for the second text area.

Calling area1.selectionStart = 0 defeats the purpose of the test in that sense.
There's also no reason to add text inside of text area 2, since that content is never being tested.

[Alice Boxhall]

(In reply to comment #14)
> (From update of attachment 114392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review
> 
> >>> LayoutTests/accessibility/textarea-insertion-point-line-number.html:29
> >>> +                var lineNumber = area1AXUIElement.insertionPointLineNumber;
> >> 
> >> it looks like there are changes to this layout test that don't actually do anything. i.e.) renaming focusedElement to area1AXUIElement.
> >> those kinds of changes make this patch harder to review because i don't know if you're changing the layout test because its wrong or what.
> >> 
> >> also, i believe this layout test was functioning correctly before. there was probably a good reason why <textarea> was empty. you're changing what this test does and it seems like we might be losing coverage. i would suggest adding a new test, since it seems like you're trying to fix a bug
> > 
> > Pulling accessibilityController.focusedElement out into a variable actually does change the logic, as at line 39 accessibilityController.focusedElement is a different element to what it is at line 24.
> > 
> > I made these changes to try and make this test correct in terms of what it says it's testing: the comment "Not focused on text area" suggests that it's intended to test the case where a given textarea's line number is being queried, but the focus is not in that textarea; however, it's actually testing the case where focus is placed in a textarea with 0 lines. If you think that's what it was intended to test, I'm happy to reword the comment and restore the original test (and code) logic. I'm asking for your opinion here as you wrote the original test, and I'm unable to see any discussion that happened on that patch as the radar link is inaccessible to me; the original code was very difficult to follow, so I wanted to make it more explicit before I made the changes in bug 71263.
> 
> I think the intended behavior is test what happens in the first text area when it no longer has focus.

I agree: that's not what the original test is testing, which is why I made these changes.

> The test is wrong is that it's clearly getting the insertion point for the second text area.

Yes: this is why I made the change to pull area1AXUIElement out into a variable.

> Calling area1.selectionStart = 0 defeats the purpose of the test in that sense.

I don't follow. Calling area1.selectionStart does not put focus back on to area1.

> There's also no reason to add text inside of text area 2, since that content is never being tested.

Ok, I will remove that text.

[chris fleizach]

Comment on attachment 114407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114407&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:947
> +    if (!containerNode->containsIncludingShadowDOM(node()) && !node()->containsIncludingShadowDOM(containerNode))

this doesn't check if node() == 0

[chris fleizach]

Comment on attachment 114407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114407&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:963
>  

don't understand what this change does since the layout test this is associated with is testing the result when focus is not in the node

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 114392 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114392&action=review
> > 
> > >>> LayoutTests/accessibility/textarea-insertion-point-line-number.html:29
> > >>> +                var lineNumber = area1AXUIElement.insertionPointLineNumber;
> > >> 
> > >> it looks like there are changes to this layout test that don't actually do anything. i.e.) renaming focusedElement to area1AXUIElement.
> > >> those kinds of changes make this patch harder to review because i don't know if you're changing the layout test because its wrong or what.
> > >> 
> > >> also, i believe this layout test was functioning correctly before. there was probably a good reason why <textarea> was empty. you're changing what this test does and it seems like we might be losing coverage. i would suggest adding a new test, since it seems like you're trying to fix a bug
> > > 
> > > Pulling accessibilityController.focusedElement out into a variable actually does change the logic, as at line 39 accessibilityController.focusedElement is a different element to what it is at line 24.
> > > 
> > > I made these changes to try and make this test correct in terms of what it says it's testing: the comment "Not focused on text area" suggests that it's intended to test the case where a given textarea's line number is being queried, but the focus is not in that textarea; however, it's actually testing the case where focus is placed in a textarea with 0 lines. If you think that's what it was intended to test, I'm happy to reword the comment and restore the original test (and code) logic. I'm asking for your opinion here as you wrote the original test, and I'm unable to see any discussion that happened on that patch as the radar link is inaccessible to me; the original code was very difficult to follow, so I wanted to make it more explicit before I made the changes in bug 71263.
> > 
> > I think the intended behavior is test what happens in the first text area when it no longer has focus.
> 
> I agree: that's not what the original test is testing, which is why I made these changes.
> 

I guess what I'm missing is what does the layout test result look like after the change, but before the fix.

> > The test is wrong is that it's clearly getting the insertion point for the second text area.
> 
> Yes: this is why I made the change to pull area1AXUIElement out into a variable.
> 
> > Calling area1.selectionStart = 0 defeats the purpose of the test in that sense.
> 
> I don't follow. Calling area1.selectionStart does not put focus back on to area1.
> 

So what's the reason for adding it? It could potentially alter the meaning of the test

> > There's also no reason to add text inside of text area 2, since that content is never being tested.
> 
> Ok, I will remove that text.

[Alice Boxhall]

Created attachment 114536 [details]
Patch

[Alice Boxhall]

Created attachment 114537 [details]
Patch

[Alice Boxhall]

Comment on attachment 114407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114407&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:947
>> +    if (!containerNode->containsIncludingShadowDOM(node()) && !node()->containsIncludingShadowDOM(containerNode))
> 
> this doesn't check if node() == 0

Fixed.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:963
>>  
> 
> don't understand what this change does since the layout test this is associated with is testing the result when focus is not in the node

The layout test checks multiple cases:
- selection at the start of the textarea
- selection in the middle of the textarea
- selection at the end of the textarea
- selection outside the textarea

Previously, this code implicitly returned -1 for certain cases. I have tried to rewrite the code to make those cases explicit, and ensure that in any other case an accurate line number is returned. I changed this to a do while loop as it doesn't make sense to check inSameLine(currentVisiblePos, savedVisiblePos) before savedVisiblePos has been set.

Can you tell me whether the current code and test reflects the intended logic?

[Alice Boxhall]

Comment on attachment 114537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114537&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1891
> +            AccessibilityObject* focusedObject = m_object->axObjectCache()->getOrCreate(m_object->document()->focusedNode()->renderer());

Please note that this code is not currently marked for review.

I realised that AccessibilityObject::lineForPosition() was always being called with a position calculated from the object being queried, rather than the document, so the logic for checking whether the given position was outside the object being queried was never being exercised. This change makes the test work as intended (without the extra call to setSelection).

I'm not sure this is the best way to achieve this behaviour; however, I am still not sure what the intended behaviour actually is. Could you please let me know whether the test as currently written is demonstrating the desired behaviour (i.e. focus outside of the object being queried results in returning -1 for the line number)?

Also, it is somewhat coincidental that the -1 value gets through to the test, as this method actually returns nil in this case, which DumpRenderTree/mac/AccessibilityUIElementMac.mm converts back into a -1 value, so I'm not sure what the actual browser user sees.

[chris fleizach]

Comment on attachment 114537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114537&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:946
> +    if (!node())

this comment should explain why you're returning -1 if no node(), (instead of explaining what it does). I think the general rule is comments should explain why, instead of what, unless the what is not very clear from the code because its complicated

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1891
>> +            AccessibilityObject* focusedObject = m_object->axObjectCache()->getOrCreate(m_object->document()->focusedNode()->renderer());
> 
> Please note that this code is not currently marked for review.
> 
> I realised that AccessibilityObject::lineForPosition() was always being called with a position calculated from the object being queried, rather than the document, so the logic for checking whether the given position was outside the object being queried was never being exercised. This change makes the test work as intended (without the extra call to setSelection).
> 
> I'm not sure this is the best way to achieve this behaviour; however, I am still not sure what the intended behaviour actually is. Could you please let me know whether the test as currently written is demonstrating the desired behaviour (i.e. focus outside of the object being queried results in returning -1 for the line number)?
> 
> Also, it is somewhat coincidental that the -1 value gets through to the test, as this method actually returns nil in this case, which DumpRenderTree/mac/AccessibilityUIElementMac.mm converts back into a -1 value, so I'm not sure what the actual browser user sees.

i think at some point in the past TextEdit would return -1 or nil when focus was not within the text area for insertionPointLineNumber. when i just checked on Lion, this is no longer true. if focus is not in the object, then it returns whatever it was before focus was lost... so i'm not sure that code is even relevant on the mac anymore.

It seems equivalent to check if the focused object == this, and if it doesn't then, return nil. that will probably be a little clearer in the code as to what's happening.  

i think you can get the focusedObject from m_object->focusedUIElement()

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1895
>                  return nil;

I think we return -1 when focus is not within the text field, so that the platform can decide what to do when the focus is not in fact within the text field. in the mac case, it seems like we want to return nil

[Alice Boxhall]

Comment on attachment 114537 [details]
Patch

Thanks for the comments; looking to clarify a few things before I touch the code again.

View in context: https://bugs.webkit.org/attachment.cgi?id=114537&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:946
>> +    if (!node())
> 
> this comment should explain why you're returning -1 if no node(), (instead of explaining what it does). I think the general rule is comments should explain why, instead of what, unless the what is not very clear from the code because its complicated

Good point. Removed comment for now, will add one back in once I understand all the logic of what gets returned when.

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1891
>>> +            AccessibilityObject* focusedObject = m_object->axObjectCache()->getOrCreate(m_object->document()->focusedNode()->renderer());
>> 
>> Please note that this code is not currently marked for review.
>> 
>> I realised that AccessibilityObject::lineForPosition() was always being called with a position calculated from the object being queried, rather than the document, so the logic for checking whether the given position was outside the object being queried was never being exercised. This change makes the test work as intended (without the extra call to setSelection).
>> 
>> I'm not sure this is the best way to achieve this behaviour; however, I am still not sure what the intended behaviour actually is. Could you please let me know whether the test as currently written is demonstrating the desired behaviour (i.e. focus outside of the object being queried results in returning -1 for the line number)?
>> 
>> Also, it is somewhat coincidental that the -1 value gets through to the test, as this method actually returns nil in this case, which DumpRenderTree/mac/AccessibilityUIElementMac.mm converts back into a -1 value, so I'm not sure what the actual browser user sees.
> 
> i think at some point in the past TextEdit would return -1 or nil when focus was not within the text area for insertionPointLineNumber. when i just checked on Lion, this is no longer true. if focus is not in the object, then it returns whatever it was before focus was lost... so i'm not sure that code is even relevant on the mac anymore.
> 
> It seems equivalent to check if the focused object == this, and if it doesn't then, return nil. that will probably be a little clearer in the code as to what's happening.  
> 
> i think you can get the focusedObject from m_object->focusedUIElement()

Oh -- I don't know how I missed the focusedUIElement method.

I'm not sure about checking focusedObject == this -- what about the case where there is a parent/child relationship between focusedObject and this? I'm not sure whether they are guaranteed to be the same thing at the point where this method is called. Should I check that too? Even then, the selection could be either in the child node, or the parent node but not the child node.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1895
>>                  return nil;
> 
> I think we return -1 when focus is not within the text field, so that the platform can decide what to do when the focus is not in fact within the text field. in the mac case, it seems like we want to return nil

Makes sense. It just makes me feel a bit weird about testing that method by looking for -1 in the layout test.

[Alice Boxhall]

Created attachment 118517 [details]
Patch

[Alice Boxhall]

(In reply to comment #24)
> > this comment should explain why you're returning -1 if no node(), (instead of explaining what it does). I think the general rule is comments should explain why, instead of what, unless the what is not very clear from the code because its complicated
> 
> Good point. Removed comment for now, will add one back in once I understand all the logic of what gets returned when.

Ended up not putting this comment back in; let me know if you think it needs one.

> > It seems equivalent to check if the focused object == this, and if it doesn't then, return nil. that will probably be a little clearer in the code as to what's happening.  
> 
> I'm not sure about checking focusedObject == this -- what about the case where there is a parent/child relationship between focusedObject and this? I'm not sure whether they are guaranteed to be the same thing at the point where this method is called. Should I check that too? Even then, the selection could be either in the child node, or the parent node but not the child node.

After much thought and experimenting with the layout test I think this actually is correct (i.e. testing for focusedObject == m_object) -- the layout test now includes a case for this.

[Alice Boxhall]

Created attachment 118724 [details]
Patch

[Alice Boxhall]

Forgot to included new test results in the last patch.

[chris fleizach]

Comment on attachment 118724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118724&action=review

> LayoutTests/accessibility/textarea-insertion-point-line-number.html:31
> +            for (var k = 0; k < 3; k++ ) {

unnecessary space added

> LayoutTests/accessibility/textarea-insertion-point-line-number.html:57
> +

You should probably use the 
shouldBe() idiom so that if the test fails, it's clear and a FAILURE message will appear

[Alice Boxhall]

Created attachment 118847 [details]
Patch

[Alice Boxhall]

(In reply to comment #29)
> (From update of attachment 118724 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118724&action=review
> 
> > LayoutTests/accessibility/textarea-insertion-point-line-number.html:31
> > +            for (var k = 0; k < 3; k++ ) {
> 
> unnecessary space added

Fixed.

> > LayoutTests/accessibility/textarea-insertion-point-line-number.html:57
> > +
> 
> You should probably use the 
> shouldBe() idiom so that if the test fails, it's clear and a FAILURE message will appear

Honestly, I find shouldBe() tests really unreadable as they don't provide any mechanism for an informative message, so you don't know which case has actually failed. Do people ever really look at test results outside the context of comparison with expected result?

[Darin Adler]

(In reply to comment #31)
> Honestly, I find shouldBe() tests really unreadable as they don't provide any mechanism for an informative message, so you don't know which case has actually failed.

I am bewildered by that. The key to a shouldBe test is to put the useful context in the expressions passed to shouldBe; then the informative information is right there in the test output.

Maybe we could discuss a particular example where shouldBe gives a bad result and I can show you how I’d improve it.

> Do people ever really look at test results outside the context of comparison with expected result?

Yes, I do a lot!

I run individual tests often many times in a row once the regression test machinery detects a failure.

[Alice Boxhall]

(In reply to comment #32)
> (In reply to comment #31)
> > Honestly, I find shouldBe() tests really unreadable as they don't provide any mechanism for an informative message, so you don't know which case has actually failed.
> 
> I am bewildered by that. The key to a shouldBe test is to put the useful context in the expressions passed to shouldBe; then the informative information is right there in the test output.

Compare http://trac.webkit.org/browser/trunk/LayoutTests/accessibility/aria-checkbox-checked-expected.txt to the expected results in this change.

> Maybe we could discuss a particular example where shouldBe gives a bad result and I can show you how I’d improve it.
> 
> > Do people ever really look at test results outside the context of comparison with expected result?
> 
> Yes, I do a lot!
> 
> I run individual tests often many times in a row once the regression test machinery detects a failure.

Why have expected results separately from the test if the test must also contain the expected results? Are they unit tests, or layout tests?

[chris fleizach]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > Honestly, I find shouldBe() tests really unreadable as they don't provide any mechanism for an informative message, so you don't know which case has actually failed.
> > 
> > I am bewildered by that. The key to a shouldBe test is to put the useful context in the expressions passed to shouldBe; then the informative information is right there in the test output.
> 
> Compare http://trac.webkit.org/browser/trunk/LayoutTests/accessibility/aria-checkbox-checked-expected.txt to the expected results in this change.
> 
> > Maybe we could discuss a particular example where shouldBe gives a bad result and I can show you how I’d improve it.
> > 
> > > Do people ever really look at test results outside the context of comparison with expected result?
> > 
> > Yes, I do a lot!
> > 
> > I run individual tests often many times in a row once the regression test machinery detects a failure.
> 
> Why have expected results separately from the test if the test must also contain the expected results? Are they unit tests, or layout tests?

You're not always going to be able to understand why a test failed looking at the expected results. The same goes for this test here. If one of the numbers change from 2 to 3, it won't mean anything until I look at the test, hopefully see comments and understand whats happening

The difference is that shouldBe outputs a nice failure message and also points out what method is causing the problem right away. In this case, we'd see insertionPointLineNumber FAILED. If I was working in that area, I wouldn't have to then investigate which method failed in that test, I'd have an area pinpointed. Without shouldBe, then I have to dig into the test, always, to try to understand what's happening

[Alice Boxhall]

(In reply to comment #34)
> > Why have expected results separately from the test if the test must also contain the expected results? Are they unit tests, or layout tests?
> 
> You're not always going to be able to understand why a test failed looking at the expected results. The same goes for this test here. If one of the numbers change from 2 to 3, it won't mean anything until I look at the test, hopefully see comments and understand whats happening
> 
> The difference is that shouldBe outputs a nice failure message and also points out what method is causing the problem right away. In this case, we'd see insertionPointLineNumber FAILED. If I was working in that area, I wouldn't have to then investigate which method failed in that test, I'd have an area pinpointed. Without shouldBe, then I have to dig into the test, always, to try to understand what's happening

I disagree that having a FAILED message adds anything in the case that you are looking at the test results in comparison with the expected results (i.e. as a diff). And in this case, you'd actually be _losing_ information: all you'd see would be

PASS insertionPointLineNumber was 0
PASS insertionPointLineNumber was -1
FAIL insertionPointLineNumber was 4 (or whatever)

You would have to count through the calls in the test to know which invocation failed. I completely fail to understand how that is an improvement.

[Alice Boxhall]

(In reply to comment #35)
> (In reply to comment #34)
> > > Why have expected results separately from the test if the test must also contain the expected results? Are they unit tests, or layout tests?
> > 
> > You're not always going to be able to understand why a test failed looking at the expected results. The same goes for this test here. If one of the numbers change from 2 to 3, it won't mean anything until I look at the test, hopefully see comments and understand whats happening
> > 
> > The difference is that shouldBe outputs a nice failure message and also points out what method is causing the problem right away. In this case, we'd see insertionPointLineNumber FAILED. If I was working in that area, I wouldn't have to then investigate which method failed in that test, I'd have an area pinpointed. Without shouldBe, then I have to dig into the test, always, to try to understand what's happening
> 
> I disagree that having a FAILED message adds anything in the case that you are looking at the test results in comparison with the expected results (i.e. as a diff). And in this case, you'd actually be _losing_ information: all you'd see would be
> 
> PASS insertionPointLineNumber was 0
> PASS insertionPointLineNumber was -1
> FAIL insertionPointLineNumber was 4 (or whatever)
> 
> You would have to count through the calls in the test to know which invocation failed. I completely fail to understand how that is an improvement.

To be clear: I am more than happy to rewrite the test for clarity; I just don't think that using shouldBe will achieve that.

[Darin Adler]

(In reply to comment #35)
> PASS insertionPointLineNumber was 0
> PASS insertionPointLineNumber was -1
> FAIL insertionPointLineNumber was 4 (or whatever)

The problem here is that the left side is just a variable. Instead it should be a function invocation that makes it clear what is being tested.

If you are writing this:

    shouldBe('<variableName>', '<number>')

Then you are doing it wrong.

[Alice Boxhall]

(In reply to comment #37)
> (In reply to comment #35)
> > PASS insertionPointLineNumber was 0
> > PASS insertionPointLineNumber was -1
> > FAIL insertionPointLineNumber was 4 (or whatever)
> 
> The problem here is that the left side is just a variable. Instead it should be a function invocation that makes it clear what is being tested.
> 
> If you are writing this:
> 
>     shouldBe('<variableName>', '<number>')
> 
> Then you are doing it wrong.

Sorry, I was in a hurry and elided part of my example. It would actually be:

[PASS|FAIL] contenteditableAXUIElement.insertionPointLineNumber was <value>

The point is, what is changing between invocations (in this case, the location of the selection/cursor) is necessarily not a part of the expression, so that part of the context simply can't be expressed via shouldBe.

[Darin Adler]

(In reply to comment #38)
> The point is, what is changing between invocations (in this case, the location of the selection/cursor) is necessarily not a part of the expression, so that part of the context simply can't be expressed via shouldBe.

It can and should be. The expression can contain multiple statements with semicolon separators. The test should be inside the shouldBe argument.

[Alice Boxhall]

Created attachment 119108 [details]
Patch

[Alice Boxhall]

(In reply to comment #39)
> (In reply to comment #38)
> > The point is, what is changing between invocations (in this case, the location of the selection/cursor) is necessarily not a part of the expression, so that part of the context simply can't be expressed via shouldBe.
> 
> It can and should be. The expression can contain multiple statements with semicolon separators. The test should be inside the shouldBe argument.

Ok, I've tried to achieve that with this patch. Could you tell me if this is what you meant?

[Alice Boxhall]

(In reply to comment #41)
> (In reply to comment #39)
> > (In reply to comment #38)
> > > The point is, what is changing between invocations (in this case, the location of the selection/cursor) is necessarily not a part of the expression, so that part of the context simply can't be expressed via shouldBe.
> > 
> > It can and should be. The expression can contain multiple statements with semicolon separators. The test should be inside the shouldBe argument.
> 
> Ok, I've tried to achieve that with this patch. Could you tell me if this is what you meant?

Would one of you have time to have a look at this?

[WebKit Review Bot]

Comment on attachment 119108 [details]
Patch

Rejecting attachment 119108 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 10163 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 10163.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10895092

[Alice Boxhall]

Created attachment 119367 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119367 [details]
Patch

Rejecting attachment 119367 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10910144

[Alice Boxhall]

Oops, I was hoping it'd magically find the R+ from the previous patch.

[chris fleizach]

Did you upload a different patch than the one that was reviewed and then set to r+? The layout test looks different from the previous patch

[Alice Boxhall]

I didn't intend to, but it looks like that change to the layout test got lost. Will fix.

[Alice Boxhall]

Created attachment 119565 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119565 [details]
Patch

Clearing flags on attachment: 119565

Committed r103197: <http://trac.webkit.org/changeset/103197>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.