90866 – Add User Agent Shadow DOM to FormControlElement just before adding User Shadow DOM.

RESOLVED FIXED 90866

Add User Agent Shadow DOM to FormControlElement just before adding User Shadow DOM.

https://bugs.webkit.org/show_bug.cgi?id=90866

Summary Add User Agent Shadow DOM to FormControlElement just before adding User Shado...

Shinya Kawanaka

Reported 2012-07-10 02:28:10 PDT

User Agent Shadow DOM might be added to FormControlElement for Validation Message lazily. Since our implementation assumes that the User Agent Shadow DOM is the oldest, we don't allow a user to add Author Shadow DOM yet. We should add User Agent Shadow DOM just before a user add Author Shadow DOM. It will allow a user to add Author Shadow DOM.

Attachments
Patch (19.49 KB, patch) 2012-07-12 21:58 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (65.47 KB, patch) 2012-07-17 04:21 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (65.37 KB, patch) 2012-07-18 23:35 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (63.34 KB, patch) 2012-07-25 19:45 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch for landing (63.25 KB, patch) 2012-07-25 20:12 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch for landing (63.21 KB, patch) 2012-07-25 22:18 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-06 (419.25 KB, application/zip) 2012-07-25 23:02 PDT, WebKit Review Bot	no flags	Details
Patch for landing (63.08 KB, patch) 2012-07-25 23:23 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-07-12 18:51:20 PDT

The classes derived from FormAssociatedElement are these two classes. HTMLObjectElement HTMLFormControlElements The tags derived from HTMLFormControlElements are: textarea, button, select, output, keygen, fieldset

Shinya Kawanaka

Comment 2 2012-07-12 21:58:21 PDT

Created attachment 152153 [details] Patch

Hajime Morrita

Comment 3 2012-07-16 19:19:23 PDT

Comment on attachment 152153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152153&action=review > Source/WebCore/ChangeLog:8 > + This patch ensures that UserAgentShadowDOM is the oldest ShadowDOM. Nit: Usually the description mentions it change instead of patch. > Source/WebCore/ChangeLog:14 > + If ValidationMessage does not assume the UserAgentShadowDOM is the oldest, we have to make it the oldest, Got confused. Does ValidationMessage assume it or not? > Source/WebCore/ChangeLog:19 > + HTMLFormControlElement are derived from FormAssociatedElement. So we will a callback to add we will what? > Source/WebCore/html/HTMLFormControlElement.cpp:72 > +ShadowRoot* HTMLFormControlElement::ensureUserAgentShadowRoot() Similar code like this start scattering around here and there. Could you factor them out to kill such duplication? > LayoutTests/fast/dom/shadow/shadowdom-for-form-associated-element-with-validation.html:14 > +var shadowRoot = new WebKitShadowRoot(input); I don't think checking crash is sufficient. How about to turn this into a reftest? > LayoutTests/fast/dom/shadow/shadowdom-for-form-associated-element.html:1 > +<!DOCTYPE html> Ditto. > LayoutTests/fast/dom/shadow/shadowdom-for-select-crash.html:1 > +<!DOCTYPE html> Ditto.

Shinya Kawanaka

Comment 4 2012-07-17 04:21:13 PDT

Created attachment 152732 [details] Patch

Hajime Morrita

Comment 5 2012-07-18 19:20:51 PDT

Comment on attachment 152732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152732&action=review Basically looks good. Let's wait a day or two before landing to have a chance to raise a voice. > Source/WebCore/html/FormAssociatedElement.cpp:73 > + ElementShadow* shadow = element->ensureShadow(); Don't we want to kill ensureShadow() or shall we close Bug 77608? > Source/WebCore/html/ValidationMessage.cpp:138 > + ASSERT(shadowRoot->type() == ShadowRoot::UserAgentShadowRoot); This ASSERT looks too obvious.

Shinya Kawanaka

Comment 6 2012-07-18 22:02:12 PDT

Comment on attachment 152732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152732&action=review >> Source/WebCore/html/FormAssociatedElement.cpp:73 >> + ElementShadow* shadow = element->ensureShadow(); > > Don't we want to kill ensureShadow() or shall we close Bug 77608? ensureShadow() and ensureShadowRoot() is different.

Shinya Kawanaka

Comment 7 2012-07-18 23:35:09 PDT

Created attachment 153187 [details] Patch

Hajime Morrita

Comment 8 2012-07-19 23:11:53 PDT

