[Webkit-unassigned] [Bug 114871] [GTK] Reimplement atk_text_get_text_*_offset for WORD boundaries

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=114871





--- Comment #10 from Mario Sanchez Prada <mario at webkit.org>  2013-06-26 06:07:14 PST ---
(From update of attachment 203441)
View in context: https://bugs.webkit.org/attachment.cgi?id=203441&action=review

Thanks for the review Martin, and no problem for the delay. I've been busy with other matters as well and wouldn't have had time before now anyway, so "perfect timing" in the end :).

Now see my comments below...

>> Source/WebCore/ChangeLog:50
>> +
> 
> Double changelog here.

Oops! Will fix that.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:473
>> +static gint offsetAdjustmentForObject(const AccessibilityObject* object)
> 
> Maybe call this listItemMarkerOffsetForObject?

Well, the idea is that it might server for more adjustments in the future, not just the list item. That said, reality is that at this moment it is just about list items, so I'm fine with changing the name now.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:489
>> +static gint webCoreOffsetToAtkOffset(const AccessibilityObject* object, gint offset)
> 
> You shouldn't use glib types for private functions. So gint -> int and for the rest of this patch.

Argh! Will fix it. Thx

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:544
>> +    Node* referenceNode = rangeStartNode->isInShadowTree() ? rangeStartNode->highestAncestor() : node;
> 
> Does the shadow DOM still exist?

At the time of writing this patch it did, but I will double check that.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:644
>> +    // word following.
> 
> I'm curious why you don't use things like startOfWord and endOfWord to do these tasks.

:-) Believe me I tried, but there's a explanation for that change:

The reason is that semantics for startOfWord() and endOfWord() are slightly different than what we need to do here, since we need to take into account different kind of boundaries (WORD_START, WORD_END), deal with special characters (e.g. the period or the '+') in special ways to provide what ATs expect to receive when you ask things like "What are the word boundaries for the current offset?". Also, startOfWord() and endOfWord() consider a "whitespace between words" as yet another "word", which is clearly not what we want, and more things like that.

For instance, if you have the following sentence:

  "This is a stupid sentence. WebKitGTK+ rocks. My taylor is rich"

If you just use startOfWord() and endOfWord(), you could easily conclude that things like " ", ". " or "+ " are valid words for the WORD_START boundary, unless you do a lot of specific, case-by-case, tests.

However, if you use this "trick" going forward and backwards (which I got the idea for from FrameSelection.cpp, as per Enrica's comment while at the meeting), it's more easy to get the expected results without having to deal with all those special cases yourself, since are already considered in lower layers.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:691
>> +        if (isStartOfWord(originalPosition) && isWhitespace(originalPosition.characterBefore()))
> 
> In what situations can something be a start of a word and not prefixed by whitespace. Does this account for if this position is the very first position in the document?

According to the semantics of startOfWort(), which considers spaces between words as another "word", you can get into this situation fairly easily.

For instance, if you have the "foo bar baz" string and you are in the 4th position (right at the end of "foo" and before the space), you would exactly be in that situation. Furthermore, if you call endOfWord() from that position you will be given the 5th position, right before "baz", since that " " in the middle is a "word" to the eyes of those functions.

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:700
>> +        endPosition = nextWordPosition(startPosition);
> 
> Why not endOfWord?

Because that would return you the beginning of the word coming right after the end of the previous word, which is basically where the space between the two words ends (so you would have the boundaries for the whitespace between them).

If you have doubts and want to check your self what happens, think that nextWordPosition() works pretty similar to Ctrl+RightArrow when you have your cursor located in an editable text field. For instance, if you have the sentence "foo bar baz" and you put the cursor at the end of "foo", what happens if you press Ctrl+RightArrow? You move to the end of "bar", which is exactly what you want here. That's why :)

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:806
>> +        return webkitAccessibleTextGetWord(text, offset, boundaryType, textPosition, startOffset, endOffset);
> 
> webkitAccessibleTextGetWord -> webkitAccessibleTextGetAtWordBoundary?

Ok

>> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:813
>>      case GetTextPositionBefore:
> 
> I guess this isn't the last one?

Nope. Unfortunately we still need one for LINE and other for SENTENCE boundaries. Hopefully those should be easier once we have the WORD boundary fixed, I believe.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.