Bug 80580 – Add TextFieldDecorator and TextFieldDecorationElement

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Add TextFieldDecorator and TextFieldDecorationElemeent

[Kent Tamura]

Created attachment 130794 [details]
WIP

[Dimitri Glazkov (Google)]

Comment on attachment 130794 [details]
WIP

This looks really great, but this code will become obsolete as soon as we start supporting multiple shadow DOM trees on the text inputs. Once that's in place, we should be able to simply add another shadow DOM tree on top of existing type and put the decoration there. Perhaps you could file a bug on this? We (the Shadow DOM folks) will take care of the refactoring once the stuff is ready.

[Kent Tamura]

(In reply to comment #2)
> This looks really great, but this code will become obsolete as soon as we start supporting multiple shadow DOM trees on the text inputs. Once that's in place, we should be able to simply add another shadow DOM tree on top of existing type and put the decoration there. Perhaps you could file a bug on this? We (the Shadow DOM folks) will take care of the refactoring once the stuff is ready.

I don't think multiple shadow DOM trees will help this much.

When one wants to add a decoration button, he needs to do:
 - Provide a custom Element class to add code for mouse events
 - Add shadowPseudoId and default style in html.css if he doesn't use scoped style.
 - Add new -webkit-appearance value and painting code for it if the appearance can not be represented by pure CSS
 - Add code to RenderTextControlSingleLine.cpp if the button affects the size of the host <input>
 - Add the custom Element instance to the host <input> as a shadow element

It seems multiple shadow DOM doesn't improve these tasks.

[Kent Tamura]

Created attachment 130987 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> (In reply to comment #2)
> > This looks really great, but this code will become obsolete as soon as we start supporting multiple shadow DOM trees on the text inputs. Once that's in place, we should be able to simply add another shadow DOM tree on top of existing type and put the decoration there. Perhaps you could file a bug on this? We (the Shadow DOM folks) will take care of the refactoring once the stuff is ready.
> 
> I don't think multiple shadow DOM trees will help this much.
> 
> When one wants to add a decoration button, he needs to do:
>  - Provide a custom Element class to add code for mouse events
>  - Add shadowPseudoId and default style in html.css if he doesn't use scoped style.
>  - Add new -webkit-appearance value and painting code for it if the appearance can not be represented by pure CSS
>  - Add code to RenderTextControlSingleLine.cpp if the button affects the size of the host <input>
>  - Add the custom Element instance to the host <input> as a shadow element
> 
> It seems multiple shadow DOM doesn't improve these tasks.

Sure, but all these are standard, typical things one would need to do to create a functional shadow DOM element. What you're proposing here is a neat, but completely new way and highly specialized way of doing things.

Additionally, if this is just a plain element, I can animate it, and use CSS to lay things out.

I am not objecting to this patch, just trying to suggest that perhaps specialization is not always the best route.

[Hajime Morrita]

Maybe Shinya can help here.

We are going to support multiple shadow roots for non-trivial elements like controls and images.
If we have some concrete usecase to "decorating" such stuff, we can take it as the first client of 
our change.

In that process, some C++ utility classes could be introduced to build such multiple shadows easily.
It'll be like what we did when we introduced ShadowRoot. Let's start from real case, then abstract it out.

Well, I admit that this is highly shadow-biased opinion but...

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> Maybe Shinya can help here.
> 
> We are going to support multiple shadow roots for non-trivial elements like controls and images.
> If we have some concrete usecase to "decorating" such stuff, we can take it as the first client of 
> our change.
> 
> In that process, some C++ utility classes could be introduced to build such multiple shadows easily.
> It'll be like what we did when we introduced ShadowRoot. Let's start from real case, then abstract it out.
> 
> Well, I admit that this is highly shadow-biased opinion but...

Me too! Kent-san, we apologize for proselytizing shadow DOM so much :)

Please connect with Shinya and Morrita and chat with them. I trust that you guys can figure this out f2f.

[Kent Tamura]

I discussed this with morrita and shinyak.
I understand the multiple shadow trees feature can be applied to this.  It would make some benefit if shadow DOM operations were exposed via WebKit API, or if we supported <style scoped> by default.  It makes no benefit at this moment, but we can use it.

I'll make the patch simpler, but the basic structure won't be changed.

[Kent Tamura]

Created attachment 133248 [details]
WIP 2

Simplified

[Kent Tamura]

Comment on attachment 133248 [details]
WIP 2

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

> Source/WebCore/page/ChromeClient.h:314
> +        virtual void addTextFieldDecorationsTo(HTMLInputElement*) { }

ChromeClientChromium::addTextFieldDecorationsTo() implementation will be like:

add another ShadowRoot;
shadowRoot->appendChild(HTMLShadowElement::create());
for (i = 0; i < webView->textFieldDecorators().size(); ++i)
    shadowRoot->appendChild(TextFieldDecorationElement::create(input->document(), webview->textFieldDecorators[i]))

[Dimitri Glazkov (Google)]

Comment on attachment 133248 [details]
WIP 2

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

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:59
> +    virtual void paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOffset) OVERRIDE

I am probably just being dense, but... can you explain why we need custom painting here? Can't we just let style recalc, etc. Take care of this?

[Kent Tamura]

(In reply to comment #11)
> (From update of attachment 133248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133248&action=review
> 
> > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:59
> > +    virtual void paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOffset) OVERRIDE
> 
> I am probably just being dense, but... can you explain why we need custom painting here? Can't we just let style recalc, etc. Take care of this?

[Kent Tamura]

Comment on attachment 133248 [details]
WIP 2

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

>>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:59
>>> +    virtual void paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOffset) OVERRIDE
>> 
>> I am probably just being dense, but... can you explain why we need custom painting here? Can't we just let style recalc, etc. Take care of this?
> 
> 

I'd like to draw an Image object for TextFieldDecorationElement.  AFAIK, there are two ways to draw an Image object for an element content.
* Use a custom Renderer like this
* Use -webkit-appearance, and implement a paint function of RenderTheme like the previos patch

I switched to the first way to prevent many platforms from depending TextFieldDecorationElement.*.


I don't like to use the standard ways to paint images such as <img src=> and background-image because making such images URL-accessible is not efficient.

[Kent Tamura]

(In reply to comment #13)
> I'd like to draw an Image object for TextFieldDecorationElement.  AFAIK, there are two ways to draw an Image object for an element content.
> * Use a custom Renderer like this
> * Use -webkit-appearance, and implement a paint function of RenderTheme like the previos patch

I have just found a way to set an Image object to HTMLImageElement.  I'll try it.

[Kent Tamura]

Created attachment 133438 [details]
Patch 2

Drop a custom renderer

[Kent Tamura]

Committed r111930: <http://trac.webkit.org/changeset/111930>