Bug 85463 - REGRESSION (r102553): Smart links do not work
Summary: REGRESSION (r102553): Smart links do not work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Nobody
URL: data:text/html,%3Cdiv%20contenteditab...
Keywords: InRadar, Regression
Depends on:
Blocks: 82198
  Show dependency treegraph
 
Reported: 2012-05-02 23:58 PDT by Alexey Proskuryakov
Modified: 2016-09-16 13:18 PDT (History)
14 users (show)

See Also:


Attachments
simple fix (2.11 KB, patch)
2012-05-14 08:48 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
add a manual test (4.55 KB, patch)
2012-05-16 11:13 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
add layouttest (18.97 KB, patch)
2012-05-17 09:02 PDT, Yi Shen
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
fix style and build issues (20.24 KB, patch)
2012-05-17 10:17 PDT, Yi Shen
rniwa: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-05-02 23:58:38 PDT
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.
Comment 1 Alexey Proskuryakov 2012-05-03 00:01:09 PDT
Other substitutions appear to work fine.
Comment 2 Adele Peterson 2012-05-03 11:16:16 PDT
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"
Comment 3 Adele Peterson 2012-05-03 11:17:00 PDT
<rdar://problem/11374517>
Comment 4 Ryosuke Niwa 2012-05-03 11:57:58 PDT
Is there anyway to test smart links in DRT?
Comment 5 Alexey Proskuryakov 2012-05-03 13:34:55 PDT
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.
Comment 6 Yi Shen 2012-05-14 08:48:28 PDT
Created attachment 141737 [details]
simple fix
Comment 7 Tony Chang 2012-05-14 11:49:36 PDT
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?
Comment 8 Ryosuke Niwa 2012-05-14 11:59:07 PDT
(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?
Comment 9 Alexey Proskuryakov 2012-05-14 12:14:50 PDT
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.
Comment 10 Yi Shen 2012-05-16 06:39:50 PDT
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.
Comment 11 Yi Shen 2012-05-16 11:13:04 PDT
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 :)
Comment 12 Ryosuke Niwa 2012-05-16 11:26:13 PDT
(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.
Comment 13 Yi Shen 2012-05-17 09:02:43 PDT
Created attachment 142485 [details]
add layouttest
Comment 14 WebKit Review Bot 2012-05-17 09:05:31 PDT
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.
Comment 15 Early Warning System Bot 2012-05-17 09:19:18 PDT
Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12725150
Comment 16 Early Warning System Bot 2012-05-17 09:21:52 PDT
Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12726163
Comment 17 WebKit Review Bot 2012-05-17 09:54:41 PDT
Comment on attachment 142485 [details]
add layouttest

Attachment 142485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717564
Comment 18 Yi Shen 2012-05-17 10:17:50 PDT
Created attachment 142497 [details]
fix style and build issues
Comment 19 Ryosuke Niwa 2012-05-17 10:21:24 PDT
Comment on attachment 142497 [details]
fix style and build issues

Thanks for the test!
Comment 20 Yi Shen 2012-05-17 10:38:50 PDT
(In reply to comment #19)
> (From update of attachment 142497 [details])
> Thanks for the test!

Thanks for the review :)
Comment 21 WebKit Review Bot 2012-05-17 11:53:44 PDT
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
Comment 22 WebKit Review Bot 2012-05-18 02:58:45 PDT
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
Comment 23 Yi Shen 2012-05-18 07:26:00 PDT
Committed r117590: <http://trac.webkit.org/changeset/117590>
Comment 24 Jessie Berlin 2012-05-22 13:46:08 PDT
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.
Comment 25 Yi Shen 2012-05-22 14:21:51 PDT
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.