RESOLVED FIXED 114872
[GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
https://bugs.webkit.org/show_bug.cgi?id=114872
Summary [GTK] Reimplement atk_text_get_text_*_offset for LINE boundaries
Mario Sanchez Prada
Reported 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.
Attachments
Patch proposal (11.85 KB, patch)
2013-07-16 10:11 PDT, Mario Sanchez Prada
eflews.bot: commit-queue-
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
Patch proposal (11.79 KB, patch)
2013-07-19 10:08 PDT, Mario Sanchez Prada
no flags
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
Patch proposal (10.21 KB, patch)
2013-08-03 08:19 PDT, Mario Sanchez Prada
no flags
Patch proposal (13.77 KB, patch)
2013-08-09 09:54 PDT, Mario Sanchez Prada
buildbot: commit-queue-
Patch proposal (13.77 KB, patch)
2013-09-04 03:20 PDT, Mario Sanchez Prada
gustavo: review+
Mario Sanchez Prada
Comment 1 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
Mario Sanchez Prada
Comment 2 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
EFL EWS Bot
Comment 3 2013-07-16 10:14:55 PDT
kov's GTK+ EWS bot
Comment 4 2013-07-16 10:17:30 PDT
EFL EWS Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Mario Sanchez Prada
Comment 8 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
EFL EWS Bot
Comment 9 2013-07-19 10:12:29 PDT
EFL EWS Bot
Comment 10 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
kov's GTK+ EWS bot
Comment 11 2013-07-19 10:15:03 PDT
Build Bot
Comment 12 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
Build Bot
Comment 13 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
Mario Sanchez Prada
Comment 14 2013-08-03 08:19:29 PDT
Created attachment 208065 [details] Patch proposal Patch rebased against trunk, to make EWS happy
Mario Sanchez Prada
Comment 15 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.
Build Bot
Comment 16 2013-08-22 00:48:31 PDT
Mario Sanchez Prada
Comment 17 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 :)
chrys
Comment 18 2013-09-05 05:06:39 PDT
Is there a chance to get it in the 2.2? I m really looking to this :)
Mario Sanchez Prada
Comment 19 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 :)
Brian Holt
Comment 20 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?
Gustavo Noronha (kov)
Comment 21 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 //
Mario Sanchez Prada
Comment 22 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.
Mario Sanchez Prada
Comment 23 2013-09-11 03:31:07 PDT
Note You need to log in before you can comment on or make changes to this bug.