Bug 75785 – [GTK] accessibility/textbox-role-reports-line-number.html fails

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

Consistently, since its checkin in r104276. Diff:

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/textbox-role-reports-line-number-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/textbox-role-reports-line-number-actual.txt 
@@ -8,7 +8,7 @@
 
 PASS window.getSelection().setBaseAndExtent(ariaTextBox.childNodes[0], 0, ariaTextBox.childNodes[0], 0); axElement.insertionPointLineNumber is 0
 PASS window.getSelection().setBaseAndExtent(multilineAriaTextBox.childNodes[0], 1, multilineAriaTextBox.childNodes[0], 1); axElement.insertionPointLineNumber is 0
-PASS window.getSelection().setBaseAndExtent(multilineAriaTextBox.childNodes[1], 1, multilineAriaTextBox.childNodes[1], 1); axElement.insertionPointLineNumber is 1
+FAIL window.getSelection().setBaseAndExtent(multilineAriaTextBox.childNodes[1], 1, multilineAriaTextBox.childNodes[1], 1); axElement.insertionPointLineNumber should be 1. Was 0.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Mario Sanchez Prada]

Created attachment 121860 [details]
Patch proposal

[Martin Robinson]

Comment on attachment 121860 [details]
Patch proposal

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

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:794
> +    if (coreObject->isPasswordField() || coreObject->selectionEnd() > 0)
> +        return -1;

Does the fact that there is selected text really mean this is no insertion point? Isn't the insertion point the end of the selection or some-such thing?

[Mario Sanchez Prada]

(In reply to comment #2)
> (From update of attachment 121860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121860&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:794
> > +    if (coreObject->isPasswordField() || coreObject->selectionEnd() > 0)
> > +        return -1;
> 
> Does the fact that there is selected text really mean this is no insertion point? Isn't the insertion point the end of the selection or some-such thing?

Not sure, I just did mimic what the Mac port does when it's asked for the insertion point's _line number_ in WebAccessibilityObjectWrapper.mm:

   [...]
        if ([attributeName isEqualToString: NSAccessibilityInsertionPointLineNumberAttribute]) {
            // if selectionEnd > 0, then there is selected text and this question should not be answered
            if (m_object->isPasswordField() || m_object->selectionEnd() > 0)
                return nil;

            AccessibilityObject* focusedObject = m_object->focusedUIElement();
            if (focusedObject != m_object)
                return nil;

            VisiblePosition focusedPosition = focusedObject->visiblePositionForIndex(focusedObject->selectionStart(), true);
            int lineNumber = m_object->lineForPosition(focusedPosition);
            if (lineNumber < 0)
                return nil;

            return [NSNumber numberWithInt:lineNumber];
        }
   [...]

The only difference is that we don't have that code in our ATK wrapper (there's no such property in ATK/ATSPI), so I needed to add this code to DumpRenderTreeSupportGTK.

Adding Chris to CC, he probably can explain better (or just 'actually' explain it) the rationale behind those checks.

[chris fleizach]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 121860 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121860&action=review
> > 
> > > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:794
> > > +    if (coreObject->isPasswordField() || coreObject->selectionEnd() > 0)
> > > +        return -1;
> > 
> > Does the fact that there is selected text really mean this is no insertion point? Isn't the insertion point the end of the selection or some-such thing?
> 
> Not sure, I just did mimic what the Mac port does when it's asked for the insertion point's _line number_ in WebAccessibilityObjectWrapper.mm:
> 
>    [...]
>         if ([attributeName isEqualToString: NSAccessibilityInsertionPointLineNumberAttribute]) {
>             // if selectionEnd > 0, then there is selected text and this question should not be answered
>             if (m_object->isPasswordField() || m_object->selectionEnd() > 0)
>                 return nil;
> 
>             AccessibilityObject* focusedObject = m_object->focusedUIElement();
>             if (focusedObject != m_object)
>                 return nil;
> 
>             VisiblePosition focusedPosition = focusedObject->visiblePositionForIndex(focusedObject->selectionStart(), true);
>             int lineNumber = m_object->lineForPosition(focusedPosition);
>             if (lineNumber < 0)
>                 return nil;
> 
>             return [NSNumber numberWithInt:lineNumber];
>         }
>    [...]
> 
> The only difference is that we don't have that code in our ATK wrapper (there's no such property in ATK/ATSPI), so I needed to add this code to DumpRenderTreeSupportGTK.
> 
> Adding Chris to CC, he probably can explain better (or just 'actually' explain it) the rationale behind those checks.

