710389 - PeekOffset returns wrong offset for word boundary

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Eitan Isaacson [:eeejay]]

Attachment: Test case HTML

With the given document, a call to PeekOffset on the div's frame with this structure:

nsPeekOffsetStruct pos;
pos.SetData(eSelectWord, eDirPrevious, 0, 0, true, false, true, false, eStartWord);

returns an mContentOffset of 60, when the expected value is 0.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Possible fix

SeekPos() erroneously adjusts the offset to -1 which is later interpreted as a last-char offset.

This patch removes whitespace-only content nodes from calculation of offset. This is totally a shot in the dark, since I am not familiar with how whitespace-only nodes are generally treated.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Possible fix

Just a slight re-order

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581425 [details] [diff] [review]
Possible fix


>@@ -2950,8 +2950,14 @@ static FrameContentRange GetRangeForFrame(nsIFrame* aFrame) {
>     parent  = content->GetParent();
>     if (parent) {
>       PRInt32 beginOffset = parent->IndexOf(content);
>-      if (beginOffset >= 0)

we usually use 8 lines of context (-U8 for most diffs).  Since you look like your using git I'd recomend using bholley's  version of git-bz.

Comment 4

[Trevor Saunders (:tbsaunde)]

Eitan says this was discovered when using nsIAccessibleText, but mochitest-a11y doesn't seem to cover it.

Comment 5

[(no longer active)]

Comment on attachment 581425 [details] [diff] [review]
Possible fix

I'd really rather roc look at this.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Eitan Isaacson [:eeejay] from comment #1)
> SeekPos() erroneously adjusts the offset to -1 which is later interpreted as
> a last-char offset.

SeekPos doesn't exist, what did you mean there? This sounds like a plausible explanation but I'd like to hear it in more detail :-). I suspect your patch isn't the correct fix for the bug.

Comment 7

[Eitan Isaacson [:eeejay]]

Yeah, what is SeekPos??
The offending line is here[1]. If the given offset is 0, and the frame's content has siblings to the left of it, they get subtracted from 0, and things go wrong. Specifically when a sibling has no text.

1. http://hg.mozilla.org/mozilla-central/file/803c353ee693/layout/generic/nsFrame.cpp#l5811

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the problem with that line is that aPos->mStartOffset is assumed to be within the frame's content node, whereas GetRangeForFrame can return a node that's not the frame's content node, so subtracting one offset from the other doesn't make sense when they're in different nodes.

Comment 9

[Mats Palmgren (inactive)]

> Eitan says this was discovered when using nsIAccessibleText

Is it nsHyperTextAccessible::GetRelativeOffset we're talking about?
Perhaps it should do what nsFrame::PeekBackwardAndForward does and move forward
one character first?
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2642

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 581425 [details] [diff] [review]
Possible fix

Removing review flag, since it is not going to happen :)

Comment 11

[James Teh [:Jamie] (away returning 14 Apr)]

Eitan, do you have any idea what the a11y repro for this was or should we just close it? I can't see a problem here, with or without the cache, even if I give the div a role to force an Accessible.

Comment 12

[Eitan Isaacson [:eeejay]]

Let's just close as works for me. This is old enough to not matter, especially if our current text implementation doesn't trip up on this.