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: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 77937 | ||||||||||||||||||||
Attachments: |
|
Description
Shinya Kawanaka
2012-07-10 02:28:10 PDT
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. |