Bug 156448 – REGRESSION (r193857): Text selection causes text to disappear.

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

[zalan]

Select/unselect text on http://www.loopinsight.com/2016/04/05/the-loop-magazine-app-is-dead/

[zalan]

rdar://problem/25578952

[zalan]

Created attachment 276099 [details]
Patch

[Sam Weinig]

Comment on attachment 276099 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Unable to test. Range selection does not trigger the bug.

What do you mean by this? Do you mean setting the selection using window.getSelection() doesn't trigger the bug? Can you use eventSender to trigger it?

[zalan]

(In reply to comment #3)
> Comment on attachment 276099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276099&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Unable to test. Range selection does not trigger the bug.
> 
> What do you mean by this? Do you mean setting the selection using
> window.getSelection() doesn't trigger the bug? Can you use eventSender to
> trigger it?
I'll try that.

[Darin Adler]

Comment on attachment 276099 [details]
Patch

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

Code change looks fine, but it’s pretty important to add the test case.

I can’t find the bug fix, with all the refactoring. Maybe the TextPainter.cpp part is the bug fix and the rest is just refactoring?

I suggest landing all the refactoring first, without the bug fix, if we don’t have the test yet. r=me for the refactoring

> Source/WebCore/rendering/InlineTextBox.cpp:603
> +        return std::make_pair(0, m_len);

I think this can be:

    return { 0, m_len };

> Source/WebCore/rendering/InlineTextBox.cpp:606
> +    int selectionStart;
> +    int selectionEnd;

Seems like this could just be start and end.

> Source/WebCore/rendering/InlineTextBox.cpp:607
> +    renderer().selectionStartEnd(selectionStart, selectionEnd);

Peculiar that this uses two out arguments and doesn’t even have get in its name. Should fix that.

> Source/WebCore/rendering/InlineTextBox.cpp:614
> +    return std::make_pair(selectionStart, selectionEnd);

Should be:

    return { selectionStart, selectionEnd };

But I would combine the last three lines:

    return { std::max(selectionStart - m_start, 0), std::min<int>(selectionEnd - m_start, m_len) };

> Source/WebCore/rendering/InlineTextBox.cpp:681
> +    int selectionEnd = std::min(endPos - offset, static_cast<int>(m_len));

I think it’s slightly nicer to write this like this:

    int selectionEnd = std::min<int>(endPos - offset, m_len);

> Source/WebCore/rendering/InlineTextBox.h:123
> +    std::pair<int, int> selectionStartEnd() const;

In other cases like this we sometimes use structures with names for the members rather than using a tuple. Sure would be nice to get rid of some those pairs of local variables and arguments using the same structure.

[zalan]

Created attachment 276119 [details]
Patch

[zalan]

(In reply to comment #5)
> Comment on attachment 276099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276099&action=review
> 
> Code change looks fine, but it’s pretty important to add the test case.
> 
> I can’t find the bug fix, with all the refactoring. Maybe the
> TextPainter.cpp part is the bug fix and the rest is just refactoring?
> 
> I suggest landing all the refactoring first, without the bug fix, if we
> don’t have the test yet. r=me for the refactoring
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:603
> > +        return std::make_pair(0, m_len);
> 
> I think this can be:
> 
>     return { 0, m_len };
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:606
> > +    int selectionStart;
> > +    int selectionEnd;
> 
> Seems like this could just be start and end.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:607
> > +    renderer().selectionStartEnd(selectionStart, selectionEnd);
> 
> Peculiar that this uses two out arguments and doesn’t even have get in its
> name. Should fix that.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:614
> > +    return std::make_pair(selectionStart, selectionEnd);
> 
> Should be:
> 
>     return { selectionStart, selectionEnd };
> 
> But I would combine the last three lines:
> 
>     return { std::max(selectionStart - m_start, 0),
> std::min<int>(selectionEnd - m_start, m_len) };
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:681
> > +    int selectionEnd = std::min(endPos - offset, static_cast<int>(m_len));
> 
> I think it’s slightly nicer to write this like this:
> 
>     int selectionEnd = std::min<int>(endPos - offset, m_len);
> 
> > Source/WebCore/rendering/InlineTextBox.h:123
> > +    std::pair<int, int> selectionStartEnd() const;
> 
> In other cases like this we sometimes use structures with names for the
> members rather than using a tuple. Sure would be nice to get rid of some
> those pairs of local variables and arguments using the same structure.
Thanks for the comments. I'll land the refactoring patch separately. (bug 156459)

[Build Bot]

Comment on attachment 276119 [details]
Patch

Attachment 276119 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1134148

New failing tests:
fast/text/text-disappear-on-deselect.html

[Build Bot]

Created attachment 276123 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[zalan]

Created attachment 276124 [details]
Patch

[zalan]

> > Source/WebCore/rendering/InlineTextBox.cpp:607
> > +    renderer().selectionStartEnd(selectionStart, selectionEnd);
> 
> Peculiar that this uses two out arguments and doesn’t even have get in its
> name. Should fix that.
> > Source/WebCore/rendering/InlineTextBox.h:123
> > +    std::pair<int, int> selectionStartEnd() const;
> 
> In other cases like this we sometimes use structures with names for the
> members rather than using a tuple. Sure would be nice to get rid of some
> those pairs of local variables and arguments using the same structure.
https://bugs.webkit.org/show_bug.cgi?id=156467

[Simon Fraser (smfr)]

Comment on attachment 276124 [details]
Patch

r193857 claimed "no change in functionality" so I'm confused.

[WebKit Commit Bot]

Comment on attachment 276124 [details]
Patch

Clearing flags on attachment: 276124

Committed r199304: <http://trac.webkit.org/changeset/199304>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.