Bug 114872 – [GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

We need to get rid of Pango/Gail dependency, and this is one of the functionalities we would need to fix.

[Mario Sanchez Prada]

Created attachment 206794 [details]
Patch proposal

I tried this patch, besides passing all the unit and layout tests, it is the first one in this series that fixes a real and important issue we currently have in WebKit2GTK+: not able to navigate line by line with ATs (see bug 73433).

So, kindly asking for review over it

[Mario Sanchez Prada]

I'm basing this patch on the one for the WORD boundary, thus reflecting that in the dependency tree

[EFL EWS Bot]

Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1090279

[kov's GTK+ EWS bot]

Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1085265

[EFL EWS Bot]

Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1089247

[Build Bot]

Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1080282

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206803 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Mario Sanchez Prada]

Created attachment 207116 [details]
Patch proposal

New patch after some changes I did while working on the SENTENCE boundary

[EFL EWS Bot]

Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1105873

[EFL EWS Bot]

Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1130760

[kov's GTK+ EWS bot]

Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1098993

[Build Bot]

Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1248150

New failing tests:
dom/xhtml/level2/events/dispatchEvent03.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/xhtml/level2/events/dispatchEvent02.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/html/level1/core/hc_attrappendchild4.html
dom/xhtml/level2/core/createAttributeNS06.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level2/events/dispatchEvent04.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/core/setAttributeNS10.html
dom/xhtml/level2/events/dispatchEvent01.xhtml
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/xhtml/level2/events/dispatchEvent05.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/events/dispatchEvent06.html
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/events/dispatchEvent01.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level2/core/createAttributeNS06.html
dom/xhtml/level2/events/dispatchEvent06.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html

[Build Bot]

Created attachment 207511 [details]
Archive of layout-test-results from APPLE-EWS-6 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-6  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Mario Sanchez Prada]

Created attachment 208065 [details]
Patch proposal

Patch rebased against trunk, to make EWS happy

[Mario Sanchez Prada]

Created attachment 208440 [details]
Patch proposal

I just realized that I can actually uncomment some tests that were previously commented due to a bug in GailTextUtil, so I'm attaching a new patch now including that.

[Build Bot]

Comment on attachment 208440 [details]
Patch proposal

Attachment 208440 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1499377

[Mario Sanchez Prada]

Created attachment 210441 [details]
Patch proposal

New patch renaming new function webkitAccessibleTextGetLineForBoundary to webkitAccessibleTextLineForBoundary for consistency with the rest of the code (see recently landed patch for bug 114873).

This is now the last patch in the "remove pango/gail dependency from ATK code" series :)

[chrys]

Is there a chance to get it in the 2.2? I m really looking to this :)

[Mario Sanchez Prada]

(In reply to comment #18)
> Is there a chance to get it in the 2.2? I m really looking to this :)

Stating the obvious, we need to go through the review process first. Once that's done, you can believe me I'll be the first one interested in getting it in, along with other patches that are currently depending on this one :)

[Brian Holt]

Comment on attachment 210441 [details]
Patch proposal

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

Informal review: looks sane to me, just a few suggestions and queries

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:982
> +        // Additionally to checking that we are not at the end of a block, we need

Additionally -> In addition

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:990
> +        if (isEndOfLine(position))

Ternary operator?
startPosition = isEndOfline(position) ? position : logicalStartOfLine(position);

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1016
> +    Document* document = coreObject->document();

Check coreObject == 0?

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026
> +    // Besides of the usual conversion from ATK offsets to WebCore offsets,

"of" can be removed

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1038
> +    case GetTextPositionAt:

Is there nothing here on purpose? Maybe a comment then?

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1061
> +    VisibleSelection selectedLine = newPosition != caretPosition ? lineAtPositionForAtkBoundary(coreObject, newPosition, boundaryType) : currentLine;

parentheses around the condition being evaluated?

[Gustavo Noronha (kov)]

Comment on attachment 210441 [details]
Patch proposal

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

LGTM

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026
> +    // Besides of the usual conversion from ATK offsets to WebCore offsets,

s/of //

[Mario Sanchez Prada]

Comment on attachment 210441 [details]
Patch proposal

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

Thanks both for your reviews. I will address those issues before landing the patch

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:982
>> +        // Additionally to checking that we are not at the end of a block, we need
> 
> Additionally -> In addition

Ok.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:990
>> +        if (isEndOfLine(position))
> 
> Ternary operator?
> startPosition = isEndOfline(position) ? position : logicalStartOfLine(position);

Yes, I could use that. I just didn't do it for consistency with similar usages in this same file (where readability would be worse if I used it), but in this case I can certainly use it.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1016
>> +    Document* document = coreObject->document();
> 
> Check coreObject == 0?

Not needed. Any AtkObject will always have a AccessibilityObject, so we normally don't do check for those objects to be different than NULL unless we are walking up or down through the hierarchy.

>>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1026
>>> +    // Besides of the usual conversion from ATK offsets to WebCore offsets,
>> 
>> "of" can be removed
> 
> s/of //

That's a typo. Or not. We'll never know, but I'll remove it anyway :D

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1038
>> +    case GetTextPositionAt:
> 
> Is there nothing here on purpose? Maybe a comment then?

Yes, there's nothing there because the AT position is the default case, hence we do not need to do any additional calculation such as getting the text after or before the line boundaries we previously found. At the same time we want to have this case here so we can catch later any other invalid position type that might have been passed here (even if unlikely) in the default switch case below.

In any case, a comment here would be helpful. I'll add one before landing

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:1061
>> +    VisibleSelection selectedLine = newPosition != caretPosition ? lineAtPositionForAtkBoundary(coreObject, newPosition, boundaryType) : currentLine;
> 
> parentheses around the condition being evaluated?

We don't normally add them if they are not strictly needed from a logical pov.

[Mario Sanchez Prada]

Committed r155516: <http://trac.webkit.org/changeset/155516>