Bug 49514

Summary: [Gtk] atk_text_get_selection returns the wrong offsets after a link
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apinheiro, eric, mario, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
test case
none
Patch proposal
none
Patch proposal + Unit test mrobinson: review+

Description Joanmarie Diggs 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 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?