Bug 49514 – [Gtk] atk_text_get_selection returns the wrong offsets after a link

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Created attachment 73857 [details]
test case

Steps to reproduce:

1. Load the test case in Epiphany and launch Accerciser.
2. For each paragraph:
   a. Select the word 'test'
   b. Select the paragraph in Accerciser's object tree.
   c. With the pargraph selected, get the first selection by
      typing 'acc.queryText().getSelection(0)' in the iPython
      console.

Expected results: The startOffset and endOffset for the selection would be the same for each paragraph, because the word 'test' appears at the same character offsest in each paragraph.

Actual results: The startOffset and endOffset for the selection in each paragraph is different, with offset 0 appearing to be the first character after the link:

  In [1]: acc.queryText().getSelection(0)
  Out[1]: (10, 14)
  In [2]: acc.queryText().getSelection(0)
  Out[2]: (6, 10)
  In [3]: acc.queryText().getSelection(0)
  Out[3]: (3, 7)
  In [4]: acc.queryText().getSelection(0)
  Out[4]: (1, 5)

Impact: Orca incorrectly states that text is not selected when it is selected. In addition, it displays the wrong text as being selected on the braille display.

[Joanmarie Diggs]

Note: I'm blocking bug 25531 as correctly indicating text selection is part of the basic functionality users will expect to have in Yelp 3.0.

[Mario Sanchez Prada]

Created attachment 73892 [details]
Patch proposal

It looks we were not calculating the startOffset value properly, by taking into account the possibility of some embedded objects being present in the paragraph, previous to the currently selected text (my fault).

The attached patch would fix this by checking the startOffset value more carefully. As the patch also fixes a bunch of not-properl-ended-comments (without '.'), I'm pasting here the so simple diff for simplicity of review:

In getSelectionOffsetsForObject(), at AccessibilityObjectWrapperAtk.cpp:

   [...]
        Node* lastLeafNode = node->lastDescendant();
        if (selRange->isPointInRange(lastLeafNode, lastOffsetInNode(lastLeafNode), ec))
            nodeRangeEnd = lastPositionInNode(lastLeafNode);
        else
            nodeRangeEnd = selRange->endPosition();
 
   -    // Set values for start and end offsets
   +  // Calculate position of the selected range inside the object.
   +  Position parentFirstPosition = firstPositionInNode(node);
   +  RefPtr<Range> rangeInParent = Range::create(node->document(), parentFirstPosition, nodeRangeStart);
   +
   +  // Set values for start and end offsets.
   +  startOffset = TextIterator::rangeLength(rangeInParent.get());
        RefPtr<Range> nodeRange = Range::create(node->document(), nodeRangeStart, nodeRangeEnd);
   -    startOffset = nodeRangeStart.offsetInContainerNode();
        endOffset = startOffset + TextIterator::rangeLength(nodeRange.get());
   [...]

[Mario Sanchez Prada]

Created attachment 74131 [details]
Patch proposal + Unit test

Attaching a new patch adding the needed bits in an already existing test case to check this specific subcase (double checked that this new check fails without this patch, in case you wonder).

So, this is the one to be reviewed.

[Mario Sanchez Prada]

Committed r72284: <http://trac.webkit.org/changeset/72284>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72284 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/css/line-height-determined-by-primary-font.html

[Mario Sanchez Prada]

(In reply to comment #5)
> http://trac.webkit.org/changeset/72284 might have broken GTK Linux 32-bit Release
> The following tests are not passing:
> fast/css/line-height-determined-by-primary-font.html

I have reviewed both the patch and the layout test and the problem must be somewhere else... Other than that, that test seems to be passing right now. A flaky test perhaps?