Bug 114873 – [GTK] Reimplement atk_text_get_text_*_offset for SENTENCE boundaries

• Component: WebKitGTK
• Status: RESOLVED FIXED

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

[Mario Sanchez Prada]

Depending on the patch for the WORD boundary, since some changes where introduced there for getSelectionOffsetsForObject() that we are using here too.

[EFL EWS Bot]

Comment on attachment 207117 [details]
Patch proposal

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

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 207117 [details]
Patch proposal

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

[Mario Sanchez Prada]

Created attachment 207190 [details]
Patch proposal

New patch without an extra snippet of code that slipped in before

[EFL EWS Bot]

Comment on attachment 207190 [details]
Patch proposal

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

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 207190 [details]
Patch proposal

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

[Mario Sanchez Prada]

Created attachment 208066 [details]
Patch proposal

Patch rebased against trunk, to make EWS happy

[Chris Dumez]

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

[chrys]

Please can review someone the Patch :D? I really need it :D.

[chris fleizach]

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?

[Mario Sanchez Prada]

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

[Mario Sanchez Prada]

Created attachment 210364 [details]
Patch proposal

Attaching new patch with the renamed function

[chris fleizach]

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

[Mario Sanchez Prada]

(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

[Mario Sanchez Prada]

Committed r155031: <http://trac.webkit.org/changeset/155031>