Bug 66937

Summary:

Padding size of input type search element is not locked on Webkit-GTK

Product:

WebKit

Reporter:

Sameer Patil <mkrp87>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

dglazkov, eric, gustavo, mrobinson, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
Padding test for input type search element	none
Safari and Webkit GTK comparison	none
Patch	none
Patch	gustavo: review-, webkit.review.bot: commit-queue-
Focus ring comparison (with and without setting outline offset when input search is active)	none
Input and Input search focus ring test	none

Sameer Patil

Reported 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

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	gustavo: 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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Sameer Patil

Comment 1 2011-08-25 07:16:24 PDT

Created attachment 105180 [details] Safari and Webkit GTK comparison

Sameer Patil

Comment 2 2011-08-25 07:25:26 PDT

Created attachment 105182 [details] Patch

Martin Robinson

Comment 3 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.

Sameer Patil

Comment 4 2011-09-06 06:39:55 PDT

Created attachment 106411 [details] Patch

Sameer Patil

Comment 5 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.

Sameer Patil

Comment 6 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)

WebKit Review Bot

Comment 7 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

Martin Robinson

Comment 8 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?

Sameer Patil

Comment 9 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?

Sameer Patil

Comment 10 2011-09-13 05:09:55 PDT

Created attachment 107166 [details] Input and Input search focus ring test

Martin Robinson

Comment 11 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?

Gustavo Noronha (kov)

Comment 12 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.

Note You need to log in before you can comment on or make changes to this bug.