Bug 66937 - Padding size of input type search element is not locked on Webkit-GTK
Summary: Padding size of input type search element is not locked on Webkit-GTK
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-25 07:15 PDT by Sameer Patil
Modified: 2013-10-11 07:54 PDT (History)
6 users (show)

See Also:


Attachments
Padding test for input type search element (659 bytes, text/html)
2011-08-25 07:15 PDT, Sameer Patil
no flags Details
Safari and Webkit GTK comparison (30.50 KB, image/jpeg)
2011-08-25 07:16 PDT, Sameer Patil
no flags Details
Patch (4.44 KB, patch)
2011-08-25 07:25 PDT, Sameer Patil
no flags Details | Formatted Diff | Diff
Patch (5.99 KB, patch)
2011-09-06 06:39 PDT, Sameer Patil
gns: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Focus ring comparison (with and without setting outline offset when input search is active) (41.50 KB, image/jpeg)
2011-09-06 07:01 PDT, Sameer Patil
no flags Details
Input and Input search focus ring test (569 bytes, text/html)
2011-09-13 05:09 PDT, Sameer Patil
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sameer Patil 2011-08-25 07:15:36 PDT
Created attachment 105179 [details]
Padding test for input type search element

Padding size of input type element is not locked on Webkit-GTK as like Safari.

Please check attached test page and screen shot comparing Safari and Webkit-GTK behavior.
Here padding of input element type search is locked on Safari, same is not observed on Webkit-GTK.


Tested on:
Windows Safari (5.1):Passed
Windows IE(6.0):Passed
Linux Mozilla Firefox(6.0):Failed
Linux Opera(11.50):Failed
Comment 1 Sameer Patil 2011-08-25 07:16:24 PDT
Created attachment 105180 [details]
Safari and Webkit GTK comparison
Comment 2 Sameer Patil 2011-08-25 07:25:26 PDT
Created attachment 105182 [details]
Patch
Comment 3 Martin Robinson 2011-08-30 09:35:48 PDT
Comment on attachment 105182 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        * platform/gtk/RenderThemeGtk.cpp:
> +        (WebCore::RenderThemeGtk::adjustSearchFieldStyle):

Please fill these in.

> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:375
> +    if (e && e->focused() && e->document()->frame()->selection()->isFocusedAndActive())
> +        style->setOutlineOffset(-2);

I don't see this on other platforms at all. It seems unrelated. Why is it here?

> LayoutTests/fast/forms/search-styled-padding.html:15
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> +   "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
> +<head>
> +<style>
> +.inputtext,
> +.inputpassword{border:1px solid #bdc7d8;font-family:"lucida grande", tahoma, verdana, arial, sans-serif;font-size:11px;padding:3px;}
> +.inputsearch{background:white url(./resources/apple.gif) no-repeat 3px 4px;padding-left:17px;padding-bottom: 100px;}
> +</style>
> +
> +</head>
> +<body>
> +<input type="search" class="inputtext inputsearch" title="Search for Events" placeholder="Search for Events" id="q_dashboard" name="q" value="" results="10" />
> +</body>
> +</html>

It seems like this test case could be greatly simplified. Should a text input type="search" field with some padding suffice? Is this covered by any existing tests? Be sure to generate your results with HEAD, as we've switched to GTK+ 3.x now.
Comment 4 Sameer Patil 2011-09-06 06:39:55 PDT
Created attachment 106411 [details]
Patch
Comment 5 Sameer Patil 2011-09-06 06:58:03 PDT
Comment on attachment 105182 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        (WebCore::RenderThemeGtk::adjustSearchFieldStyle):
> 
> Please fill these in.

Added details regarding change.

>> Source/WebCore/platform/gtk/RenderThemeGtk.cpp:375
>> +        style->setOutlineOffset(-2);
> 
> I don't see this on other platforms at all. It seems unrelated. Why is it here?

This has been done for focusring. It has been there on windows platform(WebCore/rendering/RenderThemeWin.cpp). If focus ring is enabled and if we do active input type search element by clicking it then focus ring will look outset. Please check screenshot showing comparison of focusring behavior when we do and don't set outline offset on input type search element.

>> LayoutTests/fast/forms/search-styled-padding.html:15
>> +</html>
> 
> It seems like this test case could be greatly simplified. Should a text input type="search" field with some padding suffice? Is this covered by any existing tests? Be sure to generate your results with HEAD, as we've switched to GTK+ 3.x now.

I have added simplified Layout test for input type="search" element. All the parameters(padding size) of test is not covered by single test for input type search elements so added one. Yes I have generated the test results with the head.
Comment 6 Sameer Patil 2011-09-06 07:01:20 PDT
Created attachment 106413 [details]
Focus ring comparison (with and without setting outline offset when input search is active)
Comment 7 WebKit Review Bot 2011-09-06 07:58:33 PDT
Comment on attachment 106411 [details]
Patch

Attachment 106411 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9591872
Comment 8 Martin Robinson 2011-09-11 08:53:49 PDT
(In reply to comment #6)
> Created an attachment (id=106413) [details]
> Focus ring comparison (with and without setting outline offset when input search is active)

Your GTK+ theme is odd, so I can't really parse it right away. Is the red line the focus ring? Why do we want the focus ring to overlap the widget itself. GTK+ themes specify where the focus ring goes, why shouldn't we obey them?
Comment 9 Sameer Patil 2011-09-13 05:07:18 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=106413) [details] [details]
> > Focus ring comparison (with and without setting outline offset when input search is active)
> 
> Your GTK+ theme is odd, so I can't really parse it right away. Is the red line the focus ring? Why do we want the focus ring to overlap the widget itself. GTK+ themes specify where the focus ring goes, why shouldn't we obey them?
Yes red line is focus ring, which i set through html.css to demonstrate. Focus ring set through outline property from html.css. I am attaching html page showing comparison of focus ring for input and input search element. I had set the outline(color,width) in css in same html page to simulate focus ring active state for input and input type search element.I didn't get where the gtk+ theme rules have been defined for focus ring on input or any focusable element? Do you mean any specific css file or RenderThemeGtk?
Comment 10 Sameer Patil 2011-09-13 05:09:55 PDT
Created attachment 107166 [details]
Input and Input search focus ring test
Comment 11 Martin Robinson 2011-09-14 08:48:25 PDT
(In reply to comment #10)

> Yes red line is focus ring, which i set through html.css to demonstrate. Focus ring set through outline property from html.css. I am attaching html page showing comparison of focus ring for input and input search element. I had set the outline(color,width) in css in same html page to simulate focus ring active state for input and input type search element.I didn't get where the gtk+ theme rules have been defined for focus ring on input or any focusable element? Do you mean any specific css file or RenderThemeGtk?

Take a look at RenderThemeGtk3.cpp. focus-line-width and focus-padding affect how focus rings appear.  Please ensure that your changes do not negatively affect theming. Does WebCore take over drawing focus rings in some CSS theming situations?
Comment 12 Gustavo Noronha (kov) 2013-10-11 07:54:14 PDT
Comment on attachment 106411 [details]
Patch

Been a while since a review was done asking for some things to be fixed/ensured, I'm setting r- for now, please send an updated patch if it still makes sense.