Bug 114872

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

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

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 on attachment 206794 [details] Patch proposal Attachment 206794 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1085265

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 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

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

Created attachment 207116 [details] Patch proposal New patch after some changes I did while working on the SENTENCE boundary

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 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 on attachment 207116 [details] Patch proposal Attachment 207116 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1098993

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

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

Created attachment 208065 [details] Patch proposal Patch rebased against trunk, to make EWS happy

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 on attachment 208440 [details] Patch proposal Attachment 208440 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1499377

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 :)

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

(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 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 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 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.

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