Bug 160786 – InlineTextBox::isSelected() should only return true for a non-empty selection and remove incorrect FIXME from InlineTextBox::localSelectionRect()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Myles C. Maxfield]

InlineTextBox::isSelected:

// FIXME: We should only be checking if sPos >= ePos here, because those are the
// indices used to actually generate the selection rect. Allowing us past this guard
// on any other condition creates zero-width selection rects.

Same in InlineTextBox::localSelectionRect

[Daniel Bates]

Since CSSOM View Module W3C Working Draft 22 February 2008, <https://www.w3.org/TR/2008/WD-cssom-view-20080222>, it has been acceptable for getClientRects() to return a rectangle with zero width or zero height. With respect to the latest draft [1], this is allowed by step 3 in the algorithm for getClientRects():

[[
3. Return a DOMRectList object containing DOMRect objects in content order, one for each box fragment, describing its border area (including those with a height or width of zero) with the following constraints:
...
]]

Notice the phrase "including those with a height or width of zero".

[1] <https://drafts.csswg.org/cssom-view/#dom-element-getclientrects> (Editor's Draft, 15 September 2017)

[Daniel Bates]

Re-opening to remove comment.

[Daniel Bates]

Created attachment 323379 [details]
Patch

I did not rename the arguments of InlineTextBox::localSelectionRect() as I did for InlineTextBox::isSelected() because I felt it would add too much noise to this patch and I will refactor InlineTextBox::localSelectionRect() sometime this or next week anyway.

[Daniel Bates]

For completeness, we have existing layout tests coverage the condition for the conditional in InlineTextBox::localSelectionRect() that allows for returning a rectangle with an empty width or height. One example of such a test is <https://trac.webkit.org/browser/trunk/LayoutTests/fast/text/international/iso-8859-8.html>.

With regards to the change to the boolean expression in InlineTextBox::isSelected(), the proposed patch reverts the change to this expression made in <https://trac.webkit.org/changeset/204400> (bug #160735). I will update the ChangeLog entry to reflect this before landing this patch. I am unclear why r204400 amended the expression to allow InlineTextBox::isSelected() to return true for a non-selection and unfortunately r204400 neither included a layout test that demonstrates the need for this change nor explained a reason for it in its ChangeLog entry. Simon Fraser raised questions about this change in review in bug #160735, comment #6. These questions went unanswered :( As far as I can tell it represents a programmer error for InlineTextBox::isSelected() to be called with startPos == endPos (maybe we should assert this?). Lastly, RenderTextLineBoxes::setSelectionState() is the only caller of this function.

[Daniel Bates]

I am going to update the patch to revert the change made in <https://trac.webkit.org/changeset/204400> (bug #160735) to localSelectionRect() with regards to the conditional for the early return.

Additional remarks:

Notice that <https://trac.webkit.org/changeset/204400> (bug #160735) changed the conditional for the early return in InlineTextBox::localSelectionRect() from:

sPos > ePos

to:

sPos >= ePos && !(startPos == endPos && startPos >= start() && startPos <= (start() + len()))

This seems unnecessary. I suspect this change was made to make localSelectionRect() work when passed positions that are completely outside the bounds of the text box. Passing such positions is illogical, represents a programmer error and we (the WebKit OpenSource Project) would be better served by catching such bad usage using an assertion(s) so that we can fix them instead of having localSelectionRect() effectively silently fail.

[Daniel Bates]

(In reply to Daniel Bates from comment #5)
> I am going to update the patch to revert the change made in
> <https://trac.webkit.org/changeset/204400> (bug #160735) to
> localSelectionRect() with regards to the conditional for the early return.
> 
> Additional remarks:
> 
> Notice that <https://trac.webkit.org/changeset/204400> (bug #160735) changed
> the conditional for the early return in InlineTextBox::localSelectionRect()
> from:
> 
> sPos > ePos
> 
> to:
> 
> sPos >= ePos && !(startPos == endPos && startPos >= start() && startPos <=
> (start() + len()))
> 
> This seems unnecessary. I suspect this change was made to make
> localSelectionRect() work when passed positions that are completely outside
> the bounds of the text box. Passing such positions is illogical, represents
> a programmer error and we (the WebKit OpenSource Project) would be better
> served by catching such bad usage using an assertion(s) so that we can fix
> them instead of having localSelectionRect() effectively silently fail.

Disregard these remarks. It seems like the machinery for Element.getClientRects() has changed since r204400 such that that offsets of the DOM range can effectively be passed through to localSelectionRect() with little to no change (as can be seen in test 5 in layout test LayoutTests/fast/dom/Range/getClientRects.html); => it is the responsibility of localSelectionRect() to handle arbitrary positions and clamped them.

[Myles C. Maxfield]

Comment on attachment 323379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323379&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:194
> +    // This function is used to implement Element.getClientRects(), which can return a rectangle with zero

Comments of the form "this is only used by X" often become out-of-date.

[Daniel Bates]

(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 323379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323379&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:194
> > +    // This function is used to implement Element.getClientRects(), which can return a rectangle with zero
> 
> Comments of the form "this is only used by X" often become out-of-date.

Will remove comment before landing. I only added the comment because a FIXME was added that implied there was confusion on how localSelectionRect() should behave for zero-width rectangles.

[Daniel Bates]

(In reply to Daniel Bates from comment #8)
> (In reply to Myles C. Maxfield from comment #7)
> > Comment on attachment 323379 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323379&action=review
> > 
> > > Source/WebCore/rendering/InlineTextBox.cpp:194
> > > +    // This function is used to implement Element.getClientRects(), which can return a rectangle with zero
> > 
> > Comments of the form "this is only used by X" often become out-of-date.
> 
> Will remove comment before landing. I only added the comment because a FIXME
> was added that implied there was confusion on how localSelectionRect()
> should behave for zero-width rectangles.

Forgot to mention, we have test coverage to catch a regression.

[Daniel Bates]

Committed r223196: <https://trac.webkit.org/changeset/223196>

[Radar WebKit Bug Importer]

<rdar://problem/34938702>