Bug 114872 - [GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
Summary: [GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on: 114871 118908
Blocks: 73433 114867
  Show dependency treegraph
 
Reported: 2013-04-19 06:50 PDT by Mario Sanchez Prada
Modified: 2013-09-11 03:31 PDT (History)
14 users (show)

See Also:


Attachments
Patch proposal (11.85 KB, patch)
2013-07-16 10:11 PDT, Mario Sanchez Prada
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (683.74 KB, application/zip)
2013-07-16 12:04 PDT, Build Bot
no flags Details
Patch proposal (11.79 KB, patch)
2013-07-19 10:08 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-6 for win-future (107.90 KB, application/zip)
2013-07-25 23:59 PDT, Build Bot
no flags Details
Patch proposal (10.21 KB, patch)
2013-08-03 08:19 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (13.77 KB, patch)
2013-08-09 09:54 PDT, Mario Sanchez Prada
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch proposal (13.77 KB, patch)
2013-09-04 03:20 PDT, Mario Sanchez Prada
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2013-04-19 06:50:25 PDT
We need to get rid of Pango/Gail dependency, and this is one of the functionalities we would need to fix.
Comment 1 Mario Sanchez Prada 2013-07-16 10:11:52 PDT
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
Comment 2 Mario Sanchez Prada 2013-07-16 10:12:26 PDT
I'm basing this patch on the one for the WORD boundary, thus reflecting that in the dependency tree
Comment 3 EFL EWS Bot 2013-07-16 10:14:55 PDT
Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1090279
Comment 4 kov's GTK+ EWS bot 2013-07-16 10:17:30 PDT
Comment on attachment 206794 [details]
Patch proposal

Attachment 206794 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1085265
Comment 5 EFL EWS Bot 2013-07-16 10:19:47 PDT
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
Comment 6 Build Bot 2013-07-16 12:04:13 PDT
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
Comment 7 Build Bot 2013-07-16 12:04:17 PDT
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
Comment 8 Mario Sanchez Prada 2013-07-19 10:08:36 PDT
Created attachment 207116 [details]
Patch proposal

New patch after some changes I did while working on the SENTENCE boundary
Comment 9 EFL EWS Bot 2013-07-19 10:12:29 PDT
Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1105873
Comment 10 EFL EWS Bot 2013-07-19 10:13:44 PDT
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
Comment 11 kov's GTK+ EWS bot 2013-07-19 10:15:03 PDT
Comment on attachment 207116 [details]
Patch proposal

Attachment 207116 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1098993
Comment 12 Build Bot 2013-07-25 23:59:10 PDT
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
Comment 13 Build Bot 2013-07-25 23:59:13 PDT
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
Comment 14 Mario Sanchez Prada 2013-08-03 08:19:29 PDT
Created attachment 208065 [details]
Patch proposal

Patch rebased against trunk, to make EWS happy
Comment 15 Mario Sanchez Prada 2013-08-09 09:54:21 PDT
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.
Comment 16 Build Bot 2013-08-22 00:48:31 PDT
Comment on attachment 208440 [details]
Patch proposal

Attachment 208440 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1499377
Comment 17 Mario Sanchez Prada 2013-09-04 03:20:04 PDT
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 :)
Comment 18 chrys 2013-09-05 05:06:39 PDT
Is there a chance to get it in the 2.2? I m really looking to this :)
Comment 19 Mario Sanchez Prada 2013-09-10 02:00:18 PDT
(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 :)
Comment 20 Brian Holt 2013-09-10 07:44:45 PDT
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?
Comment 21 Gustavo Noronha (kov) 2013-09-10 07:54:21 PDT
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 //
Comment 22 Mario Sanchez Prada 2013-09-10 08:22:00 PDT
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.
Comment 23 Mario Sanchez Prada 2013-09-11 03:31:07 PDT
Committed r155516: <http://trac.webkit.org/changeset/155516>