[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 #9 from Garrett Casto <gcasto at chromium.org>  2012-05-16 15:17:58 PST ---
(In reply to comment #8)
> (From update of attachment 142294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142294&action=review
> 
> >>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:143
> >>> +        style->setVisibility(HIDDEN);
> >> 
> >> If you need this code, something has gone horribly wrong.
> > 
> > So I don't understand the distinction between styles set by SetInlineStyleProperty and those set via RenderStyle, but just setting the inline style (either visibility:hidden or display:none) will not cause the element to disappear. As far as I can tell it has to be set here, and propagating from the inline style just seemed like the easiest way to accomplish this. Like I said, I don't understand why because I don't understand the distinction between them, but it's how it works at the moment.
> > 
> > The second line is because currently if the HTMLInputElement is hidden, the decorator still shows up. This doesn't seem like it should happen either.
> 
> When you set inline style on an element, the style recalculation will be triggered and the correct style should be created for TextFieldDecorationElement. Unfortunately, the customStyleForRenderer method short-circuits this process and forces you to do gnarly hacks like this. The problem with not fixing this is that _every_ time you need to modify a style on the TextFieldDecorationElement, you will have to add a little condition just like this to customStyleForRenderer. That's just bad design. Not your design, but still unacceptable. Kent-san, can we please change this to something that is not so rigid?
> 
> >>> Source/WebKit/chromium/public/WebStyledElement.h:32
> >>> +#define WebStyledElement_h
> >> 
> >> As I mentioned before, it's not the right approach. Exposing StyledElement in WebKit API is an overkill for this solution.
> >> 
> >> Perhaps instead we could morph the API to be a list of decorators in a document?
> >> 
> >> Then, we can listen to normal document events and react to them by iterating through the decorators and operating on them. In other words, instead of exposing WebStyledElement, expose the WebTextFieldDecorator? WDYT?
> > 
> > I'm not sure I understand what you are suggesting. Exposing WebTextFieldDecorator instead of WebStyledElement seems fine to me, though we can do that by just changing the function definition of decorationAdded(). It seems like you are suggesting something larger. How exactly are you proposing to change WebTextFieldDecoratorClient?
> 
> The design that I am proposing is:
> 
> * Adding textfield decorators should be limited to simply initializing the shadow DOM subtrees with hidden elements. There are no decisions made about visibility during this, since it's too early to know the context (for your particular case).
> * We register a "load" or "DOMContentLoad" event listener
> * When the event fires, we iterate over all decorators and then make our decisions on whether the decorator should be visibile
> * WebTextFieldDecorator has a setVisible(bool) method, which we set when we make our decision
> 

So the second and third point here suggest to me something where ChromeClientImpl asks Chromium if a particular element should be made visible (when the event handler fires). The last point implies that Chromium can change the visibility of the decorator whenever. I'm not sure what I'm missing, but I do think that the Chromium code needs to be able to change the visibility of a decorator at arbitrary times, since in my case the client is not going to know if a decorator should be visible until it gets some information back from the browser process. Assuming we change WebStyledElement -> WebTextFieldDecorator and fix the style propagation issue, the current WebTextFieldDecoratorClient works fine for me. Do you not approve of the interface change to WebTextFieldDecoratorClient, or was there something else you were hoping to get out of this new design?

> If we need to do something more complex, like reacting when an input element is added or removed from the document tree, you need an accurate signal of when that happens. You can't rely on Chrome::addTextFieldDecorationsTo, since this will be called way too many times (and for most of them, not when you need it). Perhaps a MutationObserver would be useful here.

I do not think that I need anything more complex. It's possible that eventually I would want this to be able to deal with dynamically generated forms, but first the password manager will need to deal with them.

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