Bug 156448

Summary: REGRESSION (r193857): Text selection causes text to disappear.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, mmaxfield, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch none

Description zalan 2016-04-09 16:54:18 PDT
Select/unselect text on http://www.loopinsight.com/2016/04/05/the-loop-magazine-app-is-dead/
Comment 1 zalan 2016-04-09 16:54:41 PDT
rdar://problem/25578952
Comment 2 zalan 2016-04-09 17:08:40 PDT
Created attachment 276099 [details]
Patch
Comment 3 Sam Weinig 2016-04-09 18:23:22 PDT
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?
Comment 4 zalan 2016-04-09 18:27:32 PDT
(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.
Comment 5 Darin Adler 2016-04-10 16:54:00 PDT
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.
Comment 6 zalan 2016-04-10 19:08:49 PDT
Created attachment 276119 [details]
Patch
Comment 7 zalan 2016-04-10 19:12:49 PDT
(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)
Comment 8 Build Bot 2016-04-10 20:00:06 PDT
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
Comment 9 Build Bot 2016-04-10 20:00:09 PDT
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
Comment 10 zalan 2016-04-10 20:14:49 PDT
Created attachment 276124 [details]
Patch
Comment 11 zalan 2016-04-11 10:41:55 PDT
> > 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
Comment 12 Simon Fraser (smfr) 2016-04-11 11:34:20 PDT
Comment on attachment 276124 [details]
Patch

r193857 claimed "no change in functionality" so I'm confused.
Comment 13 WebKit Commit Bot 2016-04-11 12:31:10 PDT
Comment on attachment 276124 [details]
Patch

Clearing flags on attachment: 276124

Committed r199304: <http://trac.webkit.org/changeset/199304>
Comment 14 WebKit Commit Bot 2016-04-11 12:31:14 PDT
All reviewed patches have been landed.  Closing bug.