Bug 162008 - dictionaryPopupInfoForRange() can change selection temporarily; updates should not be sent to the UIProcess.
Summary: dictionaryPopupInfoForRange() can change selection temporarily; updates shoul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-14 23:39 PDT by Beth Dakin
Modified: 2016-09-15 11:00 PDT (History)
5 users (show)

See Also:

Attachments
Patch (4.29 KB, patch)
2016-09-14 23:43 PDT, Beth Dakin 		no flags Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2016-09-15 10:19 PDT, Beth Dakin 		thorton: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2016-09-14 23:39:24 PDT 
dictionaryPopupInfoForRange() can change selection temporarily; updates should not be sent to the UIProcess.

rdar://problem/28312297
Comment 1 Beth Dakin 2016-09-14 23:43:16 PDT 
Created attachment 288931 [details]
Patch
Comment 2 Wenson Hsieh 2016-09-15 07:47:08 PDT 
Comment on attachment 288931 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:420
> +    m_isGettingDictionaryPopupInfo = true;

Can we use a TemporaryChange<bool> here?
Comment 3 Beth Dakin 2016-09-15 10:19:31 PDT 
(In reply to comment #2)
> Comment on attachment 288931 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288931&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:420
> > +    m_isGettingDictionaryPopupInfo = true;
> 
> Can we use a TemporaryChange<bool> here?

Oh, yes!
Comment 4 Beth Dakin 2016-09-15 10:19:50 PDT 
Created attachment 288966 [details]
Patch
Comment 5 Tim Horton 2016-09-15 10:52:15 PDT 
Comment on attachment 288966 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4841
> +    // Specifically, if there is a caret selection, it will change to a range selection of the word around the caret. And
> +    // then it will change back.

Anything that makes a TextIndicator (among probably a few other things) will have this problem too. Find-in-page, force touch, etc.
Comment 6 Beth Dakin 2016-09-15 11:00:36 PDT 
(In reply to comment #5)
> Comment on attachment 288966 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288966&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4841
> > +    // Specifically, if there is a caret selection, it will change to a range selection of the word around the caret. And
> > +    // then it will change back.
> 
> Anything that makes a TextIndicator (among probably a few other things) will
> have this problem too. Find-in-page, force touch, etc.

This will cover many of those cases, but not find in page. https://trac.webkit.org/changeset/205983

Thanks Tim!