[Webkit-unassigned] [Bug 86557] Allow WebTextFieldDecoratorClient to see applied decorations.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86557





--- Comment #27 from Garrett Casto <gcasto at chromium.org>  2012-05-24 11:35:41 PST ---
(In reply to comment #21)
> (From update of attachment 143145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143145&action=review
> 
> I think this is good. I would really like for Kent-san to look over the next iteration of the patch, since you and I came up with this idea together. Another pair of eyes wouldn't hurt :)
> 
> > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:69
> > +TextFieldDecorationElement* TextFieldDecorationElement::fromShadowRoot(ShadowRoot* shadowRoot)
> 
> Can you file a bug about making TextFieldDecorationElement _the_ ShadowRoot and add a FIXME comment referencing it?
> 
> // FIXME: After fixing http://bugs.webkit.org/...., this should be a simple cast.
> 

I've filled a bug, but I removed this comment for the moment as it seems like we can't actually make this a ShadowRoot. I can put this, or something similar back in if you want.

> > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:108
> > +    box->setInlineStyleProperty(CSSPropertyVisibility, CSSValueHidden);
> 
> I don't mind if you expose a flag in TextFieldDecorator that allows controlling whether the element is hidden or visible by default.
> 
> > Source/WebKit/chromium/public/WebTextFieldDecorationElement.h:54
> > +    // Make visible iff true.
> 
> Don't need this comment here.
> 
> > Source/WebKit/chromium/src/TextFieldDecoratorImpl.h:46
> > +    WebTextFieldDecoratorClient* decoratorClient();
> 
> Instead of exposing a client here, can we instead let it do the comparison inside:
> 
> WebTextfieldDecoratorClient::isClientFor(WebCore::TextFieldDecorator*);
> 
> This way, the casting to TextFieldDecoratorImpl becomes an implementation detail, not an assumption in WebInputElement.
> 

So I moved the cast and comparison into WebTextFieldDecoratorClient, but I believe that you still need this function (or something similar like isFromDecoratorClient(WebTextFieldDecoratorClient*))

> > Source/WebKit/chromium/src/WebInputElement.cpp:221
> > +    ShadowRoot* shadowRoot = unwrap<HTMLInputElement>()->shadow()->youngestShadowRoot();
> 
> I think the code just landed that lets you use Node::youngestShadowRoot()
> 
> > Source/WebKit/chromium/src/WebInputElement.cpp:228
> > +        shadowRoot->olderShadowRoot();
> 
> shadowRoot = shadowRoot->olderShadowRoot(); !!!
> 

Yikes. 

> > Source/WebKit/chromium/src/WebTextFieldDecorationElement.cpp:50
> > +    if (visible)
> > +        unwrap<TextFieldDecorationElement>()->setInlineStyleProperty(CSSPropertyVisibility,
> > +                                                                     CSSValueVisible);
> > +    else
> > +        unwrap<TextFieldDecorationElement>()->setInlineStyleProperty(CSSPropertyVisibility,
> > +                                                                     CSSValueHidden);
> 
> I think we can just use WebElement::setAttribute("style", "display:none|block") instead. This way, the patch will be smaller and we're not exposing any new WebCore functionality. WDYT?

I've made this change, though this does override the style set in TextFieldDecorationElement::decorate() yes? The only property that is currently getting set is -webkit-box-flex, but I'm not sure how important that is. It looks fine without it in testing anyway. Changing to display:none instead of visibility:hidden removed the mouse over issue that I was experiencing before, so that's good.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.