Bug 48980 – Implement form validation message UI

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Implement form validation message UI by shadow DOM

[Kent Tamura]

Add Morita-san because the patch will change ShadowElement.{cpp.h}.

[Kent Tamura]

Created attachment 72916 [details]
Validation message appearance by actual code

[Kent Tamura]

(In reply to comment #2)
> Created an attachment (id=72916) [details]
> Validation message appearance by actual code

This is <input type=url title="Specify URL"> with input.validaity.typeMismatch==true.

[Kent Tamura]

Created attachment 72917 [details]
Patch 1

[Kent Tamura]

Comment on attachment 72917 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=72917&action=review

> WebCore/html/ValidationMessage.cpp:116
> +    // FIXME: Adjust the bubble location.

I thought we needed to update the bubble location periodically in order to follow the target element location.  But without the location adjustment (the curent code),
 - The default bubble position is nice.  It's put on the bottom of the target element.
 - If the target element is moved, the bubble automatically follows it.

So, I think we don't need to adjust the bubble location now.

[Build Bot]

Attachment 72917 [details] did not build on win:
Build output: http://queues.webkit.org/results/5236008

[WebKit Review Bot]

Attachment 72917 [details] did not build on win:
Build output: http://queues.webkit.org/results/5343006

[Kent Tamura]

Created attachment 73033 [details]
Patch 2 (Windows build fix)

[Erik Arvidsson]

Comment on attachment 73033 [details]
Patch 2 (Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=73033&action=review

> WebCore/html/ValidationMessage.cpp:112
> +    renderer->style()->setOpacity(0.9f);

Is there a reason why this is not part of the css?

[Kent Tamura]

Comment on attachment 73033 [details]
Patch 2 (Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=73033&action=review

>> WebCore/html/ValidationMessage.cpp:112
>> +    renderer->style()->setOpacity(0.9f);
> 
> Is there a reason why this is not part of the css?

Ah, It's the remains of attempting to use CSS transition.  I'll move it to html.css.

[Dimitri Glazkov (Google)]

Comment on attachment 73033 [details]
Patch 2 (Windows build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=73033&action=review

I know you want to get the validation messages shipped, but I worry that the further use of this pattern will bring more subtle crashes. How about we try to first finish bug 48698 and then tackle this again?

> WebCore/ChangeLog:20
> +        new behavior is strongly timing-dependent.

How will we test this?

> WebCore/html/ValidationMessage.h:64
> +    RefPtr<HTMLElement> m_bubble;
> +    RefPtr<HTMLElement> m_bubbleTopOuter;
> +    RefPtr<HTMLElement> m_bubbleTopInner;
> +    RefPtr<HTMLElement> m_bubbleMessage;

I am very concerned that we keep repeating the anti-pattern where the shadow DOM elements are hung off RenderObjects or in this case, a random object -- all at the same time as I am actively trying to eliminate it. Given that ValidationMessage's lifetime is tied to that of HTMLFormControlElement, this seems only marginally more sane.

[Kent Tamura]

(In reply to comment #11)
> (From update of attachment 73033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73033&action=review
> 
> I know you want to get the validation messages shipped, but I worry that the further use of this pattern will bring more subtle crashes. How about we try to first finish bug 48698 and then tackle this again?

Do you have estimation when bug 46698 is completed?

> > WebCore/ChangeLog:20
> > +        new behavior is strongly timing-dependent.
> 
> How will we test this?

We need a manual test, or need to implement a way to stop a timer in DRT.

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 73033 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=73033&action=review
> > 
> > I know you want to get the validation messages shipped, but I worry that the further use of this pattern will bring more subtle crashes. How about we try to first finish bug 48698 and then tackle this again?
> 
> Do you have estimation when bug 46698 is completed?

I am actively working on it, hoping to have the basic framework completed in a couple of weeks. I have a patch that converts the slider to use the "natural shadow DOM" in bug 44907, but this patch is too large so I am breaking it up into smaller bits.

> 
> > > WebCore/ChangeLog:20
> > > +        new behavior is strongly timing-dependent.
> > 
> > How will we test this?
> 
> We need a manual test, or need to implement a way to stop a timer in DRT.

[Kent Tamura]

Comment on attachment 73033 [details]
Patch 2 (Windows build fix)

ok, I'll wait for Dimitri's changes.

[Kent Tamura]

Created attachment 78660 [details]
Patch 3 (follow recent shadow DOM changes)

[Dimitri Glazkov (Google)]

Comment on attachment 78660 [details]
Patch 3 (follow recent shadow DOM changes)

View in context: https://bugs.webkit.org/attachment.cgi?id=78660&action=review

Yay, first clean-slate new shadow DOM implementation! Comments below:

> Source/WebCore/html/ValidationMessage.cpp:77
> +    m_bubbleMessage->removeAllChildren();

Why do you need this?

> Source/WebCore/html/ValidationMessage.cpp:118
> +    RenderObject* hostRenderer = host->renderer();
> +    ASSERT(hostRenderer);

This should not be necessary anymore.

> Source/WebCore/html/ValidationMessage.cpp:121


Be careful here. Until we have a better API to apply multiple shadows, you will be clobbering whatever was there before. And most inputs will have something there. Soo... we probably need to invent some way to _add_ a validation message, not replace existing shadow DOM with it.

> Source/WebCore/html/ValidationMessage.cpp:127
> +    host->setShadowRoot(m_bubble);
> +    RefPtr<RenderStyle> style = doc->styleSelector()->styleForElement(m_bubble.get(), hostRenderer->style(), false);
> +    m_bubble->setRenderer(m_bubble->createRenderer(hostRenderer->renderArena(), style.get()));
> +    m_bubble->renderer()->setStyle(style.release());
> +    m_bubble->setAttached();
> +    m_bubble->setInDocument();
> +    hostRenderer->addChild(m_bubble->renderer());

You shouldn't need any of this anymore.

> Source/WebCore/html/ValidationMessage.cpp:133
> +    // FIXME: We need to use different qualified names for the following three
> +    // elements. CSSStyleSelector has a problem that each of elements with the
> +    // same hierarchy and different pseudo IDs uses the style of the pseudo ID
> +    // for the first element.

This sounds like a bug we should fix early on, not work around it.

> Source/WebCore/html/ValidationMessage.cpp:158
> +        m_bubble->detach();

Shouldn't need this anymore.

[Kent Tamura]

Created attachment 78886 [details]
Patch 4

[WebKit Review Bot]

Attachment 78886 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:984:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

Comment on attachment 78660 [details]
Patch 3 (follow recent shadow DOM changes)

View in context: https://bugs.webkit.org/attachment.cgi?id=78660&action=review

Thank you for reviewing.

>> Source/WebCore/html/ValidationMessage.cpp:77
>> +    m_bubbleMessage->removeAllChildren();
> 
> Why do you need this?

We have a code path updating validation message without building the bubble DOM tree.
We need to erase the previous validation message in that case.

>> Source/WebCore/html/ValidationMessage.cpp:121
>> +    host->setShadowRoot(m_bubble);
> 
> Be careful here. Until we have a better API to apply multiple shadows, you will be clobbering whatever was there before. And most inputs will have something there. Soo... we probably need to invent some way to _add_ a validation message, not replace existing shadow DOM with it.

Ah, right.
Actually, this code didn't work with your type=range change.
I have updated the code so that it works even if the host already has a shadowRoot.

>> Source/WebCore/html/ValidationMessage.cpp:127
>> +    hostRenderer->addChild(m_bubble->renderer());
> 
> You shouldn't need any of this anymore.

I tried without these lines, and unfortunately it didn't work.
RenderTextControl::canHaveChildren() returns false.  So Node::createRenderIfNeeded() didn't create a renderer.  I have change Node::createRenderIfNeeded() so that it always allows child renderers for a shadow root.

>> Source/WebCore/html/ValidationMessage.cpp:133
>> +    // for the first element.
> 
> This sounds like a bug we should fix early on, not work around it.

Fixed.

>> Source/WebCore/html/ValidationMessage.cpp:158
>> +        m_bubble->detach();
> 
> Shouldn't need this anymore.

Removed.

[Dimitri Glazkov (Google)]

Comment on attachment 78886 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=78886&action=review

Let's give it a spin. I am not sure yet whether the design with the holder class (ValidationMessage) is what we should use, but I don't know for sure. And please appease the style elf. He's angry.

> Source/WebCore/css/CSSStyleSelector.cpp:984
> +            (s->shadowPseudoId() == m_element->shadowPseudoId()) &&

Yay! Awesome.

> Source/WebCore/dom/Node.cpp:1384
> +    if (parentRenderer && (parentRenderer->canHaveChildren() || isShadowRoot()) && parent->childShouldCreateRenderer(this)) {

This is wrong, because we do need to respect canHaveChildren() on the renderer. Or do we? This is an interesting question. Can you at least put a FIXME at the top and file a bug to investigate this. So far, this is still a hack.

> Source/WebCore/html/ValidationMessage.cpp:120
> +    // FIXME: We need a way to host multiple shadow roots in a single node.

In XBL2, this is solved with inheritance, not multiple shadow roots. But yeah, this looks like a hack.

> Source/WebCore/html/ValidationMessage.cpp:125
> +        m_bubble->attach();

Shouldn't need this anymore. All attachment/detachment is done through the standard means. Perhaps we could just setNeedsStyleRecalc or something like that? At least put a FIXME here to investigate.

[Kent Tamura]

Comment on attachment 78886 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=78886&action=review

Thank you!
Landed with some modifications: http://trac.webkit.org/changeset/75770

>> Source/WebCore/dom/Node.cpp:1384
>> +    if (parentRenderer && (parentRenderer->canHaveChildren() || isShadowRoot()) && parent->childShouldCreateRenderer(this)) {
> 
> This is wrong, because we do need to respect canHaveChildren() on the renderer. Or do we? This is an interesting question. Can you at least put a FIXME at the top and file a bug to investigate this. So far, this is still a hack.

I have filed a bug: https://bugs.webkit.org/show_bug.cgi?id=52423
and added a comment to the code.

>> Source/WebCore/html/ValidationMessage.cpp:120
>> +    // FIXME: We need a way to host multiple shadow roots in a single node.
> 
> In XBL2, this is solved with inheritance, not multiple shadow roots. But yeah, this looks like a hack.

I updated the comment.

>> Source/WebCore/html/ValidationMessage.cpp:125
>> +        m_bubble->attach();
> 
> Shouldn't need this anymore. All attachment/detachment is done through the standard means. Perhaps we could just setNeedsStyleRecalc or something like that? At least put a FIXME here to investigate.

I add a FIXME comment.