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.
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.
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()); [...]
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.
Committed r72284: <http://trac.webkit.org/changeset/72284>
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
(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?