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.
The classes derived from FormAssociatedElement are these two classes. HTMLObjectElement HTMLFormControlElements The tags derived from HTMLFormControlElements are: textarea, button, select, output, keygen, fieldset
Created attachment 152153 [details] Patch
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.
Created attachment 152732 [details] Patch
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.
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.
Created attachment 153187 [details] Patch
Looks good to me. I'd ask kent-san for additional comment if any.
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.
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!
Created attachment 154528 [details] Patch
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()
Created attachment 154531 [details] Patch for landing
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
Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13345464
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
Comment on attachment 154531 [details] Patch for landing Attachment 154531 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13343538
Created attachment 154540 [details] Patch for landing
(In reply to comment #13) > Created an attachment (id=154531) [details] > Patch for landing Sorry, I've uploaded a wrong patch...
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
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
Ah, ... since we don't have a special care for object element, I should have changed the test.
Created attachment 154553 [details] Patch for landing
Comment on attachment 154553 [details] Patch for landing Clearing flags on attachment: 154553 Committed r123713: <http://trac.webkit.org/changeset/123713>
All reviewed patches have been landed. Closing bug.