Bug 85463 – REGRESSION (r102553): Smart links do not work

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

Substitutions > Smart Links don't work in nightly builds. They work in Safari 5.1.5.

Steps to reproduce.

1. Open bug URL (it's just a basic contenteditable div)
2. Make sure that automatic link detection is enabled (Edit > Substitutions > Smart Links).
3. Type "www.apple.com " (without quotes, with a space at the end).

Results: it should be linkified, but it no longer is.

[Alexey Proskuryakov]

Other substitutions appear to work fine.

[Adele Peterson]

Regressed in http://trac.webkit.org/changeset/102553
To fix: https://bugs.webkit.org/show_bug.cgi?id=73616
"Asynchronous path and synchronous path of SpellChecker should share the code to mark misspellings"

[Adele Peterson]

<rdar://problem/11374517>

[Ryosuke Niwa]

Is there anyway to test smart links in DRT?

[Alexey Proskuryakov]

I would expect that capturing its working is possible with textInputController. It's relatively easy to see the calls if you enable TextInput log channel.

[Yi Shen]

Created attachment 141737 [details]
simple fix

[Tony Chang]

Comment on attachment 141737 [details]
simple fix

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

> Source/WebCore/ChangeLog:11
> +        No new tests - fix the regression

Is it possible to write a layout test for this?  If not, should we add a manual test?

[Ryosuke Niwa]

(In reply to comment #7)
> (From update of attachment 141737 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141737&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests - fix the regression
> 
> Is it possible to write a layout test for this?  If not, should we add a manual test?

Maybe ap would know?

[Alexey Proskuryakov]

Comment on attachment 141737 [details]
simple fix

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

>>> Source/WebCore/ChangeLog:11
>>> +        No new tests - fix the regression
>> 
>> Is it possible to write a layout test for this?  If not, should we add a manual test?
> 
> Maybe ap would know?

I don't know how to exercise this code in a regression test.

"fix the regression" is not a good explanation of why there are no tests though.

[Yi Shen]

Thanks for the comments. To enable the smart link test in DRT, we need to add a new interface like setLinkDetectionEnabled in LayoutTestController. I can open a separated bug and work on it.

[Yi Shen]

Created attachment 142304 [details]
add a manual test

To enable the layouttest for the smart link, it requires adding a new API in WebView which may take a while for discussing. In addition, Smart links is not included in w3c spec, and I am not sure whether we should have it in LayoutTest. 

So, add a manual test for this small patch to help passing the review :)

[Ryosuke Niwa]

(In reply to comment #11)
> Created an attachment (id=142304) [details]
> add a manual test
> 
> To enable the layouttest for the smart link, it requires adding a new API in WebView which may take a while for discussing. In addition, Smart links is not included in w3c spec, and I am not sure whether we should have it in LayoutTest. 

I don't think whether something is included in W3C spec or not is a good indication as to whether we should write a test or not since layout tests aren't conformance tests.

[Yi Shen]

Created attachment 142485 [details]
add layouttest

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:852:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Tools/DumpRenderTree/LayoutTestController.cpp:1724:  Tab found; better to use spaces  [whitespace/tab] [1]
Tools/DumpRenderTree/wx/LayoutTestControllerWx.cpp:649:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:1524:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 4 in 22 files


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

[Early Warning System Bot]

Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12725150

[Early Warning System Bot]

Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12726163

[WebKit Review Bot]

Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717564

[Yi Shen]

Created attachment 142497 [details]
fix style and build issues

[Ryosuke Niwa]

Comment on attachment 142497 [details]
fix style and build issues

Thanks for the test!

[Yi Shen]

(In reply to comment #19)
> (From update of attachment 142497 [details])
> Thanks for the test!

Thanks for the review :)

[WebKit Review Bot]

Comment on attachment 142497 [details]
fix style and build issues

Rejecting attachment 142497 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 saving rejects to file LayoutTests/platform/efl/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/test_expectations.txt
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 2480 (offset -4 lines).
patching file LayoutTests/platform/qt/test_expectations.txt
patching file LayoutTests/platform/win/Skipped

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12719568

[WebKit Review Bot]

Comment on attachment 142497 [details]
fix style and build issues

Rejecting attachment 142497 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pectations.txt
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 2480 (offset -4 lines).
patching file LayoutTests/platform/qt/test_expectations.txt
Hunk #1 FAILED at 72.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/test_expectations.txt.rej
patching file LayoutTests/platform/win/Skipped

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12722765

[Yi Shen]

Committed r117590: <http://trac.webkit.org/changeset/117590>

[Jessie Berlin]

Please try to remember WKTR when you add something to DRT: http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r118030%20(7431)/editing/inserting/typing-space-to-trigger-smart-link-pretty-diff.html

I will be adding the test to the wk2 skipped list.

[Yi Shen]

Thanks for the help!
(In reply to comment #24)
> Please try to remember WKTR when you add something to DRT: http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r118030%20(7431)/editing/inserting/typing-space-to-trigger-smart-link-pretty-diff.html
> I will be adding the test to the wk2 skipped list.