Bug 227016 - Document's execCommand() / queryCommand*() should throw an exception on non-HTML/XHTML documents
Summary: Document's execCommand() / queryCommand*() should throw an exception on non-H...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-14 23:10 PDT by zyscoder@gmail.com
Modified: 2021-07-07 08:23 PDT (History)
13 users (show)

See Also:


Attachments
Patch (15.37 KB, patch)
2021-06-16 09:59 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.26 KB, patch)
2021-06-16 10:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.35 KB, patch)
2021-06-16 11:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (53.58 KB, patch)
2021-06-16 11:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (60.67 KB, patch)
2021-06-16 15:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.20 KB, patch)
2021-06-16 15:32 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zyscoder@gmail.com 2021-06-14 23:10:17 PDT
Steps to reproduce:

(1) Open a tab and navigate to any URL;
(2) Open Devtools and run the following code in the Console:
```
new Document().queryCommandSupported('str');
```
Actual results:

This code would be evaluated successfully and return a value of false, without throwing any exceptions.

Expected results:

Both in Chrome and Firefox throw expcetions:
Chrome: `DOMException: Failed to execute 'queryCommandSupported' on 'Document': queryCommandSupported is only supported on HTML documents.`
Firefox: `queryCommandSupported is only supported on HTML documents.`
It seems better to align with Chrome and Firefox.
Comment 1 Chris Dumez 2021-06-16 09:59:47 PDT
Created attachment 431556 [details]
Patch
Comment 2 Chris Dumez 2021-06-16 10:31:59 PDT
Created attachment 431561 [details]
Patch
Comment 3 EWS Watchlist 2021-06-16 10:32:46 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 Chris Dumez 2021-06-16 11:42:35 PDT
Created attachment 431575 [details]
Patch
Comment 5 Chris Dumez 2021-06-16 11:45:50 PDT
Created attachment 431577 [details]
Patch
Comment 6 Darin Adler 2021-06-16 14:51:44 PDT
Comment on attachment 431577 [details]
Patch

Why are we keeping all these pointless calls to execCommand in SVG tests?
Comment 7 Chris Dumez 2021-06-16 14:58:27 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 431577 [details]
> Patch
> 
> Why are we keeping all these pointless calls to execCommand in SVG tests?

Fair point, I hesitated. I think I'll drop them then.
Comment 8 Chris Dumez 2021-06-16 15:25:25 PDT
Created attachment 431614 [details]
Patch
Comment 9 Chris Dumez 2021-06-16 15:32:09 PDT
Created attachment 431615 [details]
Patch
Comment 10 Chris Dumez 2021-06-16 22:40:00 PDT
Committed r278974 (238900@main): <https://commits.webkit.org/238900@main>
Comment 11 Radar WebKit Bug Importer 2021-06-16 22:40:25 PDT
<rdar://problem/79433559>
Comment 12 Nikolas Zimmermann 2021-07-07 06:15:41 PDT
This broke most SVG text selection tests in LayoutTests/svg/text -- but it was hard to notice, since the failures are only visible when running pixel tests.

They need to be converted to ref tests in the future :-(
Anyhow, the purpose of the tests e.g. text-align-01-b.svg is to select all text and verify that the selection rects are correct -- now no more text is selected. How to select all text in SVG documents w/o using execCommand?
Comment 13 Nikolas Zimmermann 2021-07-07 06:16:49 PDT
Here's a full list of affected tests:

svg/text/foreignObject-text-clipping-bug.xml            
svg/text/text-align-01-b.svg    
svg/text/text-align-02-b.svg    
svg/text/text-align-03-b.svg    
svg/text/text-align-04-b.svg    
svg/text/text-align-05-b.svg    
svg/text/text-align-06-b.svg    
svg/text/text-deco-01-b.svg     
svg/text/text-fonts-01-t.svg    
svg/text/text-fonts-02-t.svg    
svg/text/text-intro-05-t.svg    
svg/text/text-path-01-b.svg     
svg/text/text-spacing-01-b.svg  
svg/text/text-text-01-b.svg     
svg/text/text-text-03-b.svg     
svg/text/text-text-04-t.svg     
svg/text/text-text-05-t.svg     
svg/text/text-text-07-t.svg     
svg/text/text-text-08-b.svg     
svg/text/text-tref-01-b.svg     
svg/text/text-tselect-01-b.svg  
svg/text/text-tspan-01-b.svg    
svg/text/text-ws-01-t.svg       
svg/text/text-ws-02-t.svg
Comment 14 Chris Dumez 2021-07-07 08:23:01 PDT
(In reply to Nikolas Zimmermann from comment #12)
> This broke most SVG text selection tests in LayoutTests/svg/text -- but it
> was hard to notice, since the failures are only visible when running pixel
> tests.
> 
> They need to be converted to ref tests in the future :-(
> Anyhow, the purpose of the tests e.g. text-align-01-b.svg is to select all
> text and verify that the selection rects are correct -- now no more text is
> selected. How to select all text in SVG documents w/o using execCommand?

I guess that's not something that's supported in other browsers (selecting all text in an SVG document). It looks like the tests in questions were dropped in Blink. Instead of dropping them, I intentionally dropped the selection logic from those tests.