Allow WebTextFieldDecoratorClient to see applied decorations.
Created attachment 142128 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 142128 [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/WebKit/chromium/public/WebStyledElement.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 142294 [details] Patch
Fixed the style issue.
Comment on attachment 142294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142294&action=review > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:143 > + if (inlineStyle()->getPropertyValue(CSSPropertyVisibility) == "hidden" > + || inputStyle->visibility() == HIDDEN) > + style->setVisibility(HIDDEN); If you need this code, something has gone horribly wrong. > 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?
(In reply to comment #6) > (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 > > + if (inlineStyle()->getPropertyValue(CSSPropertyVisibility) == "hidden" > > + || inputStyle->visibility() == HIDDEN) > > + 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. > > 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?
Comment on attachment 142294 [details] Patch 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 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.
(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.
(In reply to comment #9) > > 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? > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > > 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. Great! :)
Comment on attachment 142294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142294&action=review >>>>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:143 >>>>> + style->setVisibility(HIDDEN); I don't remember why I used customStyleForRenderer() exactly. Probably we needed to call updateImage() when <input>'s disabled/readonly state is changed. If we moved updateImage() to another place, we would be able to use normal style resolution. - The width and height should work by specifying "width:1em; height:1em;" in UA stylesheet. - HTMLElement::hidden should work well if we use normal style resolution.
(In reply to comment #10) > (In reply to comment #9) > > > > 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? > > > > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > The request is actually already being made for autofill purposes. It happens in didFinishDocumentLoad, and I don't think that this would need to move. As you mentioned, addTextFieldDecorationsTo is too early for this as we don't have the complete form data. > > > 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. > > Great! :)
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #9) > > > > > > 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? > > > > > > > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > > > > The request is actually already being made for autofill purposes. It happens in didFinishDocumentLoad, and I don't think that this would need to move. As you mentioned, addTextFieldDecorationsTo is too early for this as we don't have the complete form data. Great, so this makes the API we need fairly simple: 1) a way to iterate through all elements per decorator in a document and; 2) a way to make some of them visible. > > > > > 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. > > > > Great! :)
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > > > > > 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? > > > > > > > > > > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > > > > > > > The request is actually already being made for autofill purposes. It happens in didFinishDocumentLoad, and I don't think that this would need to move. As you mentioned, addTextFieldDecorationsTo is too early for this as we don't have the complete form data. > > Great, so this makes the API we need fairly simple: > 1) a way to iterate through all elements per decorator in a document and; > 2) a way to make some of them visible. > I mentioned this before, but I think that the API that I've currently proposed (modulo WebStyledElement -> WebTextFieldDecorator and the style propagation problems) allows this, though the user does have to worry about the lifetimes of the Elements. Is the lifetime issue what you are worried about, or just general cleanliness of the API, or what? Are you proposing adding function to WebView to iterate through these elements? Assuming that you don't want the client to worry about lifetimes, there are two other issues. The first is that having a way to do lookups as well as iteration through these elements would be useful. I'm thinking in particular of when we get the IPC from the browser iterating through all of the decorated elements looking for the one that we want to show seems slightly silly. Relatedly, I'm actually not sure if we need iteration, as I currently accomplish this by just iterating through all the forms on a page and I'm not sure how much faster just iterating through all the decorated elements would be. Secondly, we would need to change handleClick() to include a WebTextFieldDecorator, since I need that to determine how to center the popup. Not a big change though. I would also like to say that I would prefer that we get _something_ checked in reasonably soon that works. I'm willing to cleanup the API, I would just like to be able to have a testable UI in Chromium sooner rather than later. > > > > > > > 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. > > > > > > Great! :)
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (In reply to comment #10) > > > > (In reply to comment #9) > > > > > > > > > > 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? > > > > > > > > > > > > > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > > > > > > > > > > The request is actually already being made for autofill purposes. It happens in didFinishDocumentLoad, and I don't think that this would need to move. As you mentioned, addTextFieldDecorationsTo is too early for this as we don't have the complete form data. > > > > Great, so this makes the API we need fairly simple: > > 1) a way to iterate through all elements per decorator in a document and; > > 2) a way to make some of them visible. > > > > I mentioned this before, but I think that the API that I've currently proposed (modulo WebStyledElement -> WebTextFieldDecorator and the style propagation problems) allows this, though the user does have to worry about the lifetimes of the Elements. Is the lifetime issue what you are worried about, or just general cleanliness of the API, or what? Are you proposing adding function to WebView to iterate through these elements? Yes. Holding on to references like this is madness. Consider this: at any point, a script on the page could change a text field into a checkbox (or any other element), and then you're stuck holding something you don't really want. > > Assuming that you don't want the client to worry about lifetimes, there are two other issues. The first is that having a way to do lookups as well as iteration through these elements would be useful. I'm thinking in particular of when we get the IPC from the browser iterating through all of the decorated elements looking for the one that we want to show seems slightly silly. Relatedly, I'm actually not sure if we need iteration, as I currently accomplish this by just iterating through all the forms on a page and I'm not sure how much faster just iterating through all the decorated elements would be. Secondly, we would need to change handleClick() to include a WebTextFieldDecorator, since I need that to determine how to center the popup. Not a big change though. > > I would also like to say that I would prefer that we get _something_ checked in reasonably soon that works. I'm willing to cleanup the API, I would just like to be able to have a testable UI in Chromium sooner rather than later. I understand. How about this -- when you look at any WebInputElement, you can ask for its decorator. This should allow you twiddle the visibility bits when iterating through forms?
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #12) > > > > (In reply to comment #10) > > > > > (In reply to comment #9) > > > > > > > > > > > > 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? > > > > > > > > > > > > > > > > Ah, I see. I've forgotten about the IPC thing. So you need to hold on to WebTextFieldDecorator until you get the data back from the browser process? When would you request this data? addTextFieldDecorationsTo is too early, right? > > > > > > > > > > > > > The request is actually already being made for autofill purposes. It happens in didFinishDocumentLoad, and I don't think that this would need to move. As you mentioned, addTextFieldDecorationsTo is too early for this as we don't have the complete form data. > > > > > > Great, so this makes the API we need fairly simple: > > > 1) a way to iterate through all elements per decorator in a document and; > > > 2) a way to make some of them visible. > > > > > > > I mentioned this before, but I think that the API that I've currently proposed (modulo WebStyledElement -> WebTextFieldDecorator and the style propagation problems) allows this, though the user does have to worry about the lifetimes of the Elements. Is the lifetime issue what you are worried about, or just general cleanliness of the API, or what? Are you proposing adding function to WebView to iterate through these elements? > > Yes. Holding on to references like this is madness. Consider this: at any point, a script on the page could change a text field into a checkbox (or any other element), and then you're stuck holding something you don't really want. > Just to be sure, are you worried about holding a reference to a WebInputElement or to a WebTextFieldDecorator? I'm wondering because I think that you need to hold onto a WebInputElement between the time we determine that we want to show the decorator and the time that we get the IPC from the browser saying that we can show it (assuming that we can get the WebTextFieldDecorator from the WebInputElement) > > > > Assuming that you don't want the client to worry about lifetimes, there are two other issues. The first is that having a way to do lookups as well as iteration through these elements would be useful. I'm thinking in particular of when we get the IPC from the browser iterating through all of the decorated elements looking for the one that we want to show seems slightly silly. Relatedly, I'm actually not sure if we need iteration, as I currently accomplish this by just iterating through all the forms on a page and I'm not sure how much faster just iterating through all the decorated elements would be. Secondly, we would need to change handleClick() to include a WebTextFieldDecorator, since I need that to determine how to center the popup. Not a big change though. > > > > I would also like to say that I would prefer that we get _something_ checked in reasonably soon that works. I'm willing to cleanup the API, I would just like to be able to have a testable UI in Chromium sooner rather than later. > > I understand. How about this -- when you look at any WebInputElement, you can ask for its decorator. This should allow you twiddle the visibility bits when iterating through forms? Seems reasonable to me.
(In reply to comment #16) > > Just to be sure, are you worried about holding a reference to a WebInputElement or to a WebTextFieldDecorator? I'm wondering because I think that you need to hold onto a WebInputElement between the time we determine that we want to show the decorator and the time that we get the IPC from the browser saying that we can show it (assuming that we can get the WebTextFieldDecorator from the WebInputElement) Yes, it's still not optimal, but I think we've done our best here. I believe there's code in AutoFill that deals with these types of problems. The engineers who wrote this spent a looooong time sifting through odd crashes around that. You don't want to be stuck in the same long tail :) > Seems reasonable to me. Yay! Progress.
Created attachment 143145 [details] Patch
This change reworks the API as we discussed, but doesn't solve the styling issue (so changing the visibility of a WebTextFieldDecorationElement currently doesn't do anything useful). I did change it so that the DIV element is always hidden, since it didn't seem to hurt and it prevents a problem where the decorator is hidden, but mousing over the element will give you a pointer instead of a text bar. However, I'm not sure why the DIV is there in the first place so I'm not entirely sure what the side effects of this are going to be.
One other thing that I just thought about. For my use case, I would prefer that the decoration defaulted to hidden, but I'm not if other users of this API would agree. Given the current setup it seems difficult to override the default for every element. The best thing that I can come up with is to iterate over the entire document once it's finished loading and set everything to invisible that I don't want to be shown. So, do we think that the API should allow for some easier way to specify the default styling of the decorator?
Comment on attachment 143145 [details] Patch 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. > 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. > 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(); !!! > 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?
(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. > > > 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. > How should this be exposed to chromium? Something like bool WebTextFieldDecoratorClient::isDefaultVisible()? > > 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. > > > 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(); !!! > > > 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?
(In reply to comment #22) > How should this be exposed to chromium? Something like > > bool WebTextFieldDecoratorClient::isDefaultVisible()? visibleByDefault sounds WebKitty.
Comment on attachment 143145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143145&action=review >>> 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. > > How should this be exposed to chromium? Something like > > bool WebTextFieldDecoratorClient::isDefaultVisible()? I'm not sure we can make TextFieldDecorationElement a ShadowRoot. If we'd like to avoid such digging, we can add id attribute to a TextFieldDecorationElement, and can use ShadowRoot::getElementById().
(In reply to comment #24) > (From update of attachment 143145 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143145&action=review > > >>> 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. > > > > How should this be exposed to chromium? Something like > > > > bool WebTextFieldDecoratorClient::isDefaultVisible()? > > I'm not sure we can make TextFieldDecorationElement a ShadowRoot. > If we'd like to avoid such digging, we can add id attribute to a TextFieldDecorationElement, and can use ShadowRoot::getElementById(). Well, you're right -- not a Shadow Root. However, it surely does not need to be an element three levels down from the Shadow Root.
Created attachment 143861 [details] Patch
(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.
Comment on attachment 143861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143861&action=review Looks almost ok. r- because of some nits. > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:118 > setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 0.0, CSSPrimitiveValue::CSS_NUMBER); > box->appendChild(this); > + > + if (visible) > + setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock); > + else > + setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone); This should be put before box->appendChild(this) to avoid style-recalc after renderer attachment. Also, we can write this as setInlineStyleProperty(CSSPropertyDisplay, visible ? CSSValueBlock : CSSValueNone); > Source/WebKit/chromium/src/TextFieldDecoratorImpl.cpp:64 > + > + nit: should be one blank line. > Source/WebKit/chromium/src/WebInputElement.cpp:236 > + if (decoration > + && decoratorClient->isClientFor(decoration->textFieldDecorator())) { nit: You don't need to wrap these line. You don't need { }. > Source/WebKit/chromium/src/WebTextFieldDecorationElement.cpp:49 > + if (visible) > + unwrap<TextFieldDecorationElement>()->setAttribute(HTMLNames::styleAttr, > + "display:block"); > + else > + unwrap<TextFieldDecorationElement>()->setAttribute(HTMLNames::styleAttr, > + "display:none"); You should use setInlineStyleProperty() like you did in TextFieldDecorationElement::decorate(). Setting style attribute is expensive.
(In reply to comment #28) > (From update of attachment 143861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143861&action=review > > Looks almost ok. r- because of some nits. > > > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:118 > > setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 0.0, CSSPrimitiveValue::CSS_NUMBER); > > box->appendChild(this); > > + > > + if (visible) > > + setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock); > > + else > > + setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone); > > This should be put before box->appendChild(this) to avoid style-recalc after renderer attachment. > Also, we can write this as setInlineStyleProperty(CSSPropertyDisplay, visible ? CSSValueBlock : CSSValueNone); > > > Source/WebKit/chromium/src/TextFieldDecoratorImpl.cpp:64 > > + > > + > > nit: should be one blank line. > > > Source/WebKit/chromium/src/WebInputElement.cpp:236 > > + if (decoration > > + && decoratorClient->isClientFor(decoration->textFieldDecorator())) { > > nit: You don't need to wrap these line. > You don't need { }. > > > Source/WebKit/chromium/src/WebTextFieldDecorationElement.cpp:49 > > + if (visible) > > + unwrap<TextFieldDecorationElement>()->setAttribute(HTMLNames::styleAttr, > > + "display:block"); > > + else > > + unwrap<TextFieldDecorationElement>()->setAttribute(HTMLNames::styleAttr, > > + "display:none"); > > You should use setInlineStyleProperty() like you did in TextFieldDecorationElement::decorate(). > Setting style attribute is expensive. I was originally using setInlineStyleProperty, but Dimitri recommended this instead. I'm fine with whichever. Dimitri, do you still think that this should use setAttribute?
(In reply to comment #29) > > You should use setInlineStyleProperty() like you did in TextFieldDecorationElement::decorate(). > > Setting style attribute is expensive. > > I was originally using setInlineStyleProperty, but Dimitri recommended this instead. I'm fine with whichever. Dimitri, do you still think that this should use setAttribute? I did, and I still think it's ok, since Web developers do this all the time -- but only if as means of removing the need for that whole extra file WebTextDecorationElement. Adding new WebKit API leads to pain of maintenance. If there are strong feelings, I would much rather expose setting inline style on WebElement.
(In reply to comment #30) > (In reply to comment #29) > > > You should use setInlineStyleProperty() like you did in TextFieldDecorationElement::decorate(). > > > Setting style attribute is expensive. > > > > I was originally using setInlineStyleProperty, but Dimitri recommended this instead. I'm fine with whichever. Dimitri, do you still think that this should use setAttribute? > > I did, and I still think it's ok, since Web developers do this all the time -- but only if as means of removing the need for that whole extra file WebTextDecorationElement. Adding new WebKit API leads to pain of maintenance. If there are strong feelings, I would much rather expose setting inline style on WebElement. Ahh, I totally misunderstood that comment then. I didn't realize that you wanted to get rid of WebTextFieldDecorationElement. I'm fine with that. For the moment I'll just use a WebElement and not expose setInlineStyleProperty. I can add that later if we so desire.
Created attachment 144111 [details] Patch
Comment on attachment 144111 [details] Patch ok
Comment on attachment 144111 [details] Patch Clearing flags on attachment: 144111 Committed r118601: <http://trac.webkit.org/changeset/118601>
All reviewed patches have been landed. Closing bug.