[webkit-reviews] review denied: [Bug 118287] SelectAll on a page does not work if first/last element within the body is contenteditable : [Attachment 206388] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied vanivhegde at gmail.com's request for
review:
Bug 118287: SelectAll on a page does not work if first/last element within the
body is contenteditable
https://bugs.webkit.org/show_bug.cgi?id=118287

Attachment 206388: Patch
https://bugs.webkit.org/attachment.cgi?id=206388&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206388&action=review


> Source/WebCore/ChangeLog:20
> +	   Also, executing selectAll without focusing anywhere on the page
> +	   should not set focus on a contenteditable element if it happens to
be
> +	   the first/last element within the body.

This should be fixed as a separate bug. r- because due to this unrelated
change.

> Source/WebCore/editing/VisiblePosition.cpp:557
> +    // If the html element is non-editable and next/previous candidates have
editable roots,
> +    // then consider them to be in same editable element
> +    if (!editingRoot && node && node->hasTagName(htmlTag)) {
> +	   prevIsInSameEditableElement = prevNode &&
!prevNode->isRootEditableElement();
> +	   nextIsInSameEditableElement = nextNode &&
!nextNode->isRootEditableElement();
> +    } else {

Why? This is what comment, which is obvious from the code.
We need to explain why we do this. In fact, this doesn't look right.

> Source/WebCore/editing/VisibleSelection.cpp:567
> +    // If selection start & end positions are first & last elements of the
body respectively,
> +    // it means the entire page is selected. No adjustments required.

Doesn't. You can make head element visible. You can also insert a random
element before/after body and make it editable.
r- because of this wrong assumption as well.