Bug 65738

Summary: [meta] User agent shadow node with -webkit-user-modify:read-write causes problems
Product: WebKit Reporter: Kent Tamura <tkent>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, dglazkov, dominicc, haraken, macpherson, menard, morrita, rniwa, shinyak, shinyak, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: data:text/html,<style>input::-webkit-textfield-decoration-container { -webkit-user-modify: read-write; }</style><input type=search>
Bug Depends on: 92199, 92200, 92217    
Bug Blocks: 82697    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kent Tamura 2011-08-04 19:31:46 PDT
This is very similar to Bug 65362.

1. Load data:text/html,<style>input::-webkit-textfield-decoration-container { -webkit-user-modify: read-write; }</style><input type=search>
2. Focus on the search field
3. Press backward-delete key
4. Crash

Another related issue:
1. Load data:text/html,<style>input::-webkit-search-results-button { -webkit-user-modify: read-write; }</style><input type=search results=20>
2. Right-click on the magnifier button.
3. You can type into the magnifier button.
Comment 1 Ryosuke Niwa 2011-08-04 19:35:35 PDT
(In reply to comment #0)
> 1. Load data:text/html,<style>input::-webkit-textfield-decoration-container { -webkit-user-modify: read-write; }</style><input type=search>
> 2. Focus on the search field
> 3. Press backward-delete key
> 4. Crash
> 
> Another related issue:
> 1. Load data:text/html,<style>input::-webkit-search-results-button { -webkit-user-modify: read-write; }</style><input type=search results=20>
> 2. Right-click on the magnifier button.
> 3. You can type into the magnifier button.

I don't think we should allow these styles.
Comment 2 Ryosuke Niwa 2011-08-04 19:45:55 PDT
I mean the problem is that inputType's are holding onto nodes that have been removed from the document, and RenderTextControlSingleLine is blowing up because it assumes that they have renderer (which they have also when they were removed from the document).

The only way we could prevent this crash is if we either detected those cases where buttons were edited by the user and we have to ignore them in RenderTextControlSingleLine or editing code became aware of nodes that may be special for input types and avoided deleting/moving them.

I don't think the latter is realistic in that there are so many places this could happen.  The former is possible but I don't think it's really worth the effort because there are so many places where we depend on the existence of those nodes.
Comment 3 Kent Tamura 2011-08-04 20:08:42 PDT
(In reply to comment #1)
> I don't think we should allow these styles.

Yes.  The fix would be to add -webkit-user-modify:read-only to html.css.
Comment 4 Kent Tamura 2011-08-04 20:08:59 PDT
(In reply to comment #3)
> (In reply to comment #1)
> > I don't think we should allow these styles.
> 
> Yes.  The fix would be to add -webkit-user-modify:read-only to html.css.

I meant -webkit-user-modify:read-only !important.
Comment 5 Ryosuke Niwa 2011-08-04 20:14:04 PDT
(In reply to comment #4)
> I meant -webkit-user-modify:read-only !important.

Would that still work if author added "-webkit-user-modify: read-write !important"?
Comment 6 Kent Tamura 2011-08-04 21:24:14 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I meant -webkit-user-modify:read-only !important.
> 
> Would that still work if author added "-webkit-user-modify: read-write !important"?

Authors and users can't change values with !important.

Other examples:

data:text/html,<style>progress { -webkit-appearance: none; } progress::-webkit-progress-bar {-webkit-user-modify:read-write; }</style><progress value=50 max=100></progress>
data:text/html,<style>meter { -webkit-appearance: none; } meter::-webkit-meter-bar {-webkit-user-modify:read-write; }</style><meter value=50 max=100></meter>

We can type into <progress> and <meter>, and can remove the value block.
Comment 7 Ryosuke Niwa 2011-08-04 22:51:05 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I meant -webkit-user-modify:read-only !important.
> > 
> > Would that still work if author added "-webkit-user-modify: read-write !important"?
> 
> Authors and users can't change values with !important.

Okay. Great!  But we should definitely have a test for that just in case.

> Other examples:
> 
> data:text/html,<style>progress { -webkit-appearance: none; } progress::-webkit-progress-bar {-webkit-user-modify:read-write; }</style><progress value=50 max=100></progress>
> data:text/html,<style>meter { -webkit-appearance: none; } meter::-webkit-meter-bar {-webkit-user-modify:read-write; }</style><meter value=50 max=100></meter>
> 
> We can type into <progress> and <meter>, and can remove the value block.

Should I write a patch, or will you?
Comment 8 Kent Tamura 2011-08-04 23:01:27 PDT
(In reply to comment #7)
> Should I write a patch, or will you?

I don't think you should write a patch.
I feel this issue is in the shadow DOM area, rather than HTML Editing or Forms.
Comment 9 Ryosuke Niwa 2011-08-04 23:25:32 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Should I write a patch, or will you?
> 
> I don't think you should write a patch.
> I feel this issue is in the shadow DOM area, rather than HTML Editing or Forms.

Sure. Although I've gotten quite familiar with all these shadow DOM stuff by now.
Comment 10 Hajime Morrita 2012-03-06 00:15:19 PST
Created attachment 130313 [details]
Patch
Comment 11 Kent Tamura 2012-03-06 00:20:42 PST
Comment on attachment 130313 [details]
Patch

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

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:72
> +    RefPtr<RenderStyle> style = document()->styleSelector()->styleForElement(this, 0, true);
> +    style->setUserModify(READ_ONLY);

Is it safe to update a shared RenderStyle?
Comment 12 Hajime Morrita 2012-03-06 00:41:30 PST
Created attachment 130318 [details]
Patch
Comment 13 Hajime Morrita 2012-03-06 00:43:27 PST
(In reply to comment #11)
> Is it safe to update a shared RenderStyle?
Good question. No! We need a fresh RenderStyle object.
I updated the patch. Could you take another look please?
Comment 14 Kent Tamura 2012-03-06 00:48:16 PST
Comment on attachment 130318 [details]
Patch

BTW, I feel this is an overkill.

Adding
 -webkit-user-modify: read-only !important;
to the UA stylesheet is enough.
Comment 15 Hajime Morrita 2012-03-06 01:59:02 PST
Created attachment 130337 [details]
Patch
Comment 16 Kent Tamura 2012-03-06 02:14:37 PST
Comment on attachment 130337 [details]
Patch

ok
Comment 17 WebKit Review Bot 2012-03-06 03:02:56 PST
Comment on attachment 130337 [details]
Patch

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

New failing tests:
editing/selection/3690703-2.html
editing/execCommand/button.html
editing/selection/4397952.html
Comment 18 Hajime Morrita 2012-03-06 20:18:32 PST
Comment on attachment 130337 [details]
Patch

Found this doesn't work. ews is correct.
Comment 19 Eric Seidel (no email) 2012-03-27 12:45:07 PDT
Comment on attachment 130337 [details]
Patch

Cleared Kent Tamura's review+ from obsolete attachment 130337 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 20 Shinya Kawanaka 2012-07-24 19:12:56 PDT
Fixing input element is harder than others. Let's try to fix element by element...
Comment 21 Shinya Kawanaka 2012-07-24 23:18:21 PDT
Oh, I found a bug of the suggested patch.
We should not have -webkit-user-modify: read-only for <input> or <button> itself!!
Comment 22 Shinya Kawanaka 2012-07-24 23:19:40 PDT
(In reply to comment #21)
> Oh, I found a bug of the suggested patch.
> We should not have -webkit-user-modify: read-only for <input> or <button> itself!!

Any way, I'll try to fix this bug one by one...
Comment 23 Shinya Kawanaka 2012-08-06 21:49:01 PDT
Since all dependent bugs are closed, let's close this also.
If you think it's not appropriate, please feel free to reopen.