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+

Joanmarie Diggs
Reported 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.
Attachments
test case (214 bytes, text/html)
2010-11-14 14:03 PST, Joanmarie Diggs 		no flags
Details
Patch proposal (5.60 KB, patch)
2010-11-15 07:09 PST, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Patch proposal + Unit test (8.58 KB, patch)
2010-11-17 10:31 PST, Mario Sanchez Prada 		mrobinson: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joanmarie Diggs
Comment 1 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.
Mario Sanchez Prada
Comment 2 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()); [...]
Mario Sanchez Prada
Comment 3 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.
Mario Sanchez Prada
Comment 4 2010-11-18 04:42:49 PST
Committed r72284: <http://trac.webkit.org/changeset/72284>
WebKit Review Bot
Comment 5 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
Mario Sanchez Prada
Comment 6 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?
Note You need to log in before you can comment on or make changes to this bug.