I guess insertion point line number only makes sense on the Mac if there's no selection. I'm sure this was put in to mimic some behavior on the mac. (for password fields, i imagine we don't want to return any incriminating evidence). I don't there's a reason why insertion point line number shouldn't exist just because there's a selection

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> I guess insertion point line number only makes sense on the Mac if there's no selection. I'm sure this was put 
> in to mimic some behavior on the mac. (for password fields, i imagine we don't want to return any incriminating 
> evidence). I don't there's a reason why insertion point line number shouldn't exist just because there's a 
> selection

So we can safely remove the first if for the GTK port I guess, right?

[Mario Sanchez Prada]

Hmmm... hold on a second. Maybe I'll be stating the obvious or just sounding dumb, but anyway...

I've been checking some Mac a11y docs [1] and this insertionPointLineNumber thing seem to be something exclusive to the Mac, or at least I don't see anything similar to that in AT-SPI/ATK so I'm wondering if this textbox-role-reports-line-number.html layout test should just be Mac specific, thus not being placed in LayoutTests/accessibility.

After all, what I'm doing in the GTK port to pass the test is to mimic _in the DumpRenderTree_ what the Mac port does _in the a11y wrapper_, so it's not gonna have at the end any impact in WebKitGTK+ based applications, I'm afraid.

Opinions?

[1]http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html#//apple_ref/doc/uid/20000945

[chris fleizach]

(In reply to comment #6)
> Hmmm... hold on a second. Maybe I'll be stating the obvious or just sounding dumb, but anyway...
> 
> I've been checking some Mac a11y docs [1] and this insertionPointLineNumber thing seem to be something exclusive to the Mac, or at least I don't see anything similar to that in AT-SPI/ATK so I'm wondering if this textbox-role-reports-line-number.html layout test should just be Mac specific, thus not being placed in LayoutTests/accessibility.
> 
> After all, what I'm doing in the GTK port to pass the test is to mimic _in the DumpRenderTree_ what the Mac port does _in the a11y wrapper_, so it's not gonna have at the end any impact in WebKitGTK+ based applications, I'm afraid.
> 
> Opinions?
> 
> [1]http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html#//apple_ref/doc/uid/20000945

It's very possible this is a mac specific thing. If GTK does not need to know about the line number, you should disable the test

[Mario Sanchez Prada]

(In reply to comment #7)
> [...]
> It's very possible this is a mac specific thing. If GTK does not need to know about the line number, you should > disable the test

Then the "fix" would be to do nothing and leave it in the Skipped file, I'm afraid. Should we resolve this as INVALID then?

[Martin Robinson]

(In reply to comment #8)
> (In reply to comment #7)
> > [...]
> > It's very possible this is a mac specific thing. If GTK does not need to know about the line number, you should > disable the test
> 
> Then the "fix" would be to do nothing and leave it in the Skipped file, I'm afraid. Should we resolve this as INVALID then?

Or, as you said earlier to make the move the test to the Mac platform directory.

[Mario Sanchez Prada]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > [...]
> > > It's very possible this is a mac specific thing. If GTK does not need to know about the line number, you should > disable the test
> > 
> > Then the "fix" would be to do nothing and leave it in the Skipped file, I'm afraid. Should we resolve this as INVALID then?
> 
> Or, as you said earlier to make the move the test to the Mac platform directory.

Yep, but keeping it in LayoutTests/accessibility could be perhaps better so other platforms can benefit of it (e.g.  qt-mac, chromium-mac)

[Mario Sanchez Prada]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > [...]
> > > > It's very possible this is a mac specific thing. If GTK does not need to know about the line number, you should > disable the test
> > > 
> > > Then the "fix" would be to do nothing and leave it in the Skipped file, I'm afraid. Should we resolve this as INVALID then?
> > 
> > Or, as you said earlier to make the move the test to the Mac platform directory.
> 
> Yep, but keeping it in LayoutTests/accessibility could be perhaps better so other platforms can benefit of it (e.g.  qt-mac, chromium-mac)

Just for making it explicit: I'm for keeping it in the Skipped file, as per the reason above, and as per this sentence in WebKit's TRAC [1]:

"For some tests, on some ports, the test is *never* expected to pass, in which case the test is added to the {{{Skipped}} files instead"

[1]http://trac.webkit.org/wiki/TestExpectations

[Mario Sanchez Prada]

Created attachment 148521 [details]
Patch proposal

(In reply to comment #9)
> [...]
> > Then the "fix" would be to do nothing and leave it in the Skipped file, I'm afraid. Should we resolve this as INVALID then?
> 
> Or, as you said earlier to make the move the test to the Mac platform directory.

Ok, let's try to unblock this issue by following Martin's recommendation. After all, if this is a mac-only thing it's probably the best solution.

[chris fleizach]

Comment on attachment 148521 [details]
Patch proposal

looks good

[chris fleizach]

Comment on attachment 148521 [details]
Patch proposal

just make sure that you update paths in textbox-role-reports-line-number.html for any javascript includes

[Mario Sanchez Prada]

(In reply to comment #14)
> (From update of attachment 148521 [details])
> just make sure that you update paths in textbox-role-reports-line-number.html for any javascript includes

Good point. I'll check that tomorrow before landing.

Thanks

[Mario Sanchez Prada]

Committed r120907: <http://trac.webkit.org/changeset/120907>