We need to get rid of Pango/Gail dependency, and this is one of the functionalities we would need to fix.
Created attachment 207117 [details] Patch proposal Almost the last patch in these series. The last one will be one actually removing the whole Pango/Gail stuff and probably changing the ugly set of ifs in webkitAccessibleTextGetTextForOffset() into a switch statement :)
Depending on the patch for the WORD boundary, since some changes where introduced there for getSelectionOffsetsForObject() that we are using here too.
Comment on attachment 207117 [details] Patch proposal Attachment 207117 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1130762
Comment on attachment 207117 [details] Patch proposal Attachment 207117 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1106912
Comment on attachment 207117 [details] Patch proposal Attachment 207117 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1096888
Created attachment 207190 [details] Patch proposal New patch without an extra snippet of code that slipped in before
Comment on attachment 207190 [details] Patch proposal Attachment 207190 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1140103
Comment on attachment 207190 [details] Patch proposal Attachment 207190 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1144026
Comment on attachment 207190 [details] Patch proposal Attachment 207190 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1139134
Created attachment 208066 [details] Patch proposal Patch rebased against trunk, to make EWS happy
Comment on attachment 208066 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=208066&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:947 > +static bool isSentenceBoundary(const VisiblePosition &pos) nit: & on wrong side
Please can review someone the Patch :D? I really need it :D.
Comment on attachment 208066 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=208066&action=review any layout tests available? > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:952 > + // It's definitely a sentence boundary if there's nothing before. shouldn't this method just be return startOfSentence(pos) == pos; > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:961 > +static bool isSpaceBetweenRealSentences(const VisiblePosition& position) the name for the method is a bit confusing. what is it trying to determine?
(In reply to comment #13) > (From update of attachment 208066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208066&action=review > > any layout tests available? This implements atk_text_get_text_{at|before|after}_offset() for the AT_TEXT_BOUNDARY_SENTENCE_{START|END} boundaries, which are ATK specific things that are not tested with layout tests, but with the unit tests for ATK (Source/WebKit/gtk/tests/testatk.c). The point of this patch is to replace the old implementation, based on GAIL (deprecated library) and Pango, with the new one without having those dependencies, so as long as the same unit tests keep passing for the new implementation, it could be considered tested. > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:952 > > + // It's definitely a sentence boundary if there's nothing before. > > shouldn't this method just be > > return startOfSentence(pos) == pos; Not really, becaause that would fail if you are exactly in the boundary, which is what this function tries to determine. For instance, consider the following text: "This is sentence 1. And this is sentence 2" If you happen to be in the position for the 'A' in "And", right after the space, startOfSentence(pos) will return you the offset 0 (right at the beginning) since you are both at the beginning of the second sentence and at the end of the first one. That's the reason why I do the backwards & forward dance, so I can be sure that the current position is actually a boundary, since endOfSentence(0) in that case will return again the offset for the 'A' of "And". > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:961 > > +static bool isSpaceBetweenRealSentences(const VisiblePosition& position) > > the name for the method is a bit confusing. what is it trying to determine? Agreed I'm really bad at names :) What the method is trying to determine, though, is whether the position is in the middle of two "real sentences", where "real sentence" means portions of text identified as sentences withOUT any leading or trailing whitespace. For instance, consider the following example (notice multiple spaces between sentences, that should also work): "This is the 1st sentence. This is the 2nd one. And this is the 3rd one." In this case isSpaceBetweenRealSentences() should return true whenever you are in any position after a period and before the first non-space character (the 'T' or the 'A'). This weird thing is needed because we need to do some extra checks to implement the SENTENCE_END boundary, which would consider the following things to be sentences to the eyes of ATK: Sentence 1: "This is the 1st sentence." Sentence 2: " This is the 2nd one." Sentence 3: " And this is the 3rd one." Fortunately, this would be just a temporary need since we have just deprecated recently the END boundaries in ATK/AT-SPI, so this code will be removed as soon as WebKitGTK+ can safely depend on newer versions of ATK and assume newer versions of AT-SPI running in the system (1 year from now, theoretically). In any case, the name is certainly confusing, so I will change it for isWhiteSpaceBetweenSentences(). I think the "Real" part does not do any good.
Created attachment 210364 [details] Patch proposal Attaching new patch with the renamed function
Comment on attachment 210364 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=210364&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:916 > +static char* webkitAccessibleTextGetSentenceForBoundary(AtkText* text, int offset, AtkTextBoundary boundaryType, GetTextRelativePosition textPosition, int* startOffset, int* endOffset) you don't need Get in the method name
(In reply to comment #16) > (From update of attachment 210364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210364&action=review > > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:916 > > +static char* webkitAccessibleTextGetSentenceForBoundary(AtkText* text, int offset, AtkTextBoundary boundaryType, GetTextRelativePosition textPosition, int* startOffset, int* endOffset) > > you don't need Get in the method name Ok. I'll change it before landing. Thanks
Committed r155031: <http://trac.webkit.org/changeset/155031>