Looks good to me. I'd ask kent-san for additional comment if any.

Kent Tamura

Comment 9 2012-07-19 23:19:40 PDT

Comment on attachment 153187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153187&action=review > Source/WebCore/html/HTMLObjectElement.cpp:513 > +void HTMLObjectElement::willAddAuthorShadowRoot() > +{ > + ensureUserAgentShadowRoot(); > +} HTMLObjectElement never has a ValidationMessage because it doesn't inherit HTMLFormControlElement. I don't think we need special care for HTMLObjectElement. > Source/WebCore/html/ValidationMessage.cpp:122 > - if (RenderBox* container = bubble->renderer()->containingBlock()) { > - FloatPoint containerLocation = container->localToAbsolute(); > - hostX -= containerLocation.x() + container->borderLeft(); > - hostY -= containerLocation.y() + container->borderTop(); > + if (RenderObject* renderer = bubble->renderer()) { > + if (RenderBox* container = renderer->containingBlock()) { > + FloatPoint containerLocation = container->localToAbsolute(); > + hostX -= containerLocation.x() + container->borderLeft(); > + hostY -= containerLocation.y() + container->borderTop(); > + } Why do we need this change? ChangeLog should have a per-function comment about it.

Shinya Kawanaka

Comment 10 2012-07-20 00:22:30 PDT

Comment on attachment 153187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153187&action=review >> Source/WebCore/html/HTMLObjectElement.cpp:513 >> +} > > HTMLObjectElement never has a ValidationMessage because it doesn't inherit HTMLFormControlElement. > I don't think we need special care for HTMLObjectElement. OK. >> Source/WebCore/html/ValidationMessage.cpp:122 >> + } > > Why do we need this change? ChangeLog should have a per-function comment about it. Yes. This is necessary, in case AuthorShadowRoot does not have a shadow element, renderer will be null here. I'll add a comment on ChangeLog. Thanks!

Shinya Kawanaka

Comment 11 2012-07-25 19:45:11 PDT

Created attachment 154528 [details] Patch

Hajime Morrita

Comment 12 2012-07-25 19:54:59 PDT

Comment on attachment 154528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154528&action=review > Source/WebCore/html/FormAssociatedElement.cpp:75 > + if (ShadowRoot* shadowRoot = shadow->oldestShadowRoot()) { Let's use userAgentShadowRooot()

Shinya Kawanaka

Comment 13 2012-07-25 20:12:54 PDT

Created attachment 154531 [details] Patch for landing

WebKit Review Bot

Comment 14 2012-07-25 20:26:58 PDT

Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13350476

Early Warning System Bot

Comment 15 2012-07-25 20:27:19 PDT

Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13345464

Early Warning System Bot

Comment 16 2012-07-25 20:56:04 PDT

Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13346509

Gyuyoung Kim

Comment 17 2012-07-25 21:18:31 PDT

Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13343538

Shinya Kawanaka

Comment 18 2012-07-25 22:18:32 PDT

Created attachment 154540 [details] Patch for landing

Shinya Kawanaka

Comment 19 2012-07-25 22:19:04 PDT

(In reply to comment #13) > Created an attachment (id=154531) [details] > Patch for landing Sorry, I've uploaded a wrong patch...

WebKit Review Bot

Comment 20 2012-07-25 23:02:41 PDT

Comment on attachment 154540 [details] Patch for landing Attachment 154540 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13345544 New failing tests: fast/dom/shadow/shadowdom-for-form-associated-element-useragent.html

WebKit Review Bot

Comment 21 2012-07-25 23:02:46 PDT

Created attachment 154549 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Shinya Kawanaka

Comment 22 2012-07-25 23:22:48 PDT

Ah, ... since we don't have a special care for object element, I should have changed the test.

Shinya Kawanaka

Comment 23 2012-07-25 23:23:29 PDT

Created attachment 154553 [details] Patch for landing

WebKit Review Bot

Comment 24 2012-07-26 01:12:02 PDT

Comment on attachment 154553 [details] Patch for landing Clearing flags on attachment: 154553 Committed r123713: <http://trac.webkit.org/changeset/123713>

WebKit Review Bot

Comment 25 2012-07-26 01:12:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Shinya Kawanaka

Reported

2012-07-10 02:28 PDT

Modified

2012-07-26 01:12 PDT History

CC List

13 users Show

URL

Keywords

Depends on

Blocks

77937

Dependencies

tree graph