<rdar://problem/9271742>
Created attachment 122141 [details] Patch
Created attachment 122169 [details] Patch Rebase onto ToT, and get rid of old, inapplicable comment
This is a new feature. You have to announce this in webkit-dev. http://www.webkit.org/coding/adding-features.html
Comment on attachment 122169 [details] Patch Attachment 122169 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11222259 New failing tests: svg/css/getComputedStyle-basic.xhtml fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html
I have some additional cleanup and test fixing to do. Obsoleting patch.
Created attachment 122381 [details] Patch
Comment on attachment 122381 [details] Patch Attachment 122381 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11149507 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html
Comment on attachment 122381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122381&action=review Can either of the new tests be done as ref tests? There still would be dependency on font size, but using a reference test would eliminate dependency on other aspects of text field appearance. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1809 > + if (style->controlTextOverflow()) > + return cssValuePool->createIdentifierValue(CSSValueEllipsis); > + return cssValuePool->createIdentifierValue(CSSValueClip); Coding style here is strange. We don’t use "!= 0" to compare with the constant zero, but we usually do use "!= x" when comparing with a named constant like TextOverflowClip. This was just copied and pasted from the case above, but it’s curious in both cases. It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values. > Source/WebCore/html/HTMLInputElement.cpp:693 > +bool HTMLInputElement::textShouldBeTruncated() const > +{ > + return document()->focusedNode() != this > + && isTextField() > + && renderer() > + && renderer()->style()->controlTextOverflow() == TextOverflowEllipsis; > +} Generally speaking new HTMLInputElement code should not use functions like isTextField. Instead, virtual functions should be added to the InputType class, and the logic should go there. This function is called by the HTMLInputElement’s renderer. This makes the isTextField() check and the renderer() check both redundant; the existence of the renderer already establishes both facts. And the style logic is the kind of thing that normally goes into renderer code. So I would suggest moving this entire function into RenderTextControlSingleLine and not making any changes to HTMLInputElement and HTMLTextFormControlElement at all. RenderTextControlSingleLine already has code that checks focus, specifically RenderTextControlSingleLine::capsLockStateMayHaveChanged, so this would not be something new. Looking at that function makes me wonder if you also need to check frame->selection()->isFocusedAndActive() as it does. What do we want these fields to look like when the field has focus, but the window does not. > Source/WebCore/html/HTMLInputElement.h:123 > + virtual bool textShouldBeTruncated() const; This override should be marked OVERRIDE. Or maybe this can be removed (see above) or be a non-virtual function (see below). > Source/WebCore/html/HTMLTextFormControlElement.h:56 > + virtual bool textShouldBeTruncated() const; Why is this needed? I don’t see any callers calling this on an HTMLTextFormControlElement*. They seem to have an HTMLInputElement*. If this can be removed, then textShouldBeTruncated can be made non-virtual.
Comment on attachment 122381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122381&action=review >> Source/WebCore/html/HTMLInputElement.cpp:693 >> +} > > Generally speaking new HTMLInputElement code should not use functions like isTextField. Instead, virtual functions should be added to the InputType class, and the logic should go there. > > This function is called by the HTMLInputElement’s renderer. This makes the isTextField() check and the renderer() check both redundant; the existence of the renderer already establishes both facts. And the style logic is the kind of thing that normally goes into renderer code. So I would suggest moving this entire function into RenderTextControlSingleLine and not making any changes to HTMLInputElement and HTMLTextFormControlElement at all. > > RenderTextControlSingleLine already has code that checks focus, specifically RenderTextControlSingleLine::capsLockStateMayHaveChanged, so this would not be something new. Looking at that function makes me wonder if you also need to check frame->selection()->isFocusedAndActive() as it does. What do we want these fields to look like when the field has focus, but the window does not. I will work on moving this check out to the renderer. As for the focus issue, I believe we want the text field to look the same as if it was not styled with this property. That is, if the placeholder is visible, changing the active window should show a clipped placeholder. If there is text, changing the active window should keep it clipped, and should not return the scroll position to the left (or right as the case may be). I will add a test for this. >> Source/WebCore/html/HTMLInputElement.h:123 >> + virtual bool textShouldBeTruncated() const; > > This override should be marked OVERRIDE. > > Or maybe this can be removed (see above) or be a non-virtual function (see below). Done. >> Source/WebCore/html/HTMLTextFormControlElement.h:56 >> + virtual bool textShouldBeTruncated() const; > > Why is this needed? I don’t see any callers calling this on an HTMLTextFormControlElement*. They seem to have an HTMLInputElement*. > > If this can be removed, then textShouldBeTruncated can be made non-virtual. Done. > LayoutTests/fast/css/webkit-control-text-overflow-focus-placeholder.html:16 > +</html> This test could be a ref test. I will convert it. > LayoutTests/fast/css/webkit-control-text-overflow-focus-value.html:16 > +</html> This test could be a ref test. I will convert it. > LayoutTests/fast/css/webkit-control-text-overflow-input.html:37 > + </p> I feel the above section might be overkill to try to do as a ref test, but... > LayoutTests/fast/css/webkit-control-text-overflow-input.html:65 > + </p> ... everything up to this point could be a ref test. I will split it out into another one. I also realize now that I forgot to add RTL tests.
(In reply to comment #8) > (From update of attachment 122381 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122381&action=review > > Can either of the new tests be done as ref tests? There still would be dependency on font size, but using a reference test would eliminate dependency on other aspects of text field appearance. > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1809 > > + if (style->controlTextOverflow()) > > + return cssValuePool->createIdentifierValue(CSSValueEllipsis); > > + return cssValuePool->createIdentifierValue(CSSValueClip); > > Coding style here is strange. We don’t use "!= 0" to compare with the constant zero, but we usually do use "!= x" when comparing with a named constant like TextOverflowClip. This was just copied and pasted from the case above, but it’s curious in both cases. > > It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values. > Forgot to comment on this-- I will look into this and see if there's a way to avoid it.
(In reply to comment #10) > > It’s mildly unpleasant that we can’t somehow use the code in CSSPrimitiveValueMappings.h to do this for us since it already has a switch statement to convert values. Offline I spoke with Andreas, who told me they might be doing other optimizations to make this code moot, but I've filed a bug, since this is a pattern pervasive in that file: bug 76305.
Created attachment 122513 [details] Patch
Comment on attachment 122513 [details] Patch Converted some tests to ref tests, added rtl cases, and got rid of some unnecessary cases. Moved all of the WebCore code to RenderTextControlSingleLine. The issue cited in CSSComputedStyleDeclaration has been punted to bug 73605.
Created attachment 122516 [details] Same patch as before, rebased on ToT for EWS
I disagree with this patch and believe we should just made text-overflow work this way instead of introducing a new CSS property. See my response to webkit-dev.
Switch to using text-overflow instead of custom property, based on discussion in webkit-dev and www-style lists.
Created attachment 123194 [details] Patch
Get rid of custom -webkit-control-text-overflow attribute. Moved tests from webkit-control-text... to text...
Comment on attachment 123194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123194&action=review > Source/WebCore/ChangeLog:3 > + Add vendor-specific CSS attribute that allows placeholder and value text to show an ellipsis when not focused Not true anymore
Comment on attachment 123194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123194&action=review >> Source/WebCore/ChangeLog:3 >> + Add vendor-specific CSS attribute that allows placeholder and value text to show an ellipsis when not focused > > Not true anymore doh! i'll retitle.
Committed r105451: http://trac.webkit.org/changeset/105451