Bug 34915 – Chromium: Need to be able to get the bounds of selection rectangle(s)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Dmitriy Belenko]

This change is necessary to fix Chromium bug 28646 (which in turn will fix 23481 and 23481	). The gist of that bug is that a red rectangle needs to be drawn around the selection, including the case where selection is transformed.
The change will be in WebKit/chromium and is Chromium-specific. I already have a fix (diff is attached).

The bug is caused by not having a way to retrieve the bounding rectangle coordinates for the selection rectangle(s). WebKit tests do this by calling an Objective-C selector dynamically and retrieving the corresponding rect. In order to accomplish the same in C++ a trivial helper method is introduced in the interface.

I am creating this bug to have a vehicle to get the patch in.

The corresponding Chromium bug can be found here: http://code.google.com/p/chromium/issues/detail?id=28646&q=dbelenko&colspec=ID%20Stars%20Pri%20Area%20Type%20Status%20Summary%20Modified%20Owner%20Mstone%20OS

[Dmitriy Belenko]

Created attachment 48686 [details]
Patch that fixes the issue.

Proposed patch. I will follow up with the proper submission for review.

[Dmitriy Belenko]

Created attachment 48691 [details]
This patch includes the changelog and stylistic changes as per style guide. It obsoletes the previously attached patch.

Attaching a patch which includes the changelog and stylistic changes as per style guide. It obsoletes the previously attached patch.

This patch adds a selectionBoundsRect method. This method returns false if there is no selection. In case there is a selection, it sets the coordinates of the rectangle passed into it by reference to the absolute bounds enclosing all selections. I have verified that the boundary is identical to the one shown in Apple's "expected" images.

[WebKit Review Bot]

Attachment 48691 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitriy Belenko]

Created attachment 48700 [details]
Fixed the spurious tab chars in Changelog

Fixed the spurious tab chars in Changelog

[Dmitriy Belenko]

See http://code.google.com/p/chromium/issues/detail?id=28646&q=lttf&colspec=ID%20Stars%20Pri%20Area%20Type%20Status%20Summary%20Modified%20Owner%20Mstone%20OS

This is the corresponding Chromium bug.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48700 [details]
Fixed the spurious tab chars in Changelog


> +        Reviewed by NOBODY (OOPS!).
> +
> +        This change will enable 30 layout pixel tests to pass. The change
> +        simply exposes a helper function to return a bounding rectangle for the
> +        current (possibly transformed) selection rectangles. The displayed
> +        rectangle is not transformed, but it does adjust itself to completely
> +        enclose the transformed selection. In WebKit Apple makes this test work
> +        by using dynamic dispatch in Objective C, but here I had to introduce a
> +        method.
> +
> +        This change does not require new test cases.
> +
> +        Chromium: Need to be able to get the bounds of selection rectangle(s)
> +        https://bugs.webkit.org/show_bug.cgi?id=34915

^^^ please move these two lines to the top, just below the Reviewed by line.  That
way tools that just show a snippet of the ChangeLog will show those lines.


> +++ b/WebKit/chromium/public/WebFrame.h
> @@ -478,6 +478,12 @@ public:
>      // Returns a text representation of the render tree.  This method is used
>      // to support layout tests.
>      virtual WebString renderTreeAsText() const = 0;
> +  
> +    // Returns bool true if there is a selection and false if there's none.
> +    // In case there is a selection, sets the rect to the "straight" bounding 
> +    // rect of the (possibly transformed) selection rect.
> +    // This method is used to support layout tests.
> +    virtual bool selectionBoundsRect(WebRect&) const = 0;

This should just return WebRect.  If the WebRect is empty, then it can mean
that there was no selection.


