Bug 90866

Summary:

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

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

DOM

Assignee:

Shinya Kawanaka <shinyak>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, dominicc, gustavo, gyuyoung.kim, hayato, mifenton, morrita, philn, rakuco, tasak, tkent, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

77937

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none
Archive of layout-test-results from gce-cr-linux-06	none
Patch for landing	none

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.