RESOLVED FIXED 65738
[meta] User agent shadow node with -webkit-user-modify:read-write causes problems
https://bugs.webkit.org/show_bug.cgi?id=65738
Summary [meta] User agent shadow node with -webkit-user-modify:read-write causes prob...
Kent Tamura
Reported 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.
Attachments
Patch (6.43 KB, patch)
2012-03-06 00:15 PST, Hajime Morrita
no flags
Patch (6.43 KB, patch)
2012-03-06 00:41 PST, Hajime Morrita
no flags
Patch (10.99 KB, patch)
2012-03-06 01:59 PST, Hajime Morrita
no flags
Ryosuke Niwa
Comment 1 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.
Ryosuke Niwa
Comment 2 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.
Kent Tamura
Comment 3 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.
Kent Tamura
Comment 4 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.
Ryosuke Niwa
Comment 5 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"?
Kent Tamura
Comment 6 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.
Ryosuke Niwa
Comment 7 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?
Kent Tamura
Comment 8 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.
Ryosuke Niwa
Comment 9 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.
Hajime Morrita
Comment 10 2012-03-06 00:15:19 PST
Kent Tamura
Comment 11 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?
Hajime Morrita
Comment 12 2012-03-06 00:41:30 PST
Hajime Morrita
Comment 13 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?
Kent Tamura
Comment 14 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.
Hajime Morrita
Comment 15 2012-03-06 01:59:02 PST
Kent Tamura
Comment 16 2012-03-06 02:14:37 PST
Comment on attachment 130337 [details] Patch ok
WebKit Review Bot
Comment 17 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
Hajime Morrita
Comment 18 2012-03-06 20:18:32 PST
Comment on attachment 130337 [details] Patch Found this doesn't work. ews is correct.
Eric Seidel (no email)
Comment 19 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.
Shinya Kawanaka
Comment 20 2012-07-24 19:12:56 PDT
Fixing input element is harder than others. Let's try to fix element by element...
Shinya Kawanaka
Comment 21 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!!
Shinya Kawanaka
Comment 22 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...
Shinya Kawanaka
Comment 23 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.
Note You need to log in before you can comment on or make changes to this bug.