> +bool WebFrameImpl::selectionBoundsRect(WebRect& boundsRect) const 
> +{
> +    if (hasSelection()) {
> +        FloatRect r = frame()->selectionBounds(false);
> +        boundsRect.x = r.x();
> +        boundsRect.y = r.y();
> +        boundsRect.width = r.width();
> +        boundsRect.height = r.height();

Is the loss of precision from float to int okay?

-Darin

[Dmitriy Belenko]

Created attachment 49029 [details]
Changed patch to address reviewer feedback.

Code now simply returns a WebRect, and uses rounding (as opposed to truncation) to convert coordinates to integer.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49029 [details]
Changed patch to address reviewer feedback.

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 4e8a629..0d90fd0 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,20 @@
> [phone]  Dmitriy Belenko  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This change will enable about 30 test cases to pass in Chromium.
> +        All of these test cases are related to selection rect boundaries.
> +        The three issues that this will solve can be found here:
> +
> +        http://code.google.com/p/chromium/issues/detail?id=28646
> +        http://code.google.com/p/chromium/issues/detail?id=23481
> +        http://code.google.com/p/chromium/issues/detail?id=23482

I'm sorry to nit-pick, but you should really just reference the WebKit
bug number in the WebKit ChangeLog.  (The bug can have references to these
Chromium bug reports.)


> +++ b/WebKit/chromium/public/WebFrame.h
> @@ -492,6 +492,11 @@ public:
>                                           float pageWidthInPixels,
>                                           float pageHeightInPixels) const = 0;
>  
> +    // Returns the bounds rect for current selection. If selection is performed
> +    // on transformed text, the rect will still bound the selection, but will
> +    // not be transformed itself. If no selection is present the rect will be
> +    // empty ((0,0), (0,0))
> +    virtual WebRect selectionBoundsRect() const = 0;
>  protected:

nit: please preserve the new line above the "protected:" line.


> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
...
> +WebRect WebFrameImpl::selectionBoundsRect() const
> +{
> +    if (hasSelection()) {
> +        FloatRect fr = frame()->selectionBounds(false);
> +        return WebRect(roundf(fr.x()), roundf(fr.y()),
> +                       roundf(fr.width()), roundf(fr.height()));

Are you sure we want to round like this?  IntRect(const FloatRect&) just rounds
down.


> +    }
> +
> +    return WebRect(0, 0, 0, 0);

nit:  Just do "return WebRect();" since the default constructor does the same thing.

[Dmitriy Belenko]

Created attachment 49107 [details]
Incorporated review feedback.

Third time's a charm?

[Darin Fisher (:fishd, Google)]

Comment on attachment 49107 [details]
Incorporated review feedback.

> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
> @@ -1534,6 +1534,16 @@ int WebFrameImpl::pageNumberForElementById(const WebString& id,
>      return PrintContext::pageNumberForElement(element, pageSize);
>  }
>  
> +WebRect WebFrameImpl::selectionBoundsRect() const
> +{
> +    if (hasSelection()) {
> +        FloatRect fr = frame()->selectionBounds(false);
> +        return WebRect(fr.x(), fr.y(), fr.width(), fr.height());

if (hasSelection())
    return WebRect(frame()->selectionBounds(false));

^^^ that should also compile, right?  the FloatRect gets implicitly converted
to IntRect, which matches a WebRect constructor.

[Dmitriy Belenko]

Created attachment 49116 [details]
Made things more concise

[Dmitriy Belenko]

Created attachment 49185 [details]
Using implicit conversion per Darin's request.

[David Levin]

Comment on attachment 49185 [details]
Using implicit conversion per Darin's request.

Some trivial nits to fix and then I'd be willing to r+, cq+ (based on this plus past reviews).

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,21 @@
> [phone]  Dmitriy Belenko  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
Typically one does: 

Bug title
Bug link

Description.

Like this:
> +        Chromium: Need to be able to get the bounds of selection rectangle(s) 
> +        https://bugs.webkit.org/show_bug.cgi?id=34915
> +
> +        This change will enable about 30 test cases to pass in Chromium.
> +        All of these test cases are related to selection rect boundaries.
> +        This change will enable the test cases to retrieve the selection
> +        rect boundary rectangle for the current selection.
> +
> +
> +        * public/WebFrame.h:
> +        * src/WebFrameImpl.cpp:
> +        (WebKit::WebFrameImpl::selectionBoundsRect):
> +        * src/WebFrameImpl.h:
> +

> diff --git a/WebKit/chromium/public/WebFrame.h b/WebKit/chromium/public/WebFrame.h
> +    // Returns the bounds rect for current selection. If selection is performed
> +    // on transformed text, the rect will still bound the selection, but will

no comma before the but (since the phrase after but isn't a complete sentence.


> +    // not be transformed itself. If no selection is present the rect will be

Add a comma after present.

> +    // empty ((0,0), (0,0))

Add a period to the end of the sentence.

[Dmitriy Belenko]

Created attachment 49264 [details]
Fixed punctuation issues per David's feedback.

[WebKit Commit Bot]

Comment on attachment 49264 [details]
Fixed punctuation issues per David's feedback.

Clearing flags on attachment: 49264

Committed r55137: <http://trac.webkit.org/changeset/55137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.