Bug 49514 - [Gtk] atk_text_get_selection returns the wrong offsets after a link
Summary: [Gtk] atk_text_get_selection returns the wrong offsets after a link
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2010-11-14 14:03 PST by Joanmarie Diggs (irc: joanie)
Modified: 2010-11-18 06:28 PST (History)
6 users (show)

See Also:


Attachments
test case (214 bytes, text/html)
2010-11-14 14:03 PST, Joanmarie Diggs (irc: joanie)
no flags Details
Patch proposal (5.60 KB, patch)
2010-11-15 07:09 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal + Unit test (8.58 KB, patch)
2010-11-17 10:31 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2010-11-14 14:03:38 PST
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.
Comment 1 Joanmarie Diggs (irc: joanie) 2010-11-14 14:04:25 PST
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.
Comment 2 Mario Sanchez Prada 2010-11-15 07:09:12 PST
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());
   [...]
Comment 3 Mario Sanchez Prada 2010-11-17 10:31:40 PST
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.
Comment 4 Mario Sanchez Prada 2010-11-18 04:42:49 PST
Committed r72284: <http://trac.webkit.org/changeset/72284>
Comment 5 WebKit Review Bot 2010-11-18 05:31:13 PST
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
Comment 6 Mario Sanchez Prada 2010-11-18 06:28:10 PST
